[libvirt] [PATCH 7/9] nodedev: Fix the improper logic when enumerating SRIOV VF
Michal Privoznik
mprivozn at redhat.com
Mon Jan 14 19:42:22 UTC 2013
On 14.01.2013 15:34, Osier Yang wrote:
> pciGetVirtualFunctions returns 0 even if there is no "virtfn"
> entry under the device sysfs path.
>
> And pciGetVirtualFunctions returns -1 when it fails to get
> the PCI config space of one VF, however, with keeping the
> the VFs already detected.
>
> That's why udevProcessPCI and gather_pci_cap use logic like:
>
> if (!pciGetVirtualFunctions(syspath,
> &data->pci_dev.virtual_functions,
> &data->pci_dev.num_virtual_functions) ||
> data->pci_dev.num_virtual_functions > 0)
> data->pci_dev.flags |= VIR_NODE_DEV_CAP_FLAG_PCI_VIRTUAL_FUNCTION;
>
> to tag the PCI device with "virtual_function" cap.
>
> However, this results in a VF will aslo get "virtual_function" cap.
>
> This patch fixes it by:
> * Ignoring the VF which has failure of getting PCI config space
> (given that the successfully detected VFs are kept , it makes
> sense to not give up on the failure of one VF too) with a warning,
> so pciGetVirtualFunctions will not return -1 except out of memory.
>
> * Free the allocated *virtual_functions when out of memory
>
> And thus the logic can be changed to:
>
> /* Out of memory */
> int rc = pciGetVirtualFunctions(syspath,
> &data->pci_dev.virtual_functions,
> &data->pci_dev.num_virtual_functions);
s/int rc/int ret/
>
> if (ret < 0 )
> goto out;
> else if (!ret && (data->pci_dev.num_virtual_functions > 0))
> data->pci_dev.flags |= VIR_NODE_DEV_CAP_FLAG_PCI_VIRTUAL_FUNCTION;
Hm.. what about the second condition being:
else if (data->pci_dev.num_virtual_functions > 0)
data->pci_dev.flags |= ...;
My rationale is - the first 'if' catches all the errors, so we don't
have to check 'ret' again for success. We already know it succeeded
because we are taking the 'else' branch. Having said that, what about
going one step further?
int ret = pciGetVirtualFunctions(...);
if (ret < 0)
goto error;
if (data->pci_dev.num_virtual_functions)
data->pci_dev.flags |= ...;
That is - we can leave the 'else' out since we are doing 'goto'.
Likewise, '> 0' can be left out because pciGetVirtualFunctions() sets
nonzero value there.
> ---
> src/node_device/node_device_hal.c | 11 ++++++++---
> src/node_device/node_device_udev.c | 11 ++++++++---
> src/util/virpci.c | 36 +++++++++++++++++++++++-------------
> 3 files changed, 39 insertions(+), 19 deletions(-)
>
> diff --git a/src/node_device/node_device_hal.c b/src/node_device/node_device_hal.c
> index 1afa21c..d0c419d 100644
> --- a/src/node_device/node_device_hal.c
> +++ b/src/node_device/node_device_hal.c
> @@ -151,10 +151,15 @@ static int gather_pci_cap(LibHalContext *ctx, const char *udi,
> if (!pciGetPhysicalFunction(sysfs_path, &d->pci_dev.physical_function))
> d->pci_dev.flags |= VIR_NODE_DEV_CAP_FLAG_PCI_PHYSICAL_FUNCTION;
>
> - if (!pciGetVirtualFunctions(sysfs_path, &d->pci_dev.virtual_functions,
> - &d->pci_dev.num_virtual_functions) ||
> - d->pci_dev.num_virtual_functions > 0)
> + int ret = pciGetVirtualFunctions(sysfs_path,
> + &d->pci_dev.virtual_functions,
> + &d->pci_dev.num_virtual_functions);
> + if (ret < 0) {
> + VIR_FREE(sysfs_path);
> + return -1;
> + } else if (!ret && (d->pci_dev.num_virtual_functions > 0)) {
> d->pci_dev.flags |= VIR_NODE_DEV_CAP_FLAG_PCI_VIRTUAL_FUNCTION;
> + }
>
> VIR_FREE(sysfs_path);
> }
> diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c
> index 87f1000..47ac4f4 100644
> --- a/src/node_device/node_device_udev.c
> +++ b/src/node_device/node_device_udev.c
> @@ -416,6 +416,7 @@ static int udevProcessPCI(struct udev_device *device,
> union _virNodeDevCapData *data = &def->caps->data;
> int ret = -1;
> char *p;
> + int rc;
>
> syspath = udev_device_get_syspath(device);
>
> @@ -484,9 +485,13 @@ static int udevProcessPCI(struct udev_device *device,
> if (!pciGetPhysicalFunction(syspath, &data->pci_dev.physical_function))
> data->pci_dev.flags |= VIR_NODE_DEV_CAP_FLAG_PCI_PHYSICAL_FUNCTION;
>
> - if (!pciGetVirtualFunctions(syspath, &data->pci_dev.virtual_functions,
> - &data->pci_dev.num_virtual_functions) ||
> - data->pci_dev.num_virtual_functions > 0)
> + rc = pciGetVirtualFunctions(syspath,
> + &data->pci_dev.virtual_functions,
> + &data->pci_dev.num_virtual_functions);
> + /* Out of memory */
> + if (rc < 0)
> + goto out;
> + else if (!rc && (data->pci_dev.num_virtual_functions > 0))
> data->pci_dev.flags |= VIR_NODE_DEV_CAP_FLAG_PCI_VIRTUAL_FUNCTION;
>
> ret = 0;
> diff --git a/src/util/virpci.c b/src/util/virpci.c
> index 0fb9923..9f4f3c7 100644
> --- a/src/util/virpci.c
> +++ b/src/util/virpci.c
> @@ -1968,6 +1968,7 @@ pciGetVirtualFunctions(const char *sysfs_path,
> unsigned int *num_virtual_functions)
> {
> int ret = -1;
> + int i;
> DIR *dir = NULL;
> struct dirent *entry = NULL;
> char *device_link = NULL;
> @@ -1989,6 +1990,7 @@ pciGetVirtualFunctions(const char *sysfs_path,
> *num_virtual_functions = 0;
> while ((entry = readdir(dir))) {
> if (STRPREFIX(entry->d_name, "virtfn")) {
> + struct pci_config_address *config_addr = NULL;
>
> if (virBuildPath(&device_link, sysfs_path, entry->d_name) == -1) {
> virReportOOMError();
> @@ -1997,24 +1999,23 @@ pciGetVirtualFunctions(const char *sysfs_path,
>
> VIR_DEBUG("Number of virtual functions: %d",
> *num_virtual_functions);
> - if (VIR_REALLOC_N(*virtual_functions,
> - (*num_virtual_functions) + 1) != 0) {
> - virReportOOMError();
> - VIR_FREE(device_link);
> - goto out;
> - }
>
> if (pciGetPciConfigAddressFromSysfsDeviceLink(device_link,
> - &((*virtual_functions)[*num_virtual_functions])) !=
> + &config_addr) !=
> SRIOV_FOUND) {
> - /* We should not get back SRIOV_NOT_FOUND in this
> - * case, so if we do, it's an error. */
> - virReportError(VIR_ERR_INTERNAL_ERROR,
> - _("Failed to get SR IOV function from device "
> - "link '%s'"), device_link);
> + VIR_WARN("Failed to get SRIOV function from device "
> + "link '%s'", device_link);
> VIR_FREE(device_link);
> - goto out;
> + continue;
This 'continue' causes immediate jump to the beginning of the next
iteration, so the 'else' is a bit overkill. But I can live with that.
> } else {
> + if (VIR_ALLOC_N(*virtual_functions,
> + *num_virtual_functions + 1) < 0) {
> + virReportOOMError();
> + VIR_FREE(config_addr);
> + goto out;
> + }
> +
> + (*virtual_functions)[*num_virtual_functions] = config_addr;
> (*num_virtual_functions)++;
> }
> VIR_FREE(device_link);
> @@ -2022,8 +2023,17 @@ pciGetVirtualFunctions(const char *sysfs_path,
> }
>
> ret = 0;
> + goto cleanup;
>
> out:
> + if (*virtual_functions) {
> + for (i = 0; i < *num_virtual_functions; i++)
> + VIR_FREE((*virtual_functions)[i]);
> + VIR_FREE(*virtual_functions);
> + }
> +
> +cleanup:
> + VIR_FREE(device_link);
> if (dir)
> closedir(dir);
>
>
Re: these 'out' and 'cleanup' labels. I think the preferred way is:
...(some code)...
ret = 0;
cleanup:
VIR_FREE(some_var);
return ret;
error:
VIR_FREE(other_var);
<either return val < 0 or goto cleanup>
}
So if you can spare us the 'out' label which is there a while, that'd be
great.
Michal
More information about the libvir-list
mailing list