Refactor virDomainResctrlAppend to facilitate virDomainResctrlDef
with the capability to hold more element.
Signed-off-by: Wang Huaqiang <huaqiang.wang@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) {
+ 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;
-
/* 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);
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;
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);
--
2.7.4
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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@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))) {
> + 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.
> 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.
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?
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
> 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@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
> -----Original Message-----
> From: John Ferlan [mailto:jferlan@redhat.com]
> Sent: Thursday, October 11, 2018 3:54 AM
> To: Wang, Huaqiang <huaqiang.wang@intel.com>; libvir-list@redhat.com
> Cc: Feng, Shaohe <shaohe.feng@intel.com>; Niu, Bing <bing.niu@intel.com>;
> Ding, Jian-feng <jian-feng.ding@intel.com>; Zang, Rui <rui.zang@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@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);
> >
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2025 Red Hat, Inc.