On 15.01.2013 10:56, 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 ret = pciGetVirtualFunctions(syspath,
> &data->pci_dev.virtual_functions,
> &data->pci_dev.num_virtual_functions);
>
> if (ret < 0 )
> goto out;
> if (data->pci_dev.num_virtual_functions > 0)
> data->pci_dev.flags |= VIR_NODE_DEV_CAP_FLAG_PCI_VIRTUAL_FUNCTION;
> ---
> src/node_device/node_device_hal.c | 12 +++++++--
> src/node_device/node_device_udev.c | 11 ++++++--
> src/util/virpci.c | 46 +++++++++++++++++++++---------------
> 3 files changed, 44 insertions(+), 25 deletions(-)
>
> diff --git a/src/node_device/node_device_hal.c b/src/node_device/node_device_hal.c
> index e64d6ac..6da5873 100644
> --- a/src/node_device/node_device_hal.c
> +++ b/src/node_device/node_device_hal.c
> @@ -151,9 +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;
> + }
> +
> + if (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 62f6320..a90217d 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..ee4b3a8 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,45 +1990,52 @@ 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();
> - goto out;
> + goto error;
> }
>
> 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;
> - } else {
> - (*num_virtual_functions)++;
> + continue;
> }
> +
> + if (VIR_ALLOC_N(*virtual_functions,
> + *num_virtual_functions + 1) < 0) {
> + virReportOOMError();
> + VIR_FREE(config_addr);
> + goto error;
> + }
> +
> + (*virtual_functions)[*num_virtual_functions] = config_addr;
> + (*num_virtual_functions)++;
> VIR_FREE(device_link);
> }
> }
>
> ret = 0;
> -
> -out:
> +cleanup:
> + VIR_FREE(device_link);
> if (dir)
> closedir(dir);
> -
> return ret;
> +
> +error:
> + if (*virtual_functions) {
> + for (i = 0; i < *num_virtual_functions; i++)
> + VIR_FREE((*virtual_functions)[i]);
> + VIR_FREE(*virtual_functions);
> + }
> + goto cleanup;
> }
>
> /*
>
ACK to this and te follow up *_udev.c patch squashed in.
Michal
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list