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

[Erik Skultety <eskultet@xxxxxxxxxx>]

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@xxxxxxxxx>
> Reviewed-by: Erik Skultety <eskultet@xxxxxxxxxx>
> ---
...

> @@ -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

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list