[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH 04/11] util: Add util to find PCI device address by its vendor and product IDs



On 06/07/2013 01:03 PM, Osier Yang wrote:
> As the first user of virTraverseDirectory, it falls through to the 2
> depth from "/sys/devices", and returns the address of the PCI device
> of which both "vendor" and "device" have the specified value.  See the
> test for an example.

Two patches ago we the commit message mentions 'udev' backend and 'hal'
backend styles - is this covering those cases?  Are there "other" styles
that need to be considered?  I suppose as long as the layout never
changes things will work...  assuming this is the only layout that needs
to be considered.

> ---
>  src/libvirt_private.syms                           |   1 +
>  src/util/virutil.c                                 | 150 +++++++++++++++++++++
>  src/util/virutil.h                                 |   5 +
>  tests/sysfs/devices/pci0000:00/0000:00:1f.1/device |   1 +
>  tests/sysfs/devices/pci0000:00/0000:00:1f.1/vendor |   1 +
>  tests/sysfs/devices/pci0000:00/0000:00:1f.2/device |   1 +
>  tests/sysfs/devices/pci0000:00/0000:00:1f.2/vendor |   1 +
>  tests/sysfs/devices/pci0000:00/0000:00:1f.4/device |   1 +
>  tests/sysfs/devices/pci0000:00/0000:00:1f.4/vendor |   1 +
>  tests/utiltest.c                                   |  41 +++++-
>  10 files changed, 201 insertions(+), 2 deletions(-)
>  create mode 100644 tests/sysfs/devices/pci0000:00/0000:00:1f.1/device
>  create mode 100644 tests/sysfs/devices/pci0000:00/0000:00:1f.1/vendor
>  create mode 100644 tests/sysfs/devices/pci0000:00/0000:00:1f.2/device
>  create mode 100644 tests/sysfs/devices/pci0000:00/0000:00:1f.2/vendor
>  create mode 100644 tests/sysfs/devices/pci0000:00/0000:00:1f.4/device
>  create mode 100644 tests/sysfs/devices/pci0000:00/0000:00:1f.4/vendor
> 
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index fe182e8..f6ae42d 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -1932,6 +1932,7 @@ virDoubleToStr;
>  virEnumFromString;
>  virEnumToString;
>  virFindFCHostCapableVport;
> +virFindPCIDeviceByVPD;
>  virFormatIntDecimal;
>  virGetDeviceID;
>  virGetDeviceUnprivSGIO;
> diff --git a/src/util/virutil.c b/src/util/virutil.c
> index c1938f9..8309568 100644
> --- a/src/util/virutil.c
> +++ b/src/util/virutil.c
> @@ -1996,6 +1996,147 @@ cleanup:
>      VIR_FREE(vports);
>      return ret;
>  }
> +
> +struct virFindPCIDeviceByVPDData {
> +     const char *filename;
> +     const char *value;
> +};
> +
> +static int
> +virFindPCIDeviceByVPDCallback(const char *fpath,
> +                              void *opaque)
> +{
> +    struct virFindPCIDeviceByVPDData *data = opaque;
> +    char *p = NULL;
> +    char *buf = NULL;
> +    int ret = -1;
> +
> +    p = strrchr(fpath, '/');
> +    p++;
> +
> +    if (STRNEQ(p, data->filename))
> +        return -1;
> +
> +    if (virFileReadAll(fpath, 1024, &buf) < 0)
> +        return -1;
> +
> +    if ((p = strchr(buf, '\n')))
> +        *p = '\0';
> +
> +    if (STRNEQ(buf, data->value))
> +        goto cleanup;
> +
> +    ret = 0;
> +cleanup:
> +    VIR_FREE(buf);
> +    return ret;
> +}
> +
> +# define SYSFS_DEVICES_PATH "/sys/devices"
> +
> +/**
> + * virFindPCIDeviceByVPD:
> + * @sysfs_prefix: The directory path where starts to traverse, defaults
> + *                to SYSFS_DEVICES_PATH.
> + * @vendor: vendor ID in string
> + * @product: product ID in string
> + *
> + * Traverse specified directory tree (@sysfs_prefix) to find out the PCI
> + * device address (e.g. "0000\:00\:1f.2") by @vendor and @product.
> + *
> + * Return the PCI device address as string on success, or NULL on
> + * failure.
> + */
> +char *
> +virFindPCIDeviceByVPD(const char *sysfs_prefix,
> +                      const char *vendor,
> +                      const char *product)
> +{
> +    const char *prefix = sysfs_prefix ? sysfs_prefix : SYSFS_DEVICES_PATH;
> +    char **vendor_paths = NULL;
> +    int nvendor_paths = -1;
> +    char **product_paths = NULL;
> +    int nproduct_paths = -1;
> +    unsigned int flags = 0;
> +    char *ret = NULL;
> +    bool found = false;
> +    char *p1 = NULL;
> +    char *p2 = NULL;
> +    int i, j;
> +
> +    flags |= (VIR_TRAVERSE_DIRECTORY_IGNORE_HIDDEN_FILES |
> +              VIR_TRAVERSE_DIRECTORY_FALL_THROUGH);
> +
> +    struct virFindPCIDeviceByVPDData vendor_data = {
> +        .filename = "vendor",
> +        .value = vendor,
> +    };
> +
> +    struct virFindPCIDeviceByVPDData product_data = {
> +        .filename = "device",
> +        .value = product,
> +    };
> +
> +    if ((nvendor_paths = virTraverseDirectory(prefix, 2, flags,
> +                                              virFindPCIDeviceByVPDCallback,
> +                                              NULL, &vendor_data,
> +                                              &vendor_paths)) < 0 ||
> +        (nproduct_paths = virTraverseDirectory(prefix, 2, flags,
> +                                               virFindPCIDeviceByVPDCallback,
> +                                               NULL, &product_data,
> +                                               &product_paths)) < 0)
> +        goto cleanup;
> +
> +    if (!nvendor_paths || !nproduct_paths) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR,
> +                       _("Unable to find PCI device with vendor '%s' "
> +                         "product '%s' in '%s'"), vendor, product, prefix);
> +        goto cleanup;
> +    }
> +
> +    for (i = 0; i < nvendor_paths; i++) {
> +        p1 = strrchr(vendor_paths[i], '/');
> +
> +        for (j = 0; j < nproduct_paths; j++) {
> +            p2 = strrchr(product_paths[j], '/');
> +
> +            if ((p1 - vendor_paths[i]) != (p2 - product_paths[j]))
> +                continue;
> +
> +            if (STREQLEN(vendor_paths[i], product_paths[j],
> +                         p1 - vendor_paths[i])) {
> +                found = true;
> +                break;
> +            }
> +        }
> +
> +        if (found)
> +            break;
> +    }
> +
> +    if (found) {
> +        p1 = strrchr(vendor_paths[i], '/');
> +        *p1 = '\0';
> +        p2 = strrchr(vendor_paths[i], '/');
> +
> +        if (VIR_STRDUP(ret, p2 + 1) < 0)
> +            goto cleanup;
> +    }
> +
> +cleanup:
> +    if (nvendor_paths > 0) {
> +        for (i = 0; i < nvendor_paths; i++)
> +            VIR_FREE(vendor_paths[i]);
> +        VIR_FREE(vendor_paths);
> +    }
> +
> +    if (nproduct_paths > 0) {
> +        for (i = 0; i < nproduct_paths; i++)
> +            VIR_FREE(product_paths[i]);
> +        VIR_FREE(product_paths);
> +    }
> +    return ret;
> +}
>  #else
>  int
>  virReadFCHost(const char *sysfs_prefix ATTRIBUTE_UNUSED,
> @@ -2048,6 +2189,15 @@ virFindFCHostCapableVport(const char *sysfs_prefix ATTRIBUTE_UNUSED)
>      virReportSystemError(ENOSYS, "%s", _("Not supported on this platform"));
>      return NULL;
>  }
> +
> +int
> +virFindPCIDeviceByVPD(const char *sysfs_prefix,
> +                      const char *vendor,
> +                      const char *product)
> +{
> +    virReportSystemError(ENOSYS, "%s", _("Not supported on this platform"));
> +    return NULL;
> +}
>  #endif /* __linux__ */
>  
>  /**
> diff --git a/src/util/virutil.h b/src/util/virutil.h
> index 6c46f23..99d3ea2 100644
> --- a/src/util/virutil.h
> +++ b/src/util/virutil.h
> @@ -218,4 +218,9 @@ int virTraverseDirectory(const char *dirpath,
>                           char ***filepaths);
>      ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(3)
>  
> +char *virFindPCIDeviceByVPD(const char *sysfs_prefix,
> +                            const char *vendor,
> +                            const char *product)
> +    ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);

In the code if sysfs_prefix is NULL, then you use a constant path, so I
think you have to change these to 2 & 3 respectively

> +
>  #endif /* __VIR_UTIL_H__ */
> diff --git a/tests/sysfs/devices/pci0000:00/0000:00:1f.1/device b/tests/sysfs/devices/pci0000:00/0000:00:1f.1/device
> new file mode 100644
> index 0000000..9f26c70
> --- /dev/null
> +++ b/tests/sysfs/devices/pci0000:00/0000:00:1f.1/device
> @@ -0,0 +1 @@
> +0x1e04
> diff --git a/tests/sysfs/devices/pci0000:00/0000:00:1f.1/vendor b/tests/sysfs/devices/pci0000:00/0000:00:1f.1/vendor
> new file mode 100644
> index 0000000..aee5132
> --- /dev/null
> +++ b/tests/sysfs/devices/pci0000:00/0000:00:1f.1/vendor
> @@ -0,0 +1 @@
> +0x8087
> diff --git a/tests/sysfs/devices/pci0000:00/0000:00:1f.2/device b/tests/sysfs/devices/pci0000:00/0000:00:1f.2/device
> new file mode 100644
> index 0000000..a0681c4
> --- /dev/null
> +++ b/tests/sysfs/devices/pci0000:00/0000:00:1f.2/device
> @@ -0,0 +1 @@
> +0x1e03
> diff --git a/tests/sysfs/devices/pci0000:00/0000:00:1f.2/vendor b/tests/sysfs/devices/pci0000:00/0000:00:1f.2/vendor
> new file mode 100644
> index 0000000..ce6dc4d
> --- /dev/null
> +++ b/tests/sysfs/devices/pci0000:00/0000:00:1f.2/vendor
> @@ -0,0 +1 @@
> +0x8086
> diff --git a/tests/sysfs/devices/pci0000:00/0000:00:1f.4/device b/tests/sysfs/devices/pci0000:00/0000:00:1f.4/device
> new file mode 100644
> index 0000000..3c7202c
> --- /dev/null
> +++ b/tests/sysfs/devices/pci0000:00/0000:00:1f.4/device
> @@ -0,0 +1 @@
> +0x1e08
> diff --git a/tests/sysfs/devices/pci0000:00/0000:00:1f.4/vendor b/tests/sysfs/devices/pci0000:00/0000:00:1f.4/vendor
> new file mode 100644
> index 0000000..ce6dc4d
> --- /dev/null
> +++ b/tests/sysfs/devices/pci0000:00/0000:00:1f.4/vendor
> @@ -0,0 +1 @@
> +0x8086
> diff --git a/tests/utiltest.c b/tests/utiltest.c
> index 9d18652..8d3dbfa 100644
> --- a/tests/utiltest.c
> +++ b/tests/utiltest.c
> @@ -9,7 +9,12 @@
>  #include "viralloc.h"
>  #include "testutils.h"
>  #include "virutil.h"
> +#include "virstring.h"
> +#include "virfile.h"
>  
> +static char *sysfs_devices_prefix;
> +

Since there's a VIR_FREE() on this below:

static char *sysfs_devices_prefix = NULL;

> +#define TEST_SYSFS_DEVICES_PREFIX sysfs_devices_prefix
>  
>  static void
>  testQuietError(void *userData ATTRIBUTE_UNUSED,
> @@ -150,14 +155,43 @@ testParseVersionString(const void *data ATTRIBUTE_UNUSED)
>      return 0;
>  }
>  
> -
> -
> +static int
> +testFindPCIDeviceByVPD(const void *data ATTRIBUTE_UNUSED)
> +{
> +    char *addr = NULL;
> +    const char *expected_addr = "0000:00:1f.2";
> +    const char *vendor = "0x8086";
> +    const char *device = "0x1e03";
> +    int ret = -1;
> +
> +    if (!(addr = virFindPCIDeviceByVPD(TEST_SYSFS_DEVICES_PREFIX,
> +                                       vendor, device)))
> +        return -1;
> +
> +    if (STRNEQ(addr, expected_addr))
> +        goto cleanup;

Since we're reusing this:

       VIR_FREE(addr);


Seems to be ACK-able with the nits I've noted fixed.

John
> +
> +    if ((addr = virFindPCIDeviceByVPD(TEST_SYSFS_DEVICES_PREFIX,
> +                                      "0x7076", "0x2434")))
> +        goto cleanup;
> +
> +    ret = 0;
> +cleanup:
> +    VIR_FREE(addr);
> +    return ret;
> +}
>  
>  static int
>  mymain(void)
>  {
>      int result = 0;
>  
> +    if (virAsprintf(&sysfs_devices_prefix, "%s/%s", abs_srcdir,
> +                    "sysfs/devices/") < 0) {
> +        result = -1;
> +        goto cleanup;
> +    }
> +
>      virSetErrorFunc(NULL, testQuietError);
>  
>  #define DO_TEST(_name)                                                  \
> @@ -171,7 +205,10 @@ mymain(void)
>      DO_TEST(IndexToDiskName);
>      DO_TEST(DiskNameToIndex);
>      DO_TEST(ParseVersionString);
> +    DO_TEST(FindPCIDeviceByVPD);
>  
> +cleanup:
> +    VIR_FREE(sysfs_devices_prefix);
>      return result == 0 ? EXIT_SUCCESS : EXIT_FAILURE;
>  }
>  
> 


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]