On 05/24/2016 07:11 AM, Andrea Bolognani wrote:
Commit c8b1a83605e4 changed the function, making it
impossible for callers to be able to tell whether a
non-negative return value means "physical function
address found and parsed correctly" or "couldn't find
corresponding physical function".
The important difference between the two being that,
in the latter case, the returned pointer is NULL and
should never, ever be dereferenced.
In order to cope with these changes, the callers
have to be updated. Some documentation has also been
added to the function, so that callers know how to handle
the return value and returned data properly.
---
src/node_device/node_device_linux_sysfs.c | 10 ++++++++--
src/util/virpci.c | 18 ++++++++++++++++--
2 files changed, 24 insertions(+), 4 deletions(-)
diff --git a/src/node_device/node_device_linux_sysfs.c b/src/node_device/node_device_linux_sysfs.c
index 24a6a2e..b337d2d 100644
--- a/src/node_device/node_device_linux_sysfs.c
+++ b/src/node_device/node_device_linux_sysfs.c
@@ -154,19 +154,25 @@ nodeDeviceSysfsGetPCISRIOVCaps(const char *sysfsPath,
data->pci_dev.flags &= ~VIR_NODE_DEV_CAP_FLAG_PCI_VIRTUAL_FUNCTION;
data->pci_dev.flags &= ~VIR_NODE_DEV_CAP_FLAG_PCI_PHYSICAL_FUNCTION;
- if (!virPCIGetPhysicalFunction(sysfsPath, &data->pci_dev.physical_function))
+ ret = virPCIGetPhysicalFunction(sysfsPath,
+ &data->pci_dev.physical_function);
+ if (ret < 0)
+ goto out;
I avoid adding "out:" labels, and use "cleanup:" instead, even if there
currently isn't any cleanup done there (and if there isn't any cleanup
to be done, why goto anyway?)
+
+ if (data->pci_dev.physical_function)
data->pci_dev.flags |= VIR_NODE_DEV_CAP_FLAG_PCI_PHYSICAL_FUNCTION;
ret = virPCIGetVirtualFunctions(sysfsPath, &data->pci_dev.virtual_functions,
&data->pci_dev.num_virtual_functions,
&data->pci_dev.max_virtual_functions);
if (ret < 0)
- return ret;
+ goto out;
if (data->pci_dev.num_virtual_functions > 0 ||
data->pci_dev.max_virtual_functions > 0)
data->pci_dev.flags |= VIR_NODE_DEV_CAP_FLAG_PCI_VIRTUAL_FUNCTION;
+ out:
return ret;
}
diff --git a/src/util/virpci.c b/src/util/virpci.c
index 3d18bb3..e8dc987 100644
--- a/src/util/virpci.c
+++ b/src/util/virpci.c
@@ -2478,8 +2478,19 @@ virPCIGetDeviceAddressFromSysfsLink(const char *device_link)
return bdf;
}
-/*
- * Returns Physical function given a virtual function
+/**
+ * virPCIGetPhysicalFunction:
+ * @vf_sysfs_path: sysfs path for the virtual function
+ * @pf: where to store the physical function's address
+ *
+ * Given @vf_sysfs_path, this function will store the pointer
+ * to a newly-allocated virPCIDeviceAddress in @pf.
+ *
+ * @pf might be NULL if @vf_sysfs_path does not point to a
+ * virtual function. If it's not NULL, then it should be
+ * freed by the caller when no longer needed.
+ *
+ * Returns: >=0 on success, <0 on failure
As long as you're here, just to be safe, how about having
virPCIGetPhysicalFunction set "*pf = NULL" at the beginning so that it's
NULL even if there is an OOM error and the caller hasn't initialized it?
(Yeah, I know that will "never ever" happen. But as long as we're being
boy scouts, we might as well do it up right :-))
*/
int
virPCIGetPhysicalFunction(const char *vf_sysfs_path,
@@ -2719,6 +2730,9 @@ virPCIGetVirtualFunctionInfo(const char *vf_sysfs_device_path,
if (virPCIGetPhysicalFunction(vf_sysfs_device_path, &pf_config_address) < 0)
return ret;
+ if (!pf_config_address)
+ return ret;
+
if (virPCIDeviceAddressGetSysfsFile(pf_config_address,
&pf_sysfs_device_path) < 0) {
ACK.
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list