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@xxxxxxxxx>
> ---
...
>
> @@ -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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
