[libvirt] [PATCH v2 1/2] Document virPCIGetPhysicalFunction() and fix its callers
Laine Stump
laine at laine.org
Tue May 24 15:22:47 UTC 2016
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.
More information about the libvir-list
mailing list