On Wed, Dec 14, 2011 at 10:50:14AM +0000, Shradha Shah wrote:
> This functions enables us to get the Virtual Functions attached to
> a Physical function given the name of a SR-IOV physical functio.
>
> In order to accomplish the task, added a getter function pciGetDeviceAddrString
> to get the BDF of the Virtual Function in a char array.
> ---
> src/util/pci.c | 23 ++++++++++++++
> src/util/pci.h | 5 +++
> src/util/virnetdev.c | 83 ++++++++++++++++++++++++++++++++++++++++++++++++++
> src/util/virnetdev.h | 6 +++
> 4 files changed, 117 insertions(+), 0 deletions(-)
>
> diff --git a/src/util/pci.c b/src/util/pci.c
> index d0ba4c5..ca05e8f 100644
> --- a/src/util/pci.c
> +++ b/src/util/pci.c
> @@ -1293,6 +1293,29 @@ pciReadDeviceID(pciDevice *dev, const char *id_name)
> return id_str;
> }
>
> +int
> +pciGetDeviceAddrString(unsigned domain,
> + unsigned bus,
> + unsigned slot,
> + unsigned function,
> + char **pciConfigAddr)
> +{
> + pciDevice *dev = NULL;
> + int ret = -1;
> +
> + dev = pciGetDevice(domain,
> + bus,
> + slot,
> + function);
> + if (dev != NULL) {
> + *pciConfigAddr = strdup(dev->name);
We need to raise virReportOOMError() on failure here
> + ret = 0;
> + }
> +
> + pciFreeDevice(dev);
> + return ret;
> +}
> +
There is trailing whitespace on several lines in this method,
and others in this patch
If you run 'make syntax-check' it should tell you of all the
problem areas.
> pciDevice *
> pciGetDevice(unsigned domain,
> unsigned bus,
> diff --git a/src/util/pci.h b/src/util/pci.h
> index f98e745..b7723b1 100644
> --- a/src/util/pci.h
> +++ b/src/util/pci.h
> @@ -111,4 +111,9 @@ int pciDeviceNetName(char *device_link_sysfs_path, char **netname);
>
> int pciSysfsFile(char *pciDeviceName, char **pci_sysfs_device_link);
>
> +int pciGetDeviceAddrString(unsigned domain,
> + unsigned bus,
> + unsigned slot,
> + unsigned function,
> + char **pciConfigAddr);
Can add ATTRIBUTE_RETURN_CHECK to this one too
> diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c
> index fee87ef..3bbb76e 100644
> --- a/src/util/virnetdev.c
> +++ b/src/util/virnetdev.c
> @@ -970,6 +970,78 @@ virNetDevSysfsDeviceFile(char **pf_sysfs_device_link, const char *ifname,
> }
>
> /**
> + * virNetDevGetVirtualFunctions:
> + *
> + * @pfname : name of the physical function interface name
> + * @vfname: array that will hold the interface names of the virtual_functions
> + * @n_vfname: pointer to the number of virtual functions
> + *
> + * Returns 0 on success and -1 on failure
> + */
> +
> +int
> +virNetDevGetVirtualFunctions(const char *pfname,
> + char ***vfname,
> + unsigned int *n_vfname)
> +{
> + int ret = -1, i;
> + char *pf_sysfs_device_link = NULL;
> + char *pci_sysfs_device_link = NULL;
> + struct pci_config_address **virt_fns;
> + char *pciConfigAddr;
> +
> + if (virNetDevSysfsFile(&pf_sysfs_device_link, pfname, "device") < 0)
> + return ret;
> +
> + if (pciGetVirtualFunctions(pf_sysfs_device_link, &virt_fns,
> + n_vfname) < 0) {
> + virReportSystemError(ENOSYS, "%s",
> + _("Failed to get virtual functions"));
You can remove this virReportSystemError call here, because
pciGetVirtualFunctions() will already raise an error on failure
> + goto out;
> + }
> +
> + if (VIR_ALLOC_N(*vfname, *n_vfname) < 0) {
> + virReportOOMError();
> + goto out;
> + }
Since we've allocated '*vfname' at this point, we need to
make sure that we do 'VIR_FREE(vfname)' in any error paths.
> +
> + for (i = 0; i < *n_vfname; i++)
> + {
> + if (pciGetDeviceAddrString(virt_fns[i]->domain,
> + virt_fns[i]->bus,
> + virt_fns[i]->slot,
> + virt_fns[i]->function,
> + &pciConfigAddr) < 0) {
> + virReportSystemError(ENOSYS, "%s",
> + _("Failed to get PCI Config Address String"));
> + goto out;
> + }
> + if (pciSysfsFile(pciConfigAddr, &pci_sysfs_device_link) < 0) {
> + virReportSystemError(ENOSYS, "%s",
> + _("Failed to get PCI SYSFS file"));
> + goto out;
> + }
> +
> + if (pciDeviceNetName(pci_sysfs_device_link, &((*vfname)[i])) < 0) {
> + virReportSystemError(ENOSYS, "%s",
> + _("Failed to get interface name of the VF"));
> + goto out;
> + }
> + }
> +
> + ret = 0;
> +
> +out:
Our convention for goto label naming suggests 'cleanup' as
the name rather than 'out' (yes, I know not all of our code
is in compliance with this, so you might have seen other
cases of 'out' elsewhre).
> + for (i = 0; i < *n_vfname; i++)
> + VIR_FREE(virt_fns[i]);
> + VIR_FREE(virt_fns);
> + VIR_FREE(pf_sysfs_device_link);
> + VIR_FREE(pci_sysfs_device_link);
> + VIR_FREE(pciConfigAddr);
> + return ret;
> +}
> +
Regards,
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list