[libvirt] [PATCH v2 1/2] Document virPCIGetPhysicalFunction() and fix its callers

[Laine Stump]

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.