[libvirt] [PATCHv7 10/18] conf: Remove virDomainResctrlAppend and introduce virDomainResctrlNew

[John Ferlan]

On 11/6/18 4:51 AM, Huaqiang,Wang wrote:
> 
> 
> On 2018年11月06日 01:26, John Ferlan wrote:
>>
>> On 10/22/18 4:01 AM, Wang Huaqiang wrote:
>>> Introduced virDomainResctrlNew to do the most part of
>>> virDomainResctrlAppend
>>> and move the operation of appending resctrl to @def->resctrls out of
>>> function.
>>>
>>> Rather than rely on virDomainResctrlAppend to perform the allocation,
>>> move
>>> the onus to the caller and make use of virBitmapNewCopy for @vcpus and
>>> virObjectRef for @alloc, thus removing the need to set each to NULL
>>> after the
>>> call.
>>>
>>> Signed-off-by: Wang Huaqiang <huaqiang.wang at intel.com>
>>> ---
>>>   src/conf/domain_conf.c | 60
>>> +++++++++++++++++++++++++++++---------------------
>>>   1 file changed, 35 insertions(+), 25 deletions(-)
>>>
>>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>>> index e8e0adc..39bd396 100644
>>> --- a/src/conf/domain_conf.c
>>> +++ b/src/conf/domain_conf.c
>>> @@ -18920,26 +18920,22 @@
>>> virDomainCachetuneDefParseCache(xmlXPathContextPtr ctxt,
>>>   }
>>>     -static int
>>> -virDomainResctrlAppend(virDomainDefPtr def,
>>> -                       xmlNodePtr node,
>>> -                       virResctrlAllocPtr alloc,
>>> -                       virBitmapPtr vcpus,
>>> -                       unsigned int flags)
>>> +static virDomainResctrlDefPtr
>>> +virDomainResctrlNew(xmlNodePtr node,
>>> +                    virResctrlAllocPtr *alloc,
>>> +                    virBitmapPtr *vcpus,
>> Because we're not "stealing" @*alloc and/or @*vcpus, they do not need to
>> be passed by reference. I can change these.  There's some minor merge
>> impact in later patches too, but no big deal.
> 
> Agree. Please help make change.
> 
> 
>>
>>> +                    unsigned int flags)
>>>   {
>>>       char *vcpus_str = NULL;
>>>       char *alloc_id = NULL;
>>> -    virDomainResctrlDefPtr tmp_resctrl = NULL;
>>> -    int ret = -1;
>>> -
>>> -    if (VIR_ALLOC(tmp_resctrl) < 0)
>>> -        goto cleanup;
>>> +    virDomainResctrlDefPtr resctrl = NULL;
>>> +    virDomainResctrlDefPtr ret = NULL;
>>>         /* We need to format it back because we need to be consistent
>>> in the naming
>>>        * even when users specify some "sub-optimal" string there. */
>>> -    vcpus_str = virBitmapFormat(vcpus);
>>> +    vcpus_str = virBitmapFormat(*vcpus);
>>>       if (!vcpus_str)
>>> -        goto cleanup;
>>> +        return NULL;
>>>         if (!(flags & VIR_DOMAIN_DEF_PARSE_INACTIVE))
>>>           alloc_id = virXMLPropString(node, "id");
>>> @@ -18954,18 +18950,23 @@ virDomainResctrlAppend(virDomainDefPtr def,
>>>               goto cleanup;
>>>       }
>>>   
>>      /* NB: Callers assume new @alloc, need to fill in ID now */
>>
>> Not that it would prevent someone in the future from passing an @alloc
>> w/ ->id already filled in and overwriting, but at least for now it's not
>> the case.
> 
> Yes, it might happen.
> If @alloc->id is specified through XML and is not following the naming
> convention
>          virAsprintf(&alloc_id, "vcpus_%s", vcpus_str)
> 
> If you think it is necessary we might need to through a warning for this
> case.
> 

Let's see - virDomainResctrlNew has two callers:

1. virDomainCachetuneDefParse

   In this case, we "know" we have a new/empty @alloc because if
virDomainResctrlVcpuMatch found one, then there'd be a failure.

   The virDomainCachetuneDefParseCache calls don't seem to fill in
alloc->id, but virDomainResctrlNew will for the first time.

2. virDomainMemorytuneDefParse

   The virDomainResctrlVcpuMatch may find a preexisting @alloc, but
@new_alloc is set to true. The virDomainMemorytuneDefParseMemory won't
fill alloc->id. Then only if @new_alloc do we call virDomainResctrlNew

So I think both are safe "for now". If you want I could modify the
virResctrlAllocSetID code to :

    if (alloc->id) {
        virReportError(VIR_ERR_INTERNAL_ERROR,
                       _("Attempt to overwrite alloc->id='%s' with
id='%s'"),
                       alloc->id, id);
        return -1;
    }

Let me know.

John


>>
>> With the changes (that I can make),
>>
>> Reviewed-by: John Ferlan <jferlan at redhat.com>
>>
>> John
> 
> Thanks for review.
> Huaqiang
> 
>>
>> [...]
>