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

2018-07-27 Thread Erik Skultety 
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 
> Reviewed-by: Erik Skultety 
> ---
...

> @@ -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, ) < 0)
-goto cleanup;
+return -1;

-if (virCgroupNew(-1, newPath, NULL, controllers, group) < 0)
-goto cleanup;
+if (virCgroupNew(-1, newPath, NULL, controllers, ) < 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, ) < 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@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

2018-07-26 Thread Sukrit Bhatnagar 
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 
Reviewed-by: Erik Skultety 
---
 src/util/vircgroup.c | 185 +++
 1 file changed, 67 insertions(+), 118 deletions(-)

diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c
index 6f7b5b4..61fafe2 100644
--- a/src/util/vircgroup.c
+++ b/src/util/vircgroup.c
@@ -836,25 +836,21 @@ virCgroupGetValueForBlkDev(virCgroupPtr group,
 {
 VIR_AUTOFREE(char *) prefix = NULL;
 VIR_AUTOFREE(char *) str = NULL;
-char **lines = NULL;
-int ret = -1;
+VIR_AUTOPTR(virString) lines = NULL;
 
 if (virCgroupGetValueStr(group, controller, key, ) < 0)
-goto error;
+return -1;
 
 if (!(prefix = virCgroupGetBlockDevString(path)))
-goto error;
+return -1;
 
 if (!(lines = virStringSplit(str, "\n", -1)))
-goto error;
+return -1;
 
 if (VIR_STRDUP(*value, virStringListGetFirstWithPrefix(lines, prefix)) < 0)
-goto error;
+return -1;
 
-ret = 0;
- error:
-virStringListFree(lines);
-return ret;
+return 0;
 }
 
 
@@ -1217,12 +1213,11 @@ virCgroupAddTaskController(virCgroupPtr group, pid_t 
pid, int controller)
 static int
 virCgroupSetPartitionSuffix(const char *path, char **res)
 {
-char **tokens;
+VIR_AUTOPTR(virString) tokens = NULL;
 size_t i;
-int ret = -1;
 
 if (!(tokens = virStringSplit(path, "/", 0)))
-return ret;
+return -1;
 
 for (i = 0; tokens[i] != NULL; i++) {
 /* Whitelist the 3 top level fixed dirs
@@ -1241,22 +1236,18 @@ virCgroupSetPartitionSuffix(const char *path, char 
**res)
 !strchr(tokens[i], '.')) {
 if (VIR_REALLOC_N(tokens[i],
   strlen(tokens[i]) + strlen(".partition") + 1) < 
0)
-goto cleanup;
+return -1;
 strcat(tokens[i], ".partition");
 }
 
 if (virCgroupPartitionEscape(&(tokens[i])) < 0)
-goto cleanup;
+return -1;
 }
 
 if (!(*res = virStringListJoin((const char **)tokens, "/")))
-goto cleanup;
+return -1;
 
-ret = 0;
-
- cleanup:
-virStringListFree(tokens);
-return ret;
+return 0;
 }
 
 
@@ -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;
 }
 
 
@@ -1502,9 +1492,9 @@ virCgroupNewMachineSystemd(const char *name,
int controllers,
virCgroupPtr *group)
 {
-int ret = -1;
 int rv;
-virCgroupPtr init, parent = NULL;
+VIR_AUTOPTR(virCgroup) init = NULL;
+VIR_AUTOPTR(virCgroup) parent = NULL;
 VIR_AUTOFREE(char *) path = NULL;
 char *offset;
 
@@ -1531,12 +1521,10 @@ virCgroupNewMachineSystemd(const char *name,
 
 path = init->controllers[VIR_CGROUP_CONTROLLER_SYSTEMD].placement;
 init->controllers[VIR_CGROUP_CONTROLLER_SYSTEMD].placement = NULL;
-virCgroupFree(init);
 
 if (!path || STREQ(path, "/") || path[0] != '/') {
 VIR_DEBUG("Systemd didn't setup its controller");
-ret = -2;
-goto cleanup;
+return -2;
 }
 
 offset = path;
@@ -1546,7 +1534,7 @@ virCgroupNewMachineSystemd(const char *name,
  NULL,
  controllers,
  ) < 0)
-goto cleanup;
+return -1;
 
 
 for (;;) {
@@ -1560,11 +1548,11 @@ virCgroupNewMachineSystemd(const char *name,
  parent,
  controllers,
  ) < 0)
-goto cleanup;
+return -1;
 
 if (virCgroupMakeGroup(parent, tmp, true, VIR_CGROUP_NONE) < 0) {
 virCgroupFree(tmp);
-goto cleanup;
+return -1;
 }
 if (t) {
 *t = '/';
@@ -1587,10 +1575,7 @@ virCgroupNewMachineSystemd(const char *name,
 }
 }
 
-ret = 0;
- cleanup:
-virCgroupFree(parent);
-return ret;
+return 0;
 }
 
 
@@ -1611,8 +1596,7 @@ virCgroupNewMachineManual(const char *name,