[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH v1 06/32] util: netdev: use VIR_AUTOFREE instead of VIR_FREE for scalar types



On Sat, Jul 28, 2018 at 11:31:21PM +0530, Sukrit Bhatnagar wrote:
> By making use of GNU C's cleanup attribute handled by the
> VIR_AUTOFREE macro for declaring scalar variables, majority
> of the VIR_FREE calls can be dropped, which in turn leads to
> getting rid of most of our cleanup sections.
>
> Signed-off-by: Sukrit Bhatnagar <skrtbhtngr gmail com>
> ---
...

>
> @@ -1282,10 +1260,11 @@ virNetDevGetVirtualFunctions(const char *pfname,
>  {
>      int ret = -1;
>      size_t i;
> -    char *pf_sysfs_device_link = NULL;
> -    char *pci_sysfs_device_link = NULL;
> -    char *pciConfigAddr = NULL;
> -    char *pfPhysPortID = NULL;
> +    VIR_AUTOFREE(char *) pf_sysfs_device_link = NULL;
> +    VIR_AUTOFREE(char *) pci_sysfs_device_link = NULL;
> +    VIR_AUTOFREE(char *) pciConfigAddr = NULL;
> +    VIR_AUTOFREE(char *) pfPhysPortID = NULL;
> +    VIR_AUTOFREE(char **) tempVfname = NULL;

First of all, this function should return the number of VFs on success or -1 on
error rather than needing the caller to pass an argument by reference to fill
the number of VFs, but that is a refactor for another day and I don't expect
you to spend time on that. Anyhow, tempVFname should be used instead of @vfname
at all spots and only at the end of the function block call VIR_STEAL_PTR.

>
>      *virt_fns = NULL;
>      *n_vfname = 0;
> @@ -1333,13 +1312,9 @@ virNetDevGetVirtualFunctions(const char *pfname,
>
>   cleanup:
>      if (ret < 0) {
> -        VIR_FREE(*vfname);
> +        VIR_STEAL_PTR(tempVfname, *vfname);

^This is not the intended usage of VIR_STEAL_PTR, the intended usage is to
steal the local pointer *into* the caller-provided one once we know we're going
to return success, not vice-versa, so ^this "if (ret < 0)" block should be
eventually dropped - you'd need another VIR_AUTOPTR for the virt_fns.

>          VIR_FREE(*virt_fns);
>      }
> -    VIR_FREE(pfPhysPortID);
> -    VIR_FREE(pf_sysfs_device_link);
> -    VIR_FREE(pci_sysfs_device_link);
> -    VIR_FREE(pciConfigAddr);
>      return ret;
>  }
>
...

> @@ -1522,13 +1473,14 @@ int
>  virNetDevGetVirtualFunctionInfo(const char *vfname, char **pfname,
>                                  int *vf)
>  {
> -    char *pf_sysfs_path = NULL, *vf_sysfs_path = NULL;
> -    int ret = -1;
> +    VIR_AUTOFREE(char *) pf_sysfs_path = NULL;
> +    VIR_AUTOFREE(char *) vf_sysfs_path = NULL;
> +    VIR_AUTOFREE(char *) tempPfname = NULL;
>
>      *pfname = NULL;
>
>      if (virNetDevGetPhysicalFunction(vfname, pfname) < 0)
> -        return ret;
> +        return -1;
>
>      if (virNetDevSysfsFile(&pf_sysfs_path, *pfname, "device") < 0)
>          goto cleanup;
> @@ -1536,16 +1488,12 @@ virNetDevGetVirtualFunctionInfo(const char *vfname, char **pfname,
>      if (virNetDevSysfsFile(&vf_sysfs_path, vfname, "device") < 0)
>          goto cleanup;
>
> -    ret = virPCIGetVirtualFunctionIndex(pf_sysfs_path, vf_sysfs_path, vf);
> +    return virPCIGetVirtualFunctionIndex(pf_sysfs_path, vf_sysfs_path, vf);
>
>   cleanup:
> -    if (ret < 0)
> -        VIR_FREE(*pfname);
> +    VIR_STEAL_PTR(tempPfname, *pfname);

Same comment as above.

>
> -    VIR_FREE(vf_sysfs_path);
> -    VIR_FREE(pf_sysfs_path);
> -
> -    return ret;
> +    return -1;
>  }
...

>
>  #else /* !__linux__ */
>
>   cleanup:
>      nlmsg_free(nl_msg);

As I noted in previous responses, we can turn ^this into VIR_AUTOPTR too.

> -    VIR_FREE(resp);
>      return family_id;
>  }
>
> @@ -3265,13 +3184,13 @@ virNetDevSwitchdevFeature(const char *ifname,
>                            virBitmapPtr *out)
>  {
>      struct nl_msg *nl_msg = NULL;
> -    struct nlmsghdr *resp = NULL;
> +    VIR_AUTOFREE(struct nlmsghdr *) resp = NULL;
>      unsigned int recvbuflen;
>      struct nlattr *tb[DEVLINK_ATTR_MAX + 1] = {NULL, };
>      virPCIDevicePtr pci_device_ptr = NULL;
>      struct genlmsghdr* gmsgh = NULL;
>      const char *pci_name;
> -    char *pfname = NULL;
> +    VIR_AUTOFREE(char *) pfname = NULL;
>      int is_vf = -1;
>      int ret = -1;
>      uint32_t family_id;
> @@ -3333,8 +3252,6 @@ virNetDevSwitchdevFeature(const char *ifname,
>   cleanup:
>      nlmsg_free(nl_msg);
>      virPCIDeviceFree(pci_device_ptr);
> -    VIR_FREE(resp);
> -    VIR_FREE(pfname);
>      return ret;
>  }
>  # else
> @@ -3375,7 +3292,7 @@ virNetDevGetEthtoolGFeatures(virBitmapPtr bitmap,
>                               int fd,
>                               struct ifreq *ifr)
>  {
> -    struct ethtool_gfeatures *g_cmd;
> +    VIR_AUTOFREE(struct ethtool_gfeatures *) g_cmd = NULL;
>
>      if (VIR_ALLOC_VAR(g_cmd,
>                        struct ethtool_get_features_block, GFEATURES_SIZE) < 0)
> @@ -3385,7 +3302,6 @@ virNetDevGetEthtoolGFeatures(virBitmapPtr bitmap,
>      g_cmd->size = GFEATURES_SIZE;
>      if (virNetDevGFeatureAvailable(fd, ifr, g_cmd))
>          ignore_value(virBitmapSetBit(bitmap, VIR_NET_DEV_FEAT_TXUDPTNL));
> -    VIR_FREE(g_cmd);
>      return 0;
>  }
>  # else

Otherwise, I don't see any other problems, since this will need another
version, the VIR_AUTOFREE declarations should move at the end of the "declare"
block within functions (like I said, it looks IMO better and separates regular
variables from the ones that can be freed automatically).

Erik


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]