[PATCH] qemu_cgroup.c: use VIR_AUTOSTRINGLIST, g_autofree and g_autoptr

[Michal Prívozník]

On 31. 3. 2020 17:44, Seeteena Thoufeek wrote:
> Signed-off-by: Seeteena Thoufeek <s1seetee at linux.vnet.ibm.com>
> ---
>  src/qemu/qemu_cgroup.c | 91 +++++++++++++++++++-------------------------------
>  1 file changed, 35 insertions(+), 56 deletions(-)
> 
> diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c
> index c0e30f6..d34c515 100644
> --- a/src/qemu/qemu_cgroup.c
> +++ b/src/qemu/qemu_cgroup.c
> @@ -62,7 +62,7 @@ qemuSetupImagePathCgroup(virDomainObjPtr vm,
>  {
>      qemuDomainObjPrivatePtr priv = vm->privateData;
>      int perms = VIR_CGROUP_DEVICE_READ;
> -    char **targetPaths = NULL;
> +    VIR_AUTOSTRINGLIST targetPaths = NULL;
>      size_t i;
>      int rv;
>      int ret = -1;> @@ -82,12 +82,11 @@ qemuSetupImagePathCgroup(virDomainObjPtr vm,
>                               virCgroupGetDevicePermsString(perms),
>                               rv);
>      if (rv < 0)
> -        goto cleanup;
> +        return ret;

I don't quite understand. Why not return -1 immediatelly? This also
applies to the test of the patch.

>  
>      if (rv > 0) {
>          /* @path is neither character device nor block device. */
> -        ret = 0;
> -        goto cleanup;
> +        return 0;
>      }
>  
>      if (virDevMapperGetTargets(path, &targetPaths) < 0 &&


> @@ -964,16 +956,9 @@ qemuInitCgroup(virDomainObjPtr vm,
>                              cfg->maxThreadsPerProc,
>                              &priv->cgroup) < 0) {
>          if (virCgroupNewIgnoreError())
> -            goto done;
> -
> -        goto cleanup;
> +            return 0;

So previouslly, it this has failed, then if the error was to be ingored
then 0 was returned and -1 if it wasn't. But with this change 0 is
returned no matter what.

>      }
> -
> - done:
> -    ret = 0;
> - cleanup:
> -    virObjectUnref(cfg);
> -    return ret;
> +  return ret;

I don't think this is right. Previously, in success path ret was set to
0. But you removed that, so this returns -1, allways.

>  }
>  
>  static void
> @@ -1058,14 +1043,14 @@ int
>  qemuConnectCgroup(virDomainObjPtr vm)
>  {
>      qemuDomainObjPrivatePtr priv = vm->privateData;
> -    virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(priv->driver);
> +    g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(priv->driver);
>      int ret = -1;
>  
>      if (!virQEMUDriverIsPrivileged(priv->driver))
> -        goto done;
> +        return 0;
>  
>      if (!virCgroupAvailable())
> -        goto done;
> +        return 0;
>  
>      virCgroupFree(&priv->cgroup);
>  
> @@ -1075,14 +1060,9 @@ qemuConnectCgroup(virDomainObjPtr vm)
>                                    cfg->cgroupControllers,
>                                    priv->machineName,
>                                    &priv->cgroup) < 0)
> -        goto cleanup;
> +        return ret;
>  
>      qemuRestoreCgroupState(vm);
> -
> - done:
> -    ret = 0;
> - cleanup:
> -    virObjectUnref(cfg);
>      return ret;
>  }
>  
> @@ -1269,7 +1249,7 @@ qemuCgroupEmulatorAllNodesAllow(virCgroupPtr cgroup,
>                                  qemuCgroupEmulatorAllNodesDataPtr *retData)
>  {
>      qemuCgroupEmulatorAllNodesDataPtr data = NULL;
> -    char *all_nodes_str = NULL;
> +    g_autofree char *all_nodes_str = NULL;
>      virBitmapPtr all_nodes = NULL;

virBitmap can be made g_autoptr() too.

>      int ret = -1;
>  
> @@ -1298,7 +1278,6 @@ qemuCgroupEmulatorAllNodesAllow(virCgroupPtr cgroup,
>      ret = 0;
>  
>   cleanup:
> -    VIR_FREE(all_nodes_str);
>      virBitmapFree(all_nodes);
>      qemuCgroupEmulatorAllNodesDataFree(data);
>  
> 

Nevertheless, I'm fixing all the issues, ACKing and pushing.

Michal