[libvirt] [PATCH v1 08/18] use VIR_AUTOFREE in src/util/vircgroup.c

[Erik Skultety]

On Sun, Jun 03, 2018 at 01:42:06PM +0530, Sukrit Bhatnagar wrote:
> Modify code to use VIR_AUTOFREE macro wherever required.
>
> Signed-off-by: Sukrit Bhatnagar <skrtbhtngr at gmail.com>
> ---
>  src/util/vircgroup.c | 526 ++++++++++++++++++---------------------------------
>  1 file changed, 179 insertions(+), 347 deletions(-)
>
...

> @@ -378,7 +369,7 @@ virCgroupDetectMountsFromFile(virCgroupPtr group,
>      FILE *mounts = NULL;
>      struct mntent entry;
>      char buf[CGROUP_MAX_VAL];
> -    char *linksrc = NULL;
> +    VIR_AUTOFREE(char *) linksrc = NULL;

@linksrc is being used only in an 'if' block further in the function body
(the context of which is not seen in the patch), so I'd move the declaration to
that block which will allow us to ditch the VIR_FREE(linksrc) in the same
block.

...

>
>
> @@ -1339,9 +1279,9 @@ virCgroupNewPartition(const char *path,
>                        virCgroupPtr *group)
>  {
>      int ret = -1;
> -    char *parentPath = NULL;
> +    VIR_AUTOFREE(char *) parentPath = NULL;
>      virCgroupPtr parent = NULL;
> -    char *newPath = NULL;
> +    VIR_AUTOFREE(char *) newPath = NULL;

move ^this one line up, so the AUTOFREEs are nicely packed...

>      VIR_DEBUG("path=%s create=%d controllers=%x",
>                path, create, controllers);
>
> @@ -1381,8 +1321,6 @@ virCgroupNewPartition(const char *path,
>      if (ret != 0)
>          virCgroupFree(group);
>      virCgroupFree(&parent);
> -    VIR_FREE(parentPath);
> -    VIR_FREE(newPath);
>      return ret;
>  }

...

> @@ -1577,7 +1506,7 @@ virCgroupNewMachineSystemd(const char *name,
>      int ret = -1;
>      int rv;
>      virCgroupPtr init, parent = NULL;
> -    char *path = NULL;
> +    VIR_AUTOFREE(char *) path = NULL;
>      char *offset;
>
>      VIR_DEBUG("Trying to setup machine '%s' via systemd", name);
> @@ -1662,7 +1591,6 @@ virCgroupNewMachineSystemd(const char *name,
>      ret = 0;
>   cleanup:
>      virCgroupFree(&parent);
> -    VIR_FREE(path);
>      return ret;
>  }
>
> @@ -1894,9 +1822,10 @@ virCgroupGetBlkioIoServiced(virCgroupPtr group,
>                              long long *requests_write)
>  {
>      long long stats_val;
> -    char *str1 = NULL, *str2 = NULL, *p1, *p2;
> +    VIR_AUTOFREE(char *) str1 = NULL;
> +    VIR_AUTOFREE(char *) str2 = NULL;
> +    char *p1, *p2;

since you're changing it already, p1 and p2 should be on separate lines...

>      size_t i;
> -    int ret = -1;
>
>      const char *value_names[] = {
>          "Read ",
> @@ -1919,12 +1848,12 @@ virCgroupGetBlkioIoServiced(virCgroupPtr group,
>      if (virCgroupGetValueStr(group,
>                               VIR_CGROUP_CONTROLLER_BLKIO,
>                               "blkio.throttle.io_service_bytes", &str1) < 0)
> -        goto cleanup;
> +        return -1;
>
>      if (virCgroupGetValueStr(group,
>                               VIR_CGROUP_CONTROLLER_BLKIO,
>                               "blkio.throttle.io_serviced", &str2) < 0)
> -        goto cleanup;
> +        return -1;
>
>      /* sum up all entries of the same kind, from all devices */
>      for (i = 0; i < ARRAY_CARDINALITY(value_names); i++) {
> @@ -1938,7 +1867,7 @@ virCgroupGetBlkioIoServiced(virCgroupPtr group,
>                                 _("Cannot parse byte %sstat '%s'"),
>                                 value_names[i],
>                                 p1);
> -                goto cleanup;
> +                return -1;
>              }
>
>              if (stats_val < 0 ||
> @@ -1947,7 +1876,7 @@ virCgroupGetBlkioIoServiced(virCgroupPtr group,
>                  virReportError(VIR_ERR_OVERFLOW,
>                                 _("Sum of byte %sstat overflows"),
>                                 value_names[i]);
> -                goto cleanup;
> +                return -1;
>              }
>              *bytes_ptrs[i] += stats_val;
>          }
> @@ -1959,7 +1888,7 @@ virCgroupGetBlkioIoServiced(virCgroupPtr group,
>                                 _("Cannot parse %srequest stat '%s'"),
>                                 value_names[i],
>                                 p2);
> -                goto cleanup;
> +                return -1;
>              }
>
>              if (stats_val < 0 ||
> @@ -1968,18 +1897,13 @@ virCgroupGetBlkioIoServiced(virCgroupPtr group,
>                  virReportError(VIR_ERR_OVERFLOW,
>                                 _("Sum of %srequest stat overflows"),
>                                 value_names[i]);
> -                goto cleanup;
> +                return -1;
>              }
>              *requests_ptrs[i] += stats_val;
>          }
>      }
>
> -    ret = 0;
> -
> - cleanup:
> -    VIR_FREE(str2);
> -    VIR_FREE(str1);
> -    return ret;
> +    return 0;
>  }
>
>
> @@ -2003,9 +1927,11 @@ virCgroupGetBlkioIoDeviceServiced(virCgroupPtr group,
>                                    long long *requests_read,
>                                    long long *requests_write)
>  {
> -    char *str1 = NULL, *str2 = NULL, *str3 = NULL, *p1, *p2;
> +    VIR_AUTOFREE(char *) str1 = NULL;
> +    VIR_AUTOFREE(char *) str2 = NULL;
> +    VIR_AUTOFREE(char *) str3 = NULL;
> +    char *p1, *p2;

here too..separate lines...

...

>
>
> @@ -3131,10 +2992,10 @@ virCgroupGetPercpuVcpuSum(virCgroupPtr group,
>  {
>      int ret = -1;
>      ssize_t i = -1;
> -    char *buf = NULL;
>      virCgroupPtr group_vcpu = NULL;
>
>      while ((i = virBitmapNextSetBit(guestvcpus, i)) >= 0) {
> +        VIR_AUTOFREE(char *) buf = NULL;
>          char *pos;

more cleanup possible here, since there's no point in having @pos at all, on
the contrary, it's a bit confusing what it's being used for...so @pos should be
dropped in a separate patch and @buf should be used directly, beware though:

virStrToLong_ull(buf, &buf, 10, &tmp) would leak memory, so
virStrToLong_ull(buf, NULL, 10, &tmp) should be used instead...

...

>
>  int
>  virCgroupHasEmptyTasks(virCgroupPtr cgroup, int controller)
>  {
> -    int ret = -1;

^This is okay, so don't change it...

> -    char *content = NULL;
> +    int ret;
> +    VIR_AUTOFREE(char *) content = NULL;
>
>      if (!cgroup)
>          return -1;
> @@ -4104,7 +3937,6 @@ virCgroupHasEmptyTasks(virCgroupPtr cgroup, int controller)
>      if (ret == 0 && content[0] == '\0')
>          ret = 1;
>
> -    VIR_FREE(content);
>      return ret;
>  }

Erik