Re: [libvirt] [PATCH v2 10/41] util: cgroup: use VIR_AUTOPTR for aggregate types

27 Jul 2018

On Tue, Jul 24, 2018 at 09:22:11PM +0530, Sukrit Bhatnagar wrote:
...
By making use of GNU C's cleanup attribute handled by the
VIR_AUTOPTR macro for declaring aggregate pointer variables,
majority of the calls to *Free functions can be dropped, which
in turn leads to getting rid of most of our cleanup sections.
Signed-off-by: Sukrit Bhatnagar <skrtbhtngr@gmail.com>
Reviewed-by: Erik Skultety <eskultet@redhat.com>
---
...
...
@@ -1277,10 +1268,10 @@ virCgroupNewPartition(const char *path,
                       int controllers,
                       virCgroupPtr *group)
 {
-    int ret = -1;
     VIR_AUTOFREE(char *) parentPath = NULL;
     VIR_AUTOFREE(char *) newPath = NULL;
-    virCgroupPtr parent = NULL;
+    VIR_AUTOPTR(virCgroup) parent = NULL;
+    VIR_AUTOPTR(virCgroup) tmpGroup = NULL;
     VIR_DEBUG("path=%s create=%d controllers=%x",
               path, create, controllers);
@@ -1315,12 +1306,11 @@ virCgroupNewPartition(const char *path,
         }
     }
-    ret = 0;
+    return 0;
+
  cleanup:
-    if (ret != 0)
-        virCgroupFree(*group);
-    virCgroupFree(parent);
-    return ret;
+    VIR_STEAL_PTR(tmpGroup, *group);
+    return -1;
^Not exactly what I had in mind, we're still touching the caller provided data
too much. Have a look at this diff:

diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c
index 61fafe26f8..472a8167f5 100644
--- a/src/util/vircgroup.c
+++ b/src/util/vircgroup.c
@@ -1283,34 +1283,31 @@ virCgroupNewPartition(const char *path,
     }

     if (virCgroupSetPartitionSuffix(path, &newPath) < 0)
-        goto cleanup;
+        return -1;

-    if (virCgroupNew(-1, newPath, NULL, controllers, group) < 0)
-        goto cleanup;
+    if (virCgroupNew(-1, newPath, NULL, controllers, &tmpGroup) < 0)
+        return -1;

     if (STRNEQ(newPath, "/")) {
         char *tmp;
         if (VIR_STRDUP(parentPath, newPath) < 0)
-            goto cleanup;
+            return -1;

         tmp = strrchr(parentPath, '/');
         tmp++;
         *tmp = '\0';

         if (virCgroupNew(-1, parentPath, NULL, controllers, &parent) < 0)
-            goto cleanup;
+            return -1;

-        if (virCgroupMakeGroup(parent, *group, create, VIR_CGROUP_NONE) < 0) {
-            virCgroupRemove(*group);
-            goto cleanup;
+        if (virCgroupMakeGroup(parent, tmpGroup, create, VIR_CGROUP_NONE) < 0) {
+            virCgroupRemove(tmpGroup);
+            return -1;
         }
     }

+    VIR_STEAL_PTR(*group, tmpGroup);
     return 0;
-
- cleanup:
-    VIR_STEAL_PTR(tmpGroup, *group);
-    return -1;

After ^this, we'll only touch the caller provided data once we're sure nothing
can go wrong anymore. I'll squash that in.

Erik

    