[libvirt] [PATCHv5 12/19] conf: Refactor virDomainResctrlAppend
Wang, Huaqiang
huaqiang.wang at intel.com
Fri Oct 12 07:17:13 UTC 2018
> -----Original Message-----
> From: John Ferlan [mailto:jferlan at redhat.com]
> Sent: Thursday, October 11, 2018 3:54 AM
> 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] [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 at intel.com>
> > ---
> > 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);
> >
More information about the libvir-list
mailing list