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

[Wang, Huaqiang]

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

Local varaibles 'alloc', 'vcpus' will be removed.

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

This is a static function, the caller will ensure its safety.
How about removing these two lines?

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

Will remove this local variable along with this Ref. But the caller's
Ref/unRef remained.
Thanks.

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

Yes. virDomainResctrlAppend takes a Ref, and the caller also take another Ref,
it is only necessary to take a Ref at the caller level, right? A Ref active to
an object is ensuring the object memory will not be released, right?
Anyway, the local 'alloc' will be removed, and the Ref is removed too, but
the caller's Ref/unRef will be kept.

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

Agree. Thanks for the sample code. In later patch, some code for paring the
configuration of monitor is added, I also will add this part of code into this
new helper.
Will create virDomainResctrlCreate to remove the code duplication.

Thanks for review.
Huaqiang

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