1:05 a.m.
If the kernel doesn't have CONFIG_NET_DEVLINK then net devices are not
listed in nodedev-list. See 2/3 for more info.
Michal Prívozník (3):
virNetDevGetFamilyId: Change signature
virNetDevSwitchdevFeature: Make failure to get 'family_id' non-fatal
virnetdev.c: Use g_auto*()
src/util/virnetdev.c | 412 +++++++++++++++++--------------------------
1 file changed, 157 insertions(+), 255 deletions(-)
--
2.25.3
1:05 a.m.
New subject: [PATCH 1/3] virNetDevGetFamilyId: Change signature
Introduced in v3.8.0-rc1~96, the virNetDevGetFamilyId() gets
netlink family ID for passed family name (even though it's used
only for getting "devlink" ID). Nevertheless, the function
returns 0 on an error or if no family ID was found. This makes it
harder for a caller to distinguish these two. Change the retval
so that a negative value is returned upon error, zero is no ID
found (but no error encountered) and a positive value is returned
on successful translation.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/util/virnetdev.c | 23 +++++++++++++++--------
1 file changed, 15 insertions(+), 8 deletions(-)
diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c
index b465bdac2e..8c149c5fc5 100644
--- a/src/util/virnetdev.c
+++ b/src/util/virnetdev.c
@@ -3057,11 +3057,15 @@ virNetDevGetEthtoolFeatures(virBitmapPtr bitmap,
* This function supplies the devlink family id
*
* @family_name: the name of the family to query
+ * @family_id: family ID
*
- * Returns family id or 0 on failure.
+ * Returns: 0 if no family was found,
+ * 1 if family was found (@family_id is set),
+ * -1 othewise.
*/
-static uint32_t
-virNetDevGetFamilyId(const char *family_name)
+static int
+virNetDevGetFamilyId(const char *family_name,
+ uint32_t *family_id)
{
struct nl_msg *nl_msg = NULL;
struct nlmsghdr *resp = NULL;
@@ -3072,7 +3076,7 @@ virNetDevGetFamilyId(const char *family_name)
};
struct nlattr *tb[CTRL_ATTR_MAX + 1] = {NULL, };
unsigned int recvbuflen;
- uint32_t family_id = 0;
+ int ret = -1;
if (!(nl_msg = nlmsg_alloc_simple(GENL_ID_CTRL,
NLM_F_REQUEST | NLM_F_ACK))) {
@@ -3098,15 +3102,18 @@ virNetDevGetFamilyId(const char *family_name)
goto cleanup;
}
- if (tb[CTRL_ATTR_FAMILY_ID] == NULL)
+ if (tb[CTRL_ATTR_FAMILY_ID] == NULL) {
+ ret = 0;
goto cleanup;
+ }
- family_id = *(uint32_t *)RTA_DATA(tb[CTRL_ATTR_FAMILY_ID]);
+ *family_id = *(uint32_t *)RTA_DATA(tb[CTRL_ATTR_FAMILY_ID]);
+ ret = 1;
cleanup:
nlmsg_free(nl_msg);
VIR_FREE(resp);
- return family_id;
+ return ret;
}
@@ -3140,7 +3147,7 @@ virNetDevSwitchdevFeature(const char *ifname,
int ret = -1;
uint32_t family_id;
- if ((family_id = virNetDevGetFamilyId(DEVLINK_GENL_NAME)) <= 0)
+ if (virNetDevGetFamilyId(DEVLINK_GENL_NAME, &family_id) <= 0)
return ret;
if ((is_vf = virNetDevIsVirtualFunction(ifname)) < 0)
--
2.25.3
3:07 a.m.
New subject: [PATCH 1/3] virNetDevGetFamilyId: Change signature
On a Monday in 2020, Michal Privoznik wrote:
Introduced in v3.8.0-rc1~96, the virNetDevGetFamilyId() gets
netlink family ID for passed family name (even though it's used
only for getting "devlink" ID). Nevertheless, the function
returns 0 on an error or if no family ID was found. This makes it
harder for a caller to distinguish these two. Change the retval
so that a negative value is returned upon error, zero is no ID
found (but no error encountered) and a positive value is returned
on successful translation.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/util/virnetdev.c | 23 +++++++++++++++--------
1 file changed, 15 insertions(+), 8 deletions(-)
diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c
index b465bdac2e..8c149c5fc5 100644
--- a/src/util/virnetdev.c
+++ b/src/util/virnetdev.c
@@ -3057,11 +3057,15 @@ virNetDevGetEthtoolFeatures(virBitmapPtr bitmap,
* This function supplies the devlink family id
*
* @family_name: the name of the family to query
+ * @family_id: family ID
*
- * Returns family id or 0 on failure.
+ * Returns: 0 if no family was found,
+ * 1 if family was found (@family_id is set),
+ * -1 othewise.
otherwise
*/
-static uint32_t
-virNetDevGetFamilyId(const char *family_name)
+static int
+virNetDevGetFamilyId(const char *family_name,
+ uint32_t *family_id)
{
struct nl_msg *nl_msg = NULL;
struct nlmsghdr *resp = NULL;
Reviewed-by: Ján Tomko <jtomko(a)redhat.com>
Jano
1:05 a.m.
New subject: [PATCH 2/3] virNetDevSwitchdevFeature: Make failure to get 'family_id' non-fatal
I've just got a new machine and I'm still converging on the
kernel config. Anyway, since I don't have enabled any of SRIO-V
drivers, my kernel doesn't have NET_DEVLINK enabled (i.e.
virNetDevGetFamilyId() returns 0). But this makes nodedev driver
ignore all intefaces, because when enumerating all devices via
udev, the control reaches virNetDevSwitchdevFeature() eventually
and subsequently virNetDevGetFamilyId() which 'fails'. Well, it's
not really a failure - the virNetDevSwitchdevFeature() stub
simply returns 0.
Also, move the call a few lines below, just around the place
where it's needed.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/util/virnetdev.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c
index 8c149c5fc5..9bca8ce759 100644
--- a/src/util/virnetdev.c
+++ b/src/util/virnetdev.c
@@ -3146,9 +3146,7 @@ virNetDevSwitchdevFeature(const char *ifname,
int is_vf = -1;
int ret = -1;
uint32_t family_id;
-
- if (virNetDevGetFamilyId(DEVLINK_GENL_NAME, &family_id) <= 0)
- return ret;
+ int rv;
if ((is_vf = virNetDevIsVirtualFunction(ifname)) < 0)
return ret;
@@ -3168,6 +3166,9 @@ virNetDevSwitchdevFeature(const char *ifname,
goto cleanup;
}
+ if ((rv = virNetDevGetFamilyId(DEVLINK_GENL_NAME, &family_id)) <= 0)
+ return rv;
+
if (!(nl_msg = nlmsg_alloc_simple(family_id,
NLM_F_REQUEST | NLM_F_ACK))) {
virReportOOMError();
--
2.25.3
3:10 a.m.
New subject: [PATCH 2/3] virNetDevSwitchdevFeature: Make failure to get 'family_id' non-fatal
On a Monday in 2020, Michal Privoznik wrote:
I've just got a new machine and I'm still converging on the
kernel config. Anyway, since I don't have enabled any of SRIO-V
drivers, my kernel doesn't have NET_DEVLINK enabled (i.e.
virNetDevGetFamilyId() returns 0). But this makes nodedev driver
ignore all intefaces, because when enumerating all devices via
interfaces
udev, the control reaches virNetDevSwitchdevFeature() eventually
and subsequently virNetDevGetFamilyId() which 'fails'. Well, it's
not really a failure - the virNetDevSwitchdevFeature() stub
simply returns 0.
Also, move the call a few lines below, just around the place
where it's needed.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/util/virnetdev.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
Reviewed-by: Ján Tomko <jtomko(a)redhat.com>
Jano
1:05 a.m.
New subject: [PATCH 3/3] virnetdev.c: Use g_auto*()
While I'm at it, use more g_autofree and g_autoptr() in this
file. This also fixes a possible mem-leak in
virNetDevGetVirtualFunctions().
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/util/virnetdev.c | 384 ++++++++++++++++---------------------------
1 file changed, 139 insertions(+), 245 deletions(-)
diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c
index 9bca8ce759..650e81c456 100644
--- a/src/util/virnetdev.c
+++ b/src/util/virnetdev.c
@@ -505,17 +505,16 @@ int virNetDevSetMTUFromDevice(const char *ifname,
*/
int virNetDevSetNamespace(const char *ifname, pid_t pidInNs)
{
- int ret = -1;
- char *pid = NULL;
- char *phy = NULL;
- char *phy_path = NULL;
+ g_autofree char *pid = NULL;
+ g_autofree char *phy = NULL;
+ g_autofree char *phy_path = NULL;
int len;
pid = g_strdup_printf("%lld", (long long) pidInNs);
/* The 802.11 wireless devices only move together with their PHY. */
if (virNetDevSysfsFile(&phy_path, ifname, "phy80211/name") < 0)
- goto cleanup;
+ return -1;
if ((len = virFileReadAllQuiet(phy_path, 1024, &phy)) <= 0) {
/* Not a wireless device. */
@@ -525,7 +524,7 @@ int virNetDevSetNamespace(const char *ifname, pid_t pidInNs)
argv[5] = pid;
if (virRun(argv, NULL) < 0)
- goto cleanup;
+ return -1;
} else {
const char *argv[] = {
@@ -538,15 +537,10 @@ int virNetDevSetNamespace(const char *ifname, pid_t pidInNs)
argv[2] = phy;
argv[5] = pid;
if (virRun(argv, NULL) < 0)
- goto cleanup;
+ return -1;
}
- ret = 0;
- cleanup:
- VIR_FREE(phy_path);
- VIR_FREE(phy);
- VIR_FREE(pid);
- return ret;
+ return 0;
}
#if defined(SIOCSIFNAME) && defined(HAVE_STRUCT_IFREQ)
@@ -913,25 +907,21 @@ int virNetDevGetIndex(const char *ifname G_GNUC_UNUSED,
int
virNetDevGetMaster(const char *ifname, char **master)
{
- int ret = -1;
- void *nlData = NULL;
+ g_autofree void *nlData = NULL;
struct nlattr *tb[IFLA_MAX + 1] = {NULL, };
*master = NULL;
if (virNetlinkDumpLink(ifname, -1, &nlData, tb, 0, 0) < 0)
- goto cleanup;
+ return -1;
if (tb[IFLA_MASTER]) {
if (!(*master = virNetDevGetName(*(int *)RTA_DATA(tb[IFLA_MASTER]))))
- goto cleanup;
+ return -1;
}
VIR_DEBUG("IFLA_MASTER for %s is %s", ifname, *master ? *master :
"(none)");
- ret = 0;
- cleanup:
- VIR_FREE(nlData);
- return ret;
+ return 0;
}
@@ -1090,57 +1080,44 @@ virNetDevSysfsDeviceFile(char **pf_sysfs_device_link, const char
*ifname,
static bool
virNetDevIsPCIDevice(const char *devpath)
{
- char *subsys_link = NULL;
- char *abs_path = NULL;
+ g_autofree char *subsys_link = NULL;
+ g_autofree char *abs_path = NULL;
g_autofree char *subsys = NULL;
- bool ret = false;
subsys_link = g_strdup_printf("%s/subsystem", devpath);
if (!virFileExists(subsys_link))
- goto cleanup;
+ return false;
if (virFileResolveLink(subsys_link, &abs_path) < 0) {
virReportError(VIR_ERR_INTERNAL_ERROR,
_("Unable to resolve device subsystem symlink %s"),
subsys_link);
- goto cleanup;
+ return false;
}
subsys = g_path_get_basename(abs_path);
- ret = STRPREFIX(subsys, "pci");
-
- cleanup:
- VIR_FREE(subsys_link);
- VIR_FREE(abs_path);
- return ret;
+ return STRPREFIX(subsys, "pci");
}
static virPCIDevicePtr
virNetDevGetPCIDevice(const char *devName)
{
- char *vfSysfsDevicePath = NULL;
- virPCIDeviceAddressPtr vfPCIAddr = NULL;
- virPCIDevicePtr vfPCIDevice = NULL;
+ g_autofree char *vfSysfsDevicePath = NULL;
+ g_autoptr(virPCIDeviceAddress) vfPCIAddr = NULL;
if (virNetDevSysfsFile(&vfSysfsDevicePath, devName, "device") < 0)
- goto cleanup;
+ return NULL;
if (!virNetDevIsPCIDevice(vfSysfsDevicePath))
- goto cleanup;
+ return NULL;
vfPCIAddr = virPCIGetDeviceAddressFromSysfsLink(vfSysfsDevicePath);
if (!vfPCIAddr)
- goto cleanup;
+ return NULL;
- vfPCIDevice = virPCIDeviceNew(vfPCIAddr->domain, vfPCIAddr->bus,
- vfPCIAddr->slot, vfPCIAddr->function);
-
- cleanup:
- VIR_FREE(vfSysfsDevicePath);
- VIR_FREE(vfPCIAddr);
-
- return vfPCIDevice;
+ return virPCIDeviceNew(vfPCIAddr->domain, vfPCIAddr->bus,
+ vfPCIAddr->slot, vfPCIAddr->function);
}
@@ -1162,25 +1139,20 @@ int
virNetDevGetPhysPortID(const char *ifname,
char **physPortID)
{
- int ret = -1;
- char *physPortIDFile = NULL;
+ g_autofree char *physPortIDFile = NULL;
*physPortID = NULL;
if (virNetDevSysfsFile(&physPortIDFile, ifname, "phys_port_id") <
0)
- goto cleanup;
+ return -1;
/* a failure to read just means the driver doesn't support
* phys_port_id, so set success now and ignore the return from
* virFileReadAllQuiet().
*/
- ret = 0;
ignore_value(virFileReadAllQuiet(physPortIDFile, 1024, physPortID));
-
- cleanup:
- VIR_FREE(physPortIDFile);
- return ret;
+ return 0;
}
@@ -1203,10 +1175,8 @@ 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;
+ g_autofree char *pf_sysfs_device_link = NULL;
+ g_autofree char *pfPhysPortID = NULL;
*virt_fns = NULL;
*n_vfname = 0;
@@ -1226,6 +1196,9 @@ virNetDevGetVirtualFunctions(const char *pfname,
goto cleanup;
for (i = 0; i < *n_vfname; i++) {
+ g_autofree char *pciConfigAddr = NULL;
+ g_autofree char *pci_sysfs_device_link = NULL;
+
if (!(pciConfigAddr = virPCIDeviceAddressAsString((*virt_fns)[i])))
goto cleanup;
@@ -1248,13 +1221,14 @@ virNetDevGetVirtualFunctions(const char *pfname,
cleanup:
if (ret < 0) {
- VIR_FREE(*vfname);
+ virStringListFreeCount(*vfname, *n_vfname);
+
+ for (i = 0; i < *n_vfname; i++)
+ VIR_FREE((*virt_fns)[i]);
VIR_FREE(*virt_fns);
+ *vfname = NULL;
+ *n_vfname = 0;
}
- VIR_FREE(pfPhysPortID);
- VIR_FREE(pf_sysfs_device_link);
- VIR_FREE(pci_sysfs_device_link);
- VIR_FREE(pciConfigAddr);
return ret;
}
@@ -1270,17 +1244,12 @@ virNetDevGetVirtualFunctions(const char *pfname,
int
virNetDevIsVirtualFunction(const char *ifname)
{
- char *if_sysfs_device_link = NULL;
- int ret = -1;
+ g_autofree char *if_sysfs_device_link = NULL;
if (virNetDevSysfsFile(&if_sysfs_device_link, ifname, "device") <
0)
- return ret;
+ return -1;
- ret = virPCIIsVirtualFunction(if_sysfs_device_link);
-
- VIR_FREE(if_sysfs_device_link);
-
- return ret;
+ return virPCIIsVirtualFunction(if_sysfs_device_link);
}
/**
@@ -1298,25 +1267,18 @@ int
virNetDevGetVirtualFunctionIndex(const char *pfname, const char *vfname,
int *vf_index)
{
- char *pf_sysfs_device_link = NULL, *vf_sysfs_device_link = NULL;
- int ret = -1;
+ g_autofree char *pf_sysfs_device_link = NULL;
+ g_autofree char *vf_sysfs_device_link = NULL;
if (virNetDevSysfsFile(&pf_sysfs_device_link, pfname, "device") <
0)
- return ret;
+ return -1;
- if (virNetDevSysfsFile(&vf_sysfs_device_link, vfname, "device") < 0)
{
- VIR_FREE(pf_sysfs_device_link);
- return ret;
- }
+ if (virNetDevSysfsFile(&vf_sysfs_device_link, vfname, "device") <
0)
+ return -1;
- ret = virPCIGetVirtualFunctionIndex(pf_sysfs_device_link,
- vf_sysfs_device_link,
- vf_index);
-
- VIR_FREE(pf_sysfs_device_link);
- VIR_FREE(vf_sysfs_device_link);
-
- return ret;
+ return virPCIGetVirtualFunctionIndex(pf_sysfs_device_link,
+ vf_sysfs_device_link,
+ vf_index);
}
/**
@@ -1332,19 +1294,18 @@ virNetDevGetVirtualFunctionIndex(const char *pfname, const char
*vfname,
int
virNetDevGetPhysicalFunction(const char *ifname, char **pfname)
{
- char *physfn_sysfs_path = NULL;
- char *vfPhysPortID = NULL;
- int ret = -1;
+ g_autofree char *physfn_sysfs_path = NULL;
+ g_autofree char *vfPhysPortID = NULL;
if (virNetDevGetPhysPortID(ifname, &vfPhysPortID) < 0)
- goto cleanup;
+ return -1;
if (virNetDevSysfsDeviceFile(&physfn_sysfs_path, ifname, "physfn") <
0)
- goto cleanup;
+ return -1;
if (virPCIGetNetName(physfn_sysfs_path, 0,
vfPhysPortID, pfname) < 0) {
- goto cleanup;
+ return -1;
}
if (!*pfname) {
@@ -1353,14 +1314,10 @@ virNetDevGetPhysicalFunction(const char *ifname, char **pfname)
virReportError(VIR_ERR_INTERNAL_ERROR,
_("The PF device for VF %s has no network device
name"),
ifname);
- goto cleanup;
+ return -1;
}
- ret = 0;
- cleanup:
- VIR_FREE(vfPhysPortID);
- VIR_FREE(physfn_sysfs_path);
- return ret;
+ return 0;
}
@@ -1384,17 +1341,16 @@ virNetDevGetPhysicalFunction(const char *ifname, char **pfname)
int
virNetDevPFGetVF(const char *pfname, int vf, char **vfname)
{
- char *virtfnName = NULL;
- char *virtfnSysfsPath = NULL;
- char *pfPhysPortID = NULL;
- int ret = -1;
+ g_autofree char *virtfnName = NULL;
+ g_autofree char *virtfnSysfsPath = NULL;
+ g_autofree char *pfPhysPortID = NULL;
/* a VF may have multiple "ports", each one having its own netdev,
* and each netdev having a different phys_port_id. Be sure we get
* the VF netdev with a phys_port_id matchine that of pfname
*/
if (virNetDevGetPhysPortID(pfname, &pfPhysPortID) < 0)
- goto cleanup;
+ return -1;
virtfnName = g_strdup_printf("virtfn%d", vf);
@@ -1402,7 +1358,7 @@ virNetDevPFGetVF(const char *pfname, int vf, char **vfname)
* e.g. "/sys/class/net/enp2s0f0/virtfn3"
*/
if (virNetDevSysfsDeviceFile(&virtfnSysfsPath, pfname, virtfnName) < 0)
- goto cleanup;
+ return -1;
/* and this gets the netdev name associated with it, which is a
* directory entry in [virtfnSysfsPath]/net,
@@ -1411,14 +1367,7 @@ virNetDevPFGetVF(const char *pfname, int vf, char **vfname)
* isn't bound to a netdev driver, it won't have a netdev name,
* and vfname will be NULL).
*/
- ret = virPCIGetNetName(virtfnSysfsPath, 0, pfPhysPortID, vfname);
-
- cleanup:
- VIR_FREE(virtfnName);
- VIR_FREE(virtfnSysfsPath);
- VIR_FREE(pfPhysPortID);
-
- return ret;
+ return virPCIGetNetName(virtfnSysfsPath, 0, pfPhysPortID, vfname);
}
@@ -1559,7 +1508,7 @@ virNetDevSetVfConfig(const char *ifname, int vf,
{
int rc = -1;
char macstr[VIR_MAC_STRING_BUFLEN];
- struct nlmsghdr *resp = NULL;
+ g_autofree struct nlmsghdr *resp = NULL;
struct nlmsgerr *err;
unsigned int recvbuflen = 0;
struct nl_msg *nl_msg;
@@ -1671,7 +1620,6 @@ virNetDevSetVfConfig(const char *ifname, int vf,
vlanid, rc < 0 ? "Fail" : "Success");
nlmsg_free(nl_msg);
- VIR_FREE(resp);
return rc;
malformed_resp:
@@ -1743,20 +1691,14 @@ static int
virNetDevGetVfConfig(const char *ifname, int vf, virMacAddrPtr mac,
int *vlanid)
{
- int rc = -1;
- void *nlData = NULL;
+ g_autofree void *nlData = NULL;
struct nlattr *tb[IFLA_MAX + 1] = {NULL, };
int ifindex = -1;
- rc = virNetlinkDumpLink(ifname, ifindex, &nlData, tb, 0, 0);
- if (rc < 0)
- goto cleanup;
+ if (virNetlinkDumpLink(ifname, ifindex, &nlData, tb, 0, 0) < 0)
+ return -1;
- rc = virNetDevParseVfConfig(tb, vf, mac, vlanid);
-
- cleanup:
- VIR_FREE(nlData);
- return rc;
+ return virNetDevParseVfConfig(tb, vf, mac, vlanid);
}
@@ -1813,16 +1755,15 @@ virNetDevSaveNetConfig(const char *linkdev, int vf,
const char *stateDir,
bool saveVlan)
{
- int ret = -1;
const char *pfDevName = NULL;
- char *pfDevOrig = NULL;
- char *vfDevOrig = NULL;
+ g_autofree char *pfDevOrig = NULL;
+ g_autofree char *vfDevOrig = NULL;
virMacAddr oldMAC;
char MACStr[VIR_MAC_STRING_BUFLEN];
int oldVlanTag = -1;
- char *filePath = NULL;
- char *fileStr = NULL;
- virJSONValuePtr configJSON = NULL;
+ g_autofree char *filePath = NULL;
+ g_autofree char *fileStr = NULL;
+ g_autoptr(virJSONValue) configJSON = NULL;
if (vf >= 0) {
/* linkdev is the PF */
@@ -1830,7 +1771,7 @@ virNetDevSaveNetConfig(const char *linkdev, int vf,
/* linkdev should get the VF's netdev name (or NULL if none) */
if (virNetDevPFGetVF(pfDevName, vf, &vfDevOrig) < 0)
- goto cleanup;
+ return -1;
linkdev = vfDevOrig;
saveVlan = true;
@@ -1842,7 +1783,7 @@ virNetDevSaveNetConfig(const char *linkdev, int vf,
*/
if (virNetDevGetVirtualFunctionInfo(linkdev, &pfDevOrig, &vf) < 0)
- goto cleanup;
+ return -1;
pfDevName = pfDevOrig;
}
@@ -1861,7 +1802,7 @@ virNetDevSaveNetConfig(const char *linkdev, int vf,
* explicitly enable the PF in the host system network config.
*/
if (virNetDevGetOnline(pfDevName, &pfIsOnline) < 0)
- goto cleanup;
+ return -1;
if (!pfIsOnline) {
virReportError(VIR_ERR_INTERNAL_ERROR,
@@ -1870,7 +1811,7 @@ virNetDevSaveNetConfig(const char *linkdev, int vf,
"change host network config to put the "
"PF online."),
vf, pfDevName);
- goto cleanup;
+ return -1;
}
}
@@ -1886,7 +1827,7 @@ virNetDevSaveNetConfig(const char *linkdev, int vf,
/* get admin MAC and vlan tag */
if (virNetDevGetVfConfig(pfDevName, vf, &oldMAC, &oldVlanTag) < 0)
- goto cleanup;
+ return -1;
if (virJSONValueObjectAppendString(configJSON,
VIR_NETDEV_KEYNAME_ADMIN_MAC,
@@ -1894,7 +1835,7 @@ virNetDevSaveNetConfig(const char *linkdev, int vf,
virJSONValueObjectAppendNumberInt(configJSON,
VIR_NETDEV_KEYNAME_VLAN_TAG,
oldVlanTag) < 0) {
- goto cleanup;
+ return -1;
}
} else {
@@ -1903,33 +1844,26 @@ virNetDevSaveNetConfig(const char *linkdev, int vf,
if (linkdev) {
if (virNetDevGetMAC(linkdev, &oldMAC) < 0)
- goto cleanup;
+ return -1;
/* for interfaces with no pfDevName (i.e. not a VF, this will
* be the only value in the file.
*/
if (virJSONValueObjectAppendString(configJSON, VIR_NETDEV_KEYNAME_MAC,
virMacAddrFormat(&oldMAC, MACStr)) <
0)
- goto cleanup;
+ return -1;
}
if (!(fileStr = virJSONValueToString(configJSON, true)))
- goto cleanup;
+ return -1;
if (virFileWriteStr(filePath, fileStr, O_CREAT|O_TRUNC|O_WRONLY) < 0) {
virReportSystemError(errno, _("Unable to preserve mac/vlan tag "
"for device = %s, vf = %d"), linkdev,
vf);
- goto cleanup;
+ return -1;
}
- ret = 0;
- cleanup:
- VIR_FREE(pfDevOrig);
- VIR_FREE(vfDevOrig);
- VIR_FREE(filePath);
- VIR_FREE(fileStr);
- virJSONValueFree(configJSON);
- return ret;
+ return 0;
}
@@ -1963,11 +1897,11 @@ virNetDevReadNetConfig(const char *linkdev, int vf,
{
int ret = -1;
const char *pfDevName = NULL;
- char *pfDevOrig = NULL;
- char *vfDevOrig = NULL;
- char *filePath = NULL;
- char *fileStr = NULL;
- virJSONValuePtr configJSON = NULL;
+ g_autofree char *pfDevOrig = NULL;
+ g_autofree char *vfDevOrig = NULL;
+ g_autofree char *filePath = NULL;
+ g_autofree char *fileStr = NULL;
+ g_autoptr(virJSONValue) configJSON = NULL;
const char *MACStr = NULL;
const char *adminMACStr = NULL;
int vlanTag = -1;
@@ -2132,11 +2066,6 @@ virNetDevReadNetConfig(const char *linkdev, int vf,
VIR_FREE(*vlan);
}
- VIR_FREE(pfDevOrig);
- VIR_FREE(vfDevOrig);
- VIR_FREE(filePath);
- VIR_FREE(fileStr);
- virJSONValueFree(configJSON);
return ret;
}
@@ -2166,13 +2095,12 @@ virNetDevSetNetConfig(const char *linkdev, int vf,
const virMacAddr *MAC,
bool setVlan)
{
- int ret = -1;
char MACStr[VIR_MAC_STRING_BUFLEN];
const char *pfDevName = NULL;
- char *pfDevOrig = NULL;
- char *vfDevOrig = NULL;
+ g_autofree char *pfDevOrig = NULL;
+ g_autofree char *vfDevOrig = NULL;
int vlanTag = -1;
- virPCIDevicePtr vfPCIDevice = NULL;
+ g_autoptr(virPCIDevice) vfPCIDevice = NULL;
if (vf >= 0) {
/* linkdev is the PF */
@@ -2180,7 +2108,7 @@ virNetDevSetNetConfig(const char *linkdev, int vf,
/* linkdev should get the VF's netdev name (or NULL if none) */
if (virNetDevPFGetVF(pfDevName, vf, &vfDevOrig) < 0)
- goto cleanup;
+ return -1;
linkdev = vfDevOrig;
@@ -2191,7 +2119,7 @@ virNetDevSetNetConfig(const char *linkdev, int vf,
*/
if (virNetDevGetVirtualFunctionInfo(linkdev, &pfDevOrig, &vf))
- goto cleanup;
+ return -1;
pfDevName = pfDevOrig;
}
@@ -2204,14 +2132,14 @@ virNetDevSetNetConfig(const char *linkdev, int vf,
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("admin MAC can only be set for SR-IOV VFs, but "
"%s is not a VF"), linkdev);
- goto cleanup;
+ return -1;
}
if (vlan) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("vlan can only be set for SR-IOV VFs, but "
"%s is not a VF"), linkdev);
- goto cleanup;
+ return -1;
}
} else {
@@ -2220,14 +2148,14 @@ virNetDevSetNetConfig(const char *linkdev, int vf,
virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
_("vlan trunking is not supported "
"by SR-IOV network devices"));
- goto cleanup;
+ return -1;
}
if (!setVlan) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("vlan tag set for interface %s but "
"caller requested it not be set"));
- goto cleanup;
+ return -1;
}
vlanTag = vlan->tag[0];
@@ -2245,7 +2173,7 @@ virNetDevSetNetConfig(const char *linkdev, int vf,
_("VF %d of PF '%s' is not bound to a net driver,
"
"so its MAC address cannot be set to %s"),
vf, pfDevName, virMacAddrFormat(MAC, MACStr));
- goto cleanup;
+ return -1;
}
setMACrc = virNetDevSetMACInternal(linkdev, MAC, !!pfDevOrig);
@@ -2256,7 +2184,7 @@ virNetDevSetNetConfig(const char *linkdev, int vf,
/* if pfDevOrig == NULL, this isn't a VF, so we've failed */
if (!pfDevOrig ||
(errno != EADDRNOTAVAIL && errno != EPERM))
- goto cleanup;
+ return -1;
/* Otherwise this is a VF, and virNetDevSetMAC failed with
* EADDRNOTAVAIL/EPERM, which could be due to the
@@ -2270,18 +2198,18 @@ virNetDevSetNetConfig(const char *linkdev, int vf,
if (virNetDevSetVfConfig(pfDevName, vf,
MAC, vlanTag, &allowRetry) < 0) {
- goto cleanup;
+ return -1;
}
/* admin MAC is set, now we need to construct a virPCIDevice
* object so we can call virPCIDeviceRebind()
*/
if (!(vfPCIDevice = virNetDevGetPCIDevice(linkdev)))
- goto cleanup;
+ return -1;
/* Rebind the device. This should set the proper MAC address */
if (virPCIDeviceRebind(vfPCIDevice) < 0)
- goto cleanup;
+ return -1;
/* Wait until virNetDevGetIndex for the VF netdev returns success.
* This indicates that the device is ready to be used. If we don't
@@ -2333,22 +2261,17 @@ virNetDevSetNetConfig(const char *linkdev, int vf,
* with the "locally administered" bit set.
*/
if (!allowRetry)
- goto cleanup;
+ return -1;
allowRetry = false;
if (virNetDevSetVfConfig(pfDevName, vf,
&altZeroMAC, vlanTag, &allowRetry) < 0)
{
- goto cleanup;
+ return -1;
}
}
}
- ret = 0;
- cleanup:
- VIR_FREE(pfDevOrig);
- VIR_FREE(vfDevOrig);
- virPCIDeviceFree(vfPCIDevice);
- return ret;
+ return 0;
}
@@ -2428,28 +2351,27 @@ int
virNetDevGetLinkInfo(const char *ifname,
virNetDevIfLinkPtr lnk)
{
- int ret = -1;
- char *path = NULL;
- char *buf = NULL;
+ g_autofree char *path = NULL;
+ g_autofree char *buf = NULL;
char *tmp;
int tmp_state;
unsigned int tmp_speed;
if (virNetDevSysfsFile(&path, ifname, "operstate") < 0)
- goto cleanup;
+ return -1;
if (virFileReadAll(path, 1024, &buf) < 0) {
virReportSystemError(errno,
_("unable to read: %s"),
path);
- goto cleanup;
+ return -1;
}
if (!(tmp = strchr(buf, '\n'))) {
virReportError(VIR_ERR_INTERNAL_ERROR,
_("Unable to parse: %s"),
buf);
- goto cleanup;
+ return -1;
}
*tmp = '\0';
@@ -2460,7 +2382,7 @@ virNetDevGetLinkInfo(const char *ifname,
virReportError(VIR_ERR_INTERNAL_ERROR,
_("Unable to parse: %s"),
buf);
- goto cleanup;
+ return -1;
}
lnk->state = tmp_state;
@@ -2471,26 +2393,23 @@ virNetDevGetLinkInfo(const char *ifname,
* speed if that's the case. */
if (lnk->state != VIR_NETDEV_IF_STATE_UP) {
lnk->speed = 0;
- ret = 0;
- goto cleanup;
+ return 0;
}
VIR_FREE(path);
VIR_FREE(buf);
if (virNetDevSysfsFile(&path, ifname, "speed") < 0)
- goto cleanup;
+ return -1;
if (virFileReadAllQuiet(path, 1024, &buf) < 0) {
/* Some devices doesn't report speed, in which case we get EINVAL */
- if (errno == EINVAL) {
- ret = 0;
- goto cleanup;
- }
+ if (errno == EINVAL)
+ return 0;
virReportSystemError(errno,
_("unable to read: %s"),
path);
- goto cleanup;
+ return -1;
}
if (virStrToLong_ui(buf, &tmp, 10, &tmp_speed) < 0 ||
@@ -2498,16 +2417,12 @@ virNetDevGetLinkInfo(const char *ifname,
virReportError(VIR_ERR_INTERNAL_ERROR,
_("Unable to parse: %s"),
buf);
- goto cleanup;
+ return -1;
}
lnk->speed = tmp_speed;
- ret = 0;
- cleanup:
- VIR_FREE(buf);
- VIR_FREE(path);
- return ret;
+ return 0;
}
#else
@@ -2707,9 +2622,9 @@ static int virNetDevGetMcastList(const char *ifname,
virNetDevMcastListPtr mcast)
{
char *cur = NULL;
- char *buf = NULL;
+ g_autofree char *buf = NULL;
char *next = NULL;
- int ret = -1, len;
+ int len;
g_autoptr(virNetDevMcastEntry) entry = NULL;
mcast->entries = NULL;
@@ -2717,35 +2632,31 @@ static int virNetDevGetMcastList(const char *ifname,
/* Read entire multicast table into memory */
if ((len = virFileReadAll(PROC_NET_DEV_MCAST, MAX_MCAST_SIZE, &buf)) <= 0)
- goto cleanup;
+ return -1;
cur = buf;
while (cur) {
if (!entry && VIR_ALLOC(entry) < 0)
- goto cleanup;
+ return -1;
next = strchr(cur, '\n');
if (next)
next++;
if (virNetDevParseMcast(cur, entry))
- goto cleanup;
+ return -1;
/* Only return global multicast MAC addresses for
* specified interface */
if (entry->global && STREQ(ifname, entry->name)) {
if (VIR_APPEND_ELEMENT(mcast->entries, mcast->nentries, entry))
- goto cleanup;
+ return -1;
} else {
memset(entry, 0, sizeof(virNetDevMcastEntry));
}
cur = next && ((next - buf) < len) ? next : NULL;
}
- ret = 0;
- cleanup:
- VIR_FREE(buf);
-
- return ret;
+ return 0;
}
@@ -2883,10 +2794,8 @@ static int
virNetDevRDMAFeature(const char *ifname,
virBitmapPtr *out)
{
- char *eth_devpath = NULL;
- char *ib_devpath = NULL;
- char *eth_res_buf = NULL;
- char *ib_res_buf = NULL;
+ g_autofree char *eth_devpath = NULL;
+ g_autofree char *eth_res_buf = NULL;
DIR *dirp = NULL;
struct dirent *dp;
int ret = -1;
@@ -2910,24 +2819,20 @@ virNetDevRDMAFeature(const char *ifname,
goto cleanup;
while (virDirRead(dirp, &dp, SYSFS_INFINIBAND_DIR) > 0) {
- ib_devpath = g_strdup_printf(SYSFS_INFINIBAND_DIR
"%s/device/resource",
- dp->d_name);
+ g_autofree char *ib_res_buf = NULL;
+ g_autofree char *ib_devpath = g_strdup_printf(SYSFS_INFINIBAND_DIR
"%s/device/resource",
+ dp->d_name);
+
if (virFileReadAll(ib_devpath, RESOURCE_FILE_LEN, &ib_res_buf) > 0
&&
STREQ(eth_res_buf, ib_res_buf)) {
ignore_value(virBitmapSetBit(*out, VIR_NET_DEV_FEAT_RDMA));
break;
}
- VIR_FREE(ib_devpath);
- VIR_FREE(ib_res_buf);
}
ret = 0;
cleanup:
VIR_DIR_CLOSE(dirp);
- VIR_FREE(eth_devpath);
- VIR_FREE(ib_devpath);
- VIR_FREE(eth_res_buf);
- VIR_FREE(ib_res_buf);
return ret;
}
@@ -3068,7 +2973,7 @@ virNetDevGetFamilyId(const char *family_name,
uint32_t *family_id)
{
struct nl_msg *nl_msg = NULL;
- struct nlmsghdr *resp = NULL;
+ g_autofree struct nlmsghdr *resp = NULL;
struct genlmsghdr gmsgh = {
.cmd = CTRL_CMD_GETFAMILY,
.version = DEVLINK_GENL_VERSION,
@@ -3112,7 +3017,6 @@ virNetDevGetFamilyId(const char *family_name,
cleanup:
nlmsg_free(nl_msg);
- VIR_FREE(resp);
return ret;
}
@@ -3132,17 +3036,17 @@ virNetDevSwitchdevFeature(const char *ifname,
virBitmapPtr *out)
{
struct nl_msg *nl_msg = NULL;
- struct nlmsghdr *resp = NULL;
+ g_autofree struct nlmsghdr *resp = NULL;
unsigned int recvbuflen;
struct nlattr *tb[DEVLINK_ATTR_MAX + 1] = {NULL, };
- virPCIDevicePtr pci_device_ptr = NULL;
+ g_autoptr(virPCIDevice) pci_device_ptr = NULL;
struct genlmsghdr gmsgh = {
.cmd = DEVLINK_CMD_ESWITCH_GET,
.version = DEVLINK_GENL_VERSION,
.reserved = 0,
};
const char *pci_name;
- char *pfname = NULL;
+ g_autofree char *pfname = NULL;
int is_vf = -1;
int ret = -1;
uint32_t family_id;
@@ -3161,10 +3065,8 @@ virNetDevSwitchdevFeature(const char *ifname,
pci_device_ptr = pfname ? virNetDevGetPCIDevice(pfname) :
virNetDevGetPCIDevice(ifname);
/* No PCI device, then no feature bit to check/add */
- if (pci_device_ptr == NULL) {
- ret = 0;
- goto cleanup;
- }
+ if (pci_device_ptr == NULL)
+ return 0;
if ((rv = virNetDevGetFamilyId(DEVLINK_GENL_NAME, &family_id)) <= 0)
return rv;
@@ -3205,9 +3107,6 @@ virNetDevSwitchdevFeature(const char *ifname,
cleanup:
nlmsg_free(nl_msg);
- virPCIDeviceFree(pci_device_ptr);
- VIR_FREE(resp);
- VIR_FREE(pfname);
return ret;
}
# else
@@ -3248,7 +3147,7 @@ virNetDevGetEthtoolGFeatures(virBitmapPtr bitmap,
int fd,
struct ifreq *ifr)
{
- struct ethtool_gfeatures *g_cmd;
+ g_autofree struct ethtool_gfeatures *g_cmd = NULL;
if (VIR_ALLOC_VAR(g_cmd,
struct ethtool_get_features_block, GFEATURES_SIZE) < 0)
@@ -3258,7 +3157,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
@@ -3456,8 +3354,7 @@ int virNetDevSetCoalesce(const char *ifname,
int
virNetDevRunEthernetScript(const char *ifname, const char *script)
{
- virCommandPtr cmd;
- int ret;
+ g_autoptr(virCommand) cmd = NULL;
/* Not a bug! Previously we did accept script="" as a NO-OP. */
if (STREQ(script, ""))
@@ -3471,8 +3368,5 @@ virNetDevRunEthernetScript(const char *ifname, const char *script)
#endif
virCommandAddEnvPassCommon(cmd);
- ret = virCommandRun(cmd, NULL);
-
- virCommandFree(cmd);
- return ret;
+ return virCommandRun(cmd, NULL);
}
--
2.25.3
4:20 a.m.
New subject: [PATCH 3/3] virnetdev.c: Use g_auto*()
On a Monday in 2020, Michal Privoznik wrote:
While I'm at it, use more g_autofree and g_autoptr() in this
file. This also fixes a possible mem-leak in
virNetDevGetVirtualFunctions().
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/util/virnetdev.c | 384 ++++++++++++++++---------------------------
1 file changed, 139 insertions(+), 245 deletions(-)
Reviewed-by: Ján Tomko <jtomko(a)redhat.com>
Jano
1677
days inactive
1677
days old
6 comments
2 participants
participants (2)
-
Ján Tomko -
Michal Privoznik