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

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(a)intel.com&gt; > --- > 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.

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

With the changes (that I can make), Reviewed-by: John Ferlan <jferlan(a)redhat.com&gt; John

[...]