Re: [libvirt] [PATCH 07/10] conf: refactor virDomainResctrlAppend

-----Original Message----- From: John Ferlan [mailto:jferlan@redhat.com] Sent: Wednesday, September 5, 2018 11:49 PM To: Wang, Huaqiang <huaqiang.wang(a)intel.com>; libvir-list(a)redhat.com Cc: Feng, Shaohe <shaohe.feng(a)intel.com>; Niu, Bing <bing.niu(a)intel.com>; Ding, Jian-feng <jian-feng.ding(a)intel.com>; Zang, Rui <rui.zang(a)intel.com&gt; 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(a)intel.com&gt; > --- > 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}

> + > 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.

> + > + 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

> + 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?

> + > + 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.

John > } > > ret = 0; > cleanup: > ctxt->node = oldnode; > - virObjectUnref(alloc); > virBitmapFree(vcpus); > + virObjectUnref(alloc); > + VIR_FREE(tmp_resctrl); > VIR_FREE(nodes); > return ret; > } >