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