> -----Original Message----- > From: John Ferlan [mailto:jferlan@xxxxxxxxxx] > Sent: Thursday, October 11, 2018 3:54 AM > 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: [PATCHv5 12/19] conf: Refactor virDomainResctrlAppend > > > This is more "Introduce virDomainResctrlNew" > > On 10/9/18 6:30 AM, Wang Huaqiang wrote: > > Refactor virDomainResctrlAppend to facilitate virDomainResctrlDef with > > the capability to hold more element. > > and then this says something like: > > "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@xxxxxxxxx> > > --- > > src/conf/domain_conf.c | 64 > > +++++++++++++++++++++++++++++++++++--------------- > > 1 file changed, 45 insertions(+), 19 deletions(-) > > > > diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index > > e2b4701..9a514a6 100644 > > --- a/src/conf/domain_conf.c > > +++ b/src/conf/domain_conf.c > > @@ -18920,24 +18920,43 @@ > > virDomainCachetuneDefParseCache(xmlXPathContextPtr ctxt, } > > > > > > +static virDomainResctrlDefPtr > > +virDomainResctrlNew(virResctrlAllocPtr alloc, > > + virBitmapPtr vcpus) { > > + virDomainResctrlDefPtr resctrl = NULL; > > + > > + if (VIR_ALLOC(resctrl) < 0) > > + return NULL; > > + > > + if ((resctrl->vcpus = virBitmapNewCopy(vcpus)) == NULL) { > > I'd prefer: > > if (!(resctrl->vcpus = virBitmapNewCopy(vcpus))) { OK. Seems more consistent with the rest of the code. > > > + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", > > + _("failed to copy 'vcpus'")); > > + goto error; > > + } > > + > > + resctrl->alloc = virObjectRef(alloc); > > + > > + return resctrl; > > + error: > > + virDomainResctrlDefFree(resctrl); > > + return NULL; > > +} > > + > > + > > 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; > > int ret = -1; > > > > - if (VIR_ALLOC(tmp_resctrl) < 0) > > - goto cleanup; > > - > > Based on below, I think this is where you call the virDomainResctrlNew w/ > @cpus and @alloc > > > /* 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(resctrl->vcpus); > > if (!vcpus_str) > > goto cleanup; > > > > @@ -18954,18 +18973,14 @@ virDomainResctrlAppend(virDomainDefPtr def, > > goto cleanup; > > } > > > > - if (virResctrlAllocSetID(alloc, alloc_id) < 0) > > + if (virResctrlAllocSetID(resctrl->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); > > You'd keep the above by use @resctrl for a parameter. On success, > VIR_APPEND_ELEMENT will set resctrl = NULL so the *DefFree won't do > anything. Without that, then the @resctrl would be leaked if the APPEND failed > for any reason. After code refactoring, the this function's argument @resctrl is passed in from its caller, so the caller allocates object memory for @resctrl, and it is better let caller to do the object release job when this function returns an error. @resctrl object is allocated and freed for error in function virDomainCachetuneDefParse and virDomainMemorytuneDefParse. I think this code will not make the memory leak. > > > VIR_FREE(alloc_id); > > VIR_FREE(vcpus_str); > > return ret; > > @@ -18982,6 +18997,7 @@ virDomainCachetuneDefParse(virDomainDefPtr > def, > > xmlNodePtr *nodes = NULL; > > virBitmapPtr vcpus = NULL; > > virResctrlAllocPtr alloc = NULL; > > + virDomainResctrlDefPtr resctrl = NULL; > > ssize_t i = 0; > > int n; > > int ret = -1; > > @@ -19030,15 +19046,18 @@ > virDomainCachetuneDefParse(virDomainDefPtr def, > > goto cleanup; > > } > > > > - if (virDomainResctrlAppend(def, node, alloc, vcpus, flags) < 0) > > + resctrl = virDomainResctrlNew(alloc, vcpus); > > + if (!resctrl) > > goto cleanup; > > > > - vcpus = NULL; > > - alloc = NULL; > > + if (virDomainResctrlAppend(def, node, resctrl, flags) < 0) > > + goto cleanup; > > > > + resctrl = NULL; > > Of course this is where it gets tricky - although you could pass &resctrl and then > use (*resctrl)-> in the function - that way upon successful return resctrl is either > added or free'd already. If passing a pointer of @resctrl to virDomainResctrlAppend, it will work as you said. I don't think the current implementation of this refactoring will cause the memory leak as you pointed out in above lines, since you prefer the way by passing in a &resctrl to virDomainResctrlAppend, I can change code accordingly. > > Alternatively, since both areas are changing to first alloc and then append, is > there any specific reason the virDomainResctrlNew has to be outside of > virDomainResctrlAppend? The @resctrl structure, which is virDomainResctrlDefPtr type, will be extended later in a new form of: struct _virDomainResctrlDef { char *id; virBitmapPtr vcpus; virResctrlAllocPtr alloc; virDomainResctrlMonDefPtr *monitors; size_t nmonitors; }; If without virDomainResctrlNew, the virDomainResctrlAppend function will have a long argument list finally, like this: static int virDomainResctrlAppend(virDomainDefPtr def, xmlNodePtr node, virResctrlAllocPtr alloc, virBitmapPtr vcpus, virDomainResctrlMonDefPtr monitors, size_t nmonitors, unsigned int flags) To make the function argument list be more concise, so we decide to create the virDomainResctrlNew function and create the @resctrl out of virDomainResctrlAppend in v1. > > I do see the future does some other virDomainResctrlMonDefParse and > virResctrlAllocIsEmpty calls before virDomainResctrlAppend - may have to > rethink all that or just go with the &resctrl logic. Maybe I'll have a different > thought later - let's see what happens when I get there. > > John Thanks for review. Huaqiang > > > > ret = 0; > > cleanup: > > ctxt->node = oldnode; > > + virDomainResctrlDefFree(resctrl); > > virObjectUnref(alloc); > > virBitmapFree(vcpus); > > VIR_FREE(nodes); > > @@ -19196,6 +19215,8 @@ > virDomainMemorytuneDefParse(virDomainDefPtr def, > > xmlNodePtr *nodes = NULL; > > virBitmapPtr vcpus = NULL; > > virResctrlAllocPtr alloc = NULL; > > + virDomainResctrlDefPtr resctrl = NULL; > > + > > ssize_t i = 0; > > int n; > > int ret = -1; > > @@ -19240,15 +19261,20 @@ > 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) > > + resctrl = virDomainResctrlNew(alloc, vcpus); > > + if (!resctrl) > > goto cleanup; > > - vcpus = NULL; > > - alloc = NULL; > > + > > + if (virDomainResctrlAppend(def, node, resctrl, flags) < 0) > > + goto cleanup; > > + > > + resctrl = NULL; > > } > > > > ret = 0; > > cleanup: > > ctxt->node = oldnode; > > + virDomainResctrlDefFree(resctrl); > > virObjectUnref(alloc); > > virBitmapFree(vcpus); > > VIR_FREE(nodes); > > -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
