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

12 Oct 2018

...
-----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);


    