2:17 a.m.
-----Original Message-----
From: John Ferlan [mailto:jferlan@redhat.com]
Sent: Thursday, October 11, 2018 3:54 AM
To: Wang, Huaqiang <huaqiang.wang(a)intel.com>; libvir-list(a)redhat.com
Cc: Feng, Shaohe <shaohe.feng(a)intel.com>; Niu, Bing <bing.niu(a)intel.com>;
Ding, Jian-feng <jian-feng.ding(a)intel.com>; Zang, Rui <rui.zang(a)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(a)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);
>