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

-----Original Message----- From: John Ferlan [mailto:jferlan@redhat.com] Sent: Wednesday, November 7, 2018 12:15 AM To: Wang, Huaqiang <huaqiang.wang(a)intel.com>; libvir-list(a)redhat.com Cc: Feng, Shaohe <shaohe.feng(a)intel.com>; bing.niu(a)intel.com; Ding, Jian- feng <jian-feng.ding(a)intel.com>; Zang, Rui <rui.zang(a)intel.com&gt; Subject: Re: [PATCHv7 10/18] conf: Remove virDomainResctrlAppend and introduce virDomainResctrlNew On 11/6/18 4:51 AM, Huaqiang,Wang wrote: > > > On 2018年11月06日 01:26, John Ferlan wrote: >> >> 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. > > Agree. Please help make change. > > >> >>> +                    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. > > Yes, it might happen. > If @alloc->id is specified through XML and is not following the naming > convention >          virAsprintf(&alloc_id, "vcpus_%s", vcpus_str) > > If you think it is necessary we might need to through a warning for > this case. > Let's see - virDomainResctrlNew has two callers: 1. virDomainCachetuneDefParse In this case, we "know" we have a new/empty @alloc because if virDomainResctrlVcpuMatch found one, then there'd be a failure. The virDomainCachetuneDefParseCache calls don't seem to fill in alloc->id, but virDomainResctrlNew will for the first time. 2. virDomainMemorytuneDefParse The virDomainResctrlVcpuMatch may find a preexisting @alloc, but @new_alloc is set to true. The virDomainMemorytuneDefParseMemory won't fill alloc->id. Then only if @new_alloc do we call virDomainResctrlNew So I think both are safe "for now".

If you want I could modify the virResctrlAllocSetID code to : if (alloc->id) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Attempt to overwrite alloc->id='%s' with id='%s'"), alloc->id, id); return -1; } Let me know.

John

>> >> With the changes (that I can make), >> >> Reviewed-by: John Ferlan <jferlan(a)redhat.com&gt; >> >> John > > Thanks for review. > Huaqiang > >> >> [...] >