[libvirt] [PATCH 7/9] nodedev: Fix the improper logic when enumerating SRIOV VF
Osier Yang
jyang at redhat.com
Tue Jan 15 09:55:46 UTC 2013
On 2013年01月15日 03:42, Michal Privoznik wrote:
> 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 |= ...;
Agreed. The function now only returns -1 or 0.
>
> 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.
Okay. This is improved in v2.
>
>> } 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>
Agreed, this is the convention we use in libvirt.
> }
>
> 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