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

[Wang, Huaqiang]

> -----Original Message-----
> From: John Ferlan [mailto:jferlan at redhat.com]
> Sent: Wednesday, November 7, 2018 12:15 AM
> To: Wang, Huaqiang <huaqiang.wang at intel.com>; libvir-list at redhat.com
> Cc: Feng, Shaohe <shaohe.feng at intel.com>; bing.niu at intel.com; Ding, Jian-
> feng <jian-feng.ding at intel.com>; Zang, Rui <rui.zang at intel.com>
> 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 at intel.com>
> >>> ---
> >>>   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".

Yes. Agree. Thanks for the analysis.

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

virResctrlMonitorSetID should also do similar patch, right?
Then the body of two functions, virRresctrlAllocSetID and virResctrlMonitorSetID,
are very similar. 

I will introduce two patches, the first patch will refactor virRresctrlAllocSetID
and the second patch will reuse the refactor for virResctrlMonitorSetID.

I know you have a solution solving this, my code is just for your reference.

> John
> 
> 

Thanks for review.
Huaqiang

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