[libvirt] [PATCH 2/5 v3] Added Function virNetDevGetVirtualFunctions

[Daniel P. Berrange]

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 :|