On 09/10/2018 02:13 PM, Wang, Huaqiang wrote: > > >> -----Original Message----- >> From: John Ferlan [mailto:jferlan@xxxxxxxxxx] >> Sent: Wednesday, September 5, 2018 11:49 PM >> To: Wang, Huaqiang <huaqiang.wang@xxxxxxxxx>; libvir-list@xxxxxxxxxx >> Cc: Feng, Shaohe <shaohe.feng@xxxxxxxxx>; Niu, Bing <bing.niu@xxxxxxxxx>; >> Ding, Jian-feng <jian-feng.ding@xxxxxxxxx>; Zang, Rui <rui.zang@xxxxxxxxx> >> Subject: Re: [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@xxxxxxxxx> >>> --- >>> 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; >>> } >>> -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
