Re: [PATCHv5 12/19] conf: Refactor virDomainResctrlAppend

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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



[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]

  Powered by Linux