3:57 a.m.
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(a)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