[libvirt] [PATCH 07/10] conf: refactor virDomainResctrlAppend
John Ferlan
jferlan at redhat.com
Tue Sep 11 16:39:30 UTC 2018
On 09/10/2018 02:13 PM, Wang, Huaqiang wrote:
>
>
>> -----Original Message-----
>> From: John Ferlan [mailto:jferlan at redhat.com]
>> Sent: Wednesday, September 5, 2018 11:49 PM
>> To: Wang, Huaqiang <huaqiang.wang at intel.com>; libvir-list at redhat.com
>> Cc: Feng, Shaohe <shaohe.feng at intel.com>; Niu, Bing <bing.niu at intel.com>;
>> Ding, Jian-feng <jian-feng.ding at intel.com>; Zang, Rui <rui.zang at intel.com>
>> Subject: Re: [libvirt] [PATCH 07/10] conf: refactor virDomainResctrlAppend
>>
>>
>>
>> On 08/27/2018 07:23 AM, Wang Huaqiang wrote:
>>> Changed the interface from
>>> virDomainResctrlAppend(virDomainDefPtr def,
>>> xmlNodePtr node,
>>> virResctrlAllocPtr alloc,
>>> virBitmapPtr vcpus,
>>> unsigned int flags); to
>>> virDomainResctrlAppend(virDomainDefPtr def,
>>> xmlNodePtr node,
>>> virDomainResctrlDefPtr resctrl,
>>> unsigned int flags);
>>>
>>> Changes will let virDomainRestrlAppend pass through more information
>>> with virDomainResctrlDefPtr, such as monitoring groups associated with
>>> the allocation.
>>>
>>> Signed-off-by: Wang Huaqiang <huaqiang.wang at intel.com>
>>> ---
>>> src/conf/domain_conf.c | 48
>>> ++++++++++++++++++++++++++++++++++--------------
>>> 1 file changed, 34 insertions(+), 14 deletions(-)
>>>
>>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index
>>> bde9fef..9a65655 100644
>>> --- a/src/conf/domain_conf.c
>>> +++ b/src/conf/domain_conf.c
>>> @@ -19247,17 +19247,21 @@
>>> virDomainCachetuneDefParseCache(xmlXPathContextPtr ctxt, static int
>>> virDomainResctrlAppend(virDomainDefPtr def,
>>> xmlNodePtr node,
>>> - virResctrlAllocPtr alloc,
>>> - virBitmapPtr vcpus,
>>> + virDomainResctrlDefPtr resctrl,
>>> unsigned int flags) {
>>> char *vcpus_str = NULL;
>>> char *alloc_id = NULL;
>>> - virDomainResctrlDefPtr tmp_resctrl = NULL;
>>> + virResctrlAllocPtr alloc = NULL;
>>> + virBitmapPtr vcpus = NULL;
>>
>> No need for locals here - just change to resctrl->{alloc|vcpus}
>>
>
> Local varaibles 'alloc', 'vcpus' will be removed.
>
>>> +
>>> int ret = -1;
>>>
>>> - if (VIR_ALLOC(tmp_resctrl) < 0)
>>> - goto cleanup;
>>> + if (!resctrl)
>>> + return -1;
>>
>> Again, here we have a programming error without an error message which
>> results in a generic libvirt an error occurred. Either create a specific error
>> message or "assume" that your caller has done the right thing.
>>
>
> This is a static function, the caller will ensure its safety.
> How about removing these two lines?
>
Seems reasonable...
>>> +
>>> + alloc = virObjectRef(resctrl->alloc);
>>
>> Yikes, how many Ref's are we taking on this? [1]
>>
>> I don't think this is necessary since we Unref later and currently both callers do
>> the Ref
>>
>
> Will remove this local variable along with this Ref. But the caller's
> Ref/unRef remained.
> Thanks.
>
>>> + vcpus = resctrl->vcpus;
>>>
>>> /* We need to format it back because we need to be consistent in the
>> naming
>>> * even when users specify some "sub-optimal" string there. */ @@
>>> -19281,15 +19285,12 @@ virDomainResctrlAppend(virDomainDefPtr def,
>>> if (virResctrlAllocSetID(alloc, alloc_id) < 0)
>>> goto cleanup;
>>>
>>> - tmp_resctrl->vcpus = vcpus;
>>> - tmp_resctrl->alloc = alloc;
>>> -
>>> - if (VIR_APPEND_ELEMENT(def->resctrls, def->nresctrls, tmp_resctrl) < 0)
>>> + if (VIR_APPEND_ELEMENT(def->resctrls, def->nresctrls, resctrl) <
>>> + 0)
>>> goto cleanup;
>>>
>>> ret = 0;
>>> cleanup:
>>> - virDomainResctrlDefFree(tmp_resctrl);
>>> + virObjectUnref(alloc);
>>> VIR_FREE(alloc_id);
>>> VIR_FREE(vcpus_str);
>>> return ret;
>>> @@ -19306,6 +19307,8 @@ virDomainCachetuneDefParse(virDomainDefPtr
>> def,
>>> xmlNodePtr *nodes = NULL;
>>> virBitmapPtr vcpus = NULL;
>>> virResctrlAllocPtr alloc = NULL;
>>> + virDomainResctrlDefPtr tmp_resctrl = NULL;
>>> +
>>> ssize_t i = 0;
>>> int n;
>>> int ret = -1;
>>> @@ -19349,15 +19352,24 @@
>> virDomainCachetuneDefParse(virDomainDefPtr def,
>>> goto cleanup;
>>> }
>>>
>>> - if (virDomainResctrlAppend(def, node, alloc, vcpus, flags) < 0)
>>> + if (VIR_ALLOC(tmp_resctrl) < 0)
>>> goto cleanup;
>>> - vcpus = NULL;
>>> +
>>> + tmp_resctrl->vcpus = vcpus;
>>> + tmp_resctrl->alloc = virObjectRef(alloc);
>>
>> [1] Seems the called function also takes a Ref?
>>
>
> Yes. virDomainResctrlAppend takes a Ref, and the caller also take another Ref,
> it is only necessary to take a Ref at the caller level, right? A Ref active to
> an object is ensuring the object memory will not be released, right?
> Anyway, the local 'alloc' will be removed, and the Ref is removed too, but
> the caller's Ref/unRef will be kept.
>
When you place some object into a second structure and that structure is
successfully placed into a list that would be Free'd at a different
point in time of it's initial "parent", then increase the refcnt. Each
Free routine then would have the Unref of the object indicating it's
done using it.
John
>>> +
>>> + if (virDomainResctrlAppend(def, node, tmp_resctrl, flags) < 0)
>>> + goto cleanup;
>>> +
>>> alloc = NULL;
>>> + vcpus = NULL;
>>> + tmp_resctrl = NULL;
>>
>> This sequence is quite familiar with [2] ...
>>
>>>
>>> ret = 0;
>>> cleanup:
>>> ctxt->node = oldnode;
>>> virObjectUnref(alloc);
>>> + VIR_FREE(tmp_resctrl);
>>> virBitmapFree(vcpus);
>>> VIR_FREE(nodes);
>>> return ret;
>>> @@ -19514,6 +19526,7 @@
>> virDomainMemorytuneDefParse(virDomainDefPtr def,
>>> xmlNodePtr *nodes = NULL;
>>> virBitmapPtr vcpus = NULL;
>>> virResctrlAllocPtr alloc = NULL;
>>> + virDomainResctrlDefPtr tmp_resctrl = NULL;
>>> ssize_t i = 0;
>>> int n;
>>> int ret = -1;
>>> @@ -19560,17 +19573,24 @@
>> virDomainMemorytuneDefParse(virDomainDefPtr def,
>>> * just update the existing alloc information, which is done in above
>>> * virDomainMemorytuneDefParseMemory */
>>> if (new_alloc) {
>>> - if (virDomainResctrlAppend(def, node, alloc, vcpus, flags) < 0)
>>> + if (VIR_ALLOC(tmp_resctrl) < 0)
>>> + goto cleanup;
>>> +
>>> + tmp_resctrl->alloc = virObjectRef(alloc);
>>
>> [1] Seems the called function also takes a Ref?
>>
>>> + tmp_resctrl->vcpus = vcpus;
>>> + if (virDomainResctrlAppend(def, node, tmp_resctrl, flags) <
>>> + 0)
>>> goto cleanup;
>>> vcpus = NULL;
>>> alloc = NULL;
>>> + tmp_resctrl = NULL;
>>
>> [2] ... this sequence
>>
>> It seems to me you could create helper :
>>
>> virDomainResctrlCreate(def, node, alloc, vcpus, flags)
>>
>> which could :
>>
>> if (VIR_ALLOC(resctrl) < 0)
>> return -1;
>>
>> resctrl->alloc = alloc;
>> resctrl->vcpus = vcpus;
>> if (virDomainResctrlAppend(def, node, resctrl, flags) < 0) {
>> VIR_FREE(resctrl);
>> return -1;
>> }
>>
>> virObjectRef(alloc);
>> return 0;
>>
>> with the current callers just changing from Append to Create keeping their alloc
>> = NULL and vcpus = NULL on success.
>>
>
> Agree. Thanks for the sample code. In later patch, some code for paring the
> configuration of monitor is added, I also will add this part of code into this
> new helper.
> Will create virDomainResctrlCreate to remove the code duplication.
>
> Thanks for review.
> Huaqiang
>
>> John
>>
>>> }
>>>
>>> ret = 0;
>>> cleanup:
>>> ctxt->node = oldnode;
>>> - virObjectUnref(alloc);
>>> virBitmapFree(vcpus);
>>> + virObjectUnref(alloc);
>>> + VIR_FREE(tmp_resctrl);
>>> VIR_FREE(nodes);
>>> return ret;
>>> }
>>>
More information about the libvir-list
mailing list