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

Re: [libvirt] [PATCH 05/11] util: Add util to parse the stable scsi host address



On 06/07/2013 01:03 PM, Osier Yang wrote:
> By traversing sysfs directory like "/sys/bus/pci/devices/0000:00:1f:2/"
> to find out the scsi host whose "unique_id" has the specified value.
> And returns the host number.
> 
> Address like "0000:00:1f:2" will be retrieved from the "parent" of
> scsi_host adapter. E.g.
> 
>   <adapter type='scsi_host' parent='pci_0000_00_1f_2' unique_id='2'/>

Like my comment in 4/11 this code certainly assumes a specific directory
path.

> ---
>  src/libvirt_private.syms                           |   1 +
>  src/util/virutil.c                                 | 128 +++++++++++++++++++++
>  src/util/virutil.h                                 |   6 +
>  .../ata1/host0/scsi_host/host0/unique_id           |   1 +
>  .../ata2/host1/scsi_host/host1/unique_id           |   1 +
>  tests/utiltest.c                                   |  65 +++++++++--
>  6 files changed, 192 insertions(+), 10 deletions(-)
>  create mode 100644 tests/sysfs/bus/pci/devices/0000:00:1f.2/ata1/host0/scsi_host/host0/unique_id
>  create mode 100644 tests/sysfs/bus/pci/devices/0000:00:1f.2/ata2/host1/scsi_host/host1/unique_id
> 
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index f6ae42d..ce39cc6 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -1954,6 +1954,7 @@ virIsCapableVport;
>  virIsDevMapperDevice;
>  virManageVport;
>  virParseNumber;
> +virParseStableScsiHostAddress;
>  virParseVersionString;
>  virPipeReadUntilEOF;
>  virReadFCHost;
> diff --git a/src/util/virutil.c b/src/util/virutil.c
> index 8309568..a80574f 100644
> --- a/src/util/virutil.c
> +++ b/src/util/virutil.c
> @@ -2137,6 +2137,125 @@ cleanup:
>      }
>      return ret;
>  }
> +
> +struct virParseStableScsiHostAddressData {
> +    const char *filename;
> +    unsigned int unique_id;
> +};
> +
> +static int
> +virParseStableScsiHostAddressCallback(const char *fpath,
> +                                      void *opaque)
> +{
> +    struct virParseStableScsiHostAddressData *data  = opaque;
> +    unsigned int unique_id;
> +    char *buf = NULL;
> +    char *p;
> +    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 (virStrToLong_ui(buf, NULL, 10, &unique_id) < 0)
> +        goto cleanup;
> +
> +    if (unique_id != data->unique_id)
> +        goto cleanup;
> +
> +    ret = 0;
> +cleanup:
> +    VIR_FREE(buf);
> +    return ret;
> +}
> +
> +# define SYSFS_BUS_PCI_DEVICES "/sys/bus/pci/devices"
> +
> +/**
> + * virParseStableScsiHostAddress:
> + * @sysfs_prefix: The directory path where starts to traverse, defaults
> + *                to SYSFS_BUS_PCI_DEVICES.
> + * @addr: The parent's PCI address
> + * @unique_id: The value of sysfs "unique_id" of the scsi host.
> + *
> + * Returns the host name (e.g. host10) of the scsi host whose parent
> + * has address @addr, and "unique_id" has value @unique_id, on success.
> + * Or NULL on failure.
> + */
> +char *
> +virParseStableScsiHostAddress(const char *sysfs_prefix,
> +                              const char *address,
> +                              unsigned int unique_id)
> +{
> +    const char *prefix = sysfs_prefix ? sysfs_prefix : SYSFS_BUS_PCI_DEVICES;
> +    unsigned int flags = 0;
> +    char **paths = NULL;
> +    int npaths = 0;
> +    char *dir = NULL;
> +    char *p1 = NULL;
> +    char *p2 = NULL;
> +    char *ret = NULL;
> +    int i;
> +
> +    struct virParseStableScsiHostAddressData data = {
> +        .filename = "unique_id",
> +        .unique_id = unique_id,
> +    };
> +
> +    if (virAsprintf(&dir, "%s/%s", prefix, address) < 0) {
> +        virReportOOMError();
> +        return NULL;
> +    }
> +
> +    flags |= (VIR_TRAVERSE_DIRECTORY_IGNORE_HIDDEN_FILES |
> +              VIR_TRAVERSE_DIRECTORY_FALL_THROUGH);
> +
> +    if ((npaths = virTraverseDirectory(dir, 4, flags,
> +                                       virParseStableScsiHostAddressCallback,
> +                                       NULL, &data, &paths)) < 0) {
> +        goto cleanup;
> +    } else if (npaths == 0) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR,
> +                       _("unable to find scsi host with PCI address "
> +                         "'%s' unique_id '%u'"), address, unique_id);
> +        goto cleanup;
> +    } else if (npaths != 1) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR,
> +                       _("unexpected multiple paths returned for PCI address "
> +                         "'%s' unique_id '%u'"), address, unique_id);

Is this the right error? Will it be confusing with errors from
virTraverseDirectory()?  I don't think it's multiple paths being returned.

> +        goto cleanup;
> +    }
> +
> +    if (!(p1 = strstr(paths[0], "scsi_host"))) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR,
> +                       _("unexpected returned path '%s' for PCI address "
> +                         "'%s' unique_id '%u'"), paths[0], address, unique_id);
> +        goto cleanup;
> +    }
> +
> +    p1 += strlen("scsi_host");
> +    p2 = strrchr(p1, '/');
> +
> +    if (VIR_STRNDUP(ret, p1 + 1, p2 - p1 -1) < 0)
> +        goto cleanup;
> +
> +cleanup:
> +    VIR_FREE(dir);
> +    if (npaths > 0) {
> +        for (i = 0; i < npaths; i++)
> +            VIR_FREE(paths[i]);
> +        VIR_FREE(paths);
> +    }
> +    return ret;
> +}
>  #else
>  int
>  virReadFCHost(const char *sysfs_prefix ATTRIBUTE_UNUSED,
> @@ -2198,6 +2317,15 @@ virFindPCIDeviceByVPD(const char *sysfs_prefix,
>      virReportSystemError(ENOSYS, "%s", _("Not supported on this platform"));
>      return NULL;
>  }
> +
> +char *
> +virParseStableScsiHostAddress(const char *sysfs_prefix,
> +                              const char *address,
> +                              unsigned int unique_id)
> +{
> +    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 99d3ea2..2747cf1 100644
> --- a/src/util/virutil.h
> +++ b/src/util/virutil.h
> @@ -223,4 +223,10 @@ char *virFindPCIDeviceByVPD(const char *sysfs_prefix,
>                              const char *product)
>      ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
>  
> +char *virParseStableScsiHostAddress(const char *sysfs_prefix,
> +                                    const char *address,
> +                                    unsigned int unique_id)
> +
> +    ATTRIBUTE_NONNULL(2);
> +
>  #endif /* __VIR_UTIL_H__ */
> diff --git a/tests/sysfs/bus/pci/devices/0000:00:1f.2/ata1/host0/scsi_host/host0/unique_id b/tests/sysfs/bus/pci/devices/0000:00:1f.2/ata1/host0/scsi_host/host0/unique_id
> new file mode 100644
> index 0000000..d00491f
> --- /dev/null
> +++ b/tests/sysfs/bus/pci/devices/0000:00:1f.2/ata1/host0/scsi_host/host0/unique_id
> @@ -0,0 +1 @@
> +1
> diff --git a/tests/sysfs/bus/pci/devices/0000:00:1f.2/ata2/host1/scsi_host/host1/unique_id b/tests/sysfs/bus/pci/devices/0000:00:1f.2/ata2/host1/scsi_host/host1/unique_id
> new file mode 100644
> index 0000000..0cfbf08
> --- /dev/null
> +++ b/tests/sysfs/bus/pci/devices/0000:00:1f.2/ata2/host1/scsi_host/host1/unique_id
> @@ -0,0 +1 @@
> +2
> diff --git a/tests/utiltest.c b/tests/utiltest.c
> index 8d3dbfa..41fdd7e 100644
> --- a/tests/utiltest.c
> +++ b/tests/utiltest.c
> @@ -11,10 +11,12 @@
>  #include "virutil.h"
>  #include "virstring.h"
>  #include "virfile.h"
> +#include "virerror.h"
>  
> -static char *sysfs_devices_prefix;
> +static char *test_sysfs;

Perhaps should have been part of 4/11...

>  
> -#define TEST_SYSFS_DEVICES_PREFIX sysfs_devices_prefix
> +#define VIR_FROM_THIS VIR_FROM_NONE
> +#define TEST_SYSFS test_sysfs
>  
>  static void
>  testQuietError(void *userData ATTRIBUTE_UNUSED,
> @@ -158,36 +160,78 @@ testParseVersionString(const void *data ATTRIBUTE_UNUSED)
>  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";
> +    char *dir = NULL;
> +    char *addr = NULL;
>      int ret = -1;
>  
> -    if (!(addr = virFindPCIDeviceByVPD(TEST_SYSFS_DEVICES_PREFIX,
> -                                       vendor, device)))
> +    if (virAsprintf(&dir, "%s/%s", TEST_SYSFS, "devices") < 0) {
> +        virReportOOMError();
>          return -1;
> +    }
> +
> +    if (!(addr = virFindPCIDeviceByVPD(dir, vendor, device)))
> +        goto cleanup;
>  
>      if (STRNEQ(addr, expected_addr))
>          goto cleanup;
>  
> -    if ((addr = virFindPCIDeviceByVPD(TEST_SYSFS_DEVICES_PREFIX,
> -                                      "0x7076", "0x2434")))
> +    if ((addr = virFindPCIDeviceByVPD(dir, "0x7076", "0x2434")))
>          goto cleanup;
>  
>      ret = 0;
>  cleanup:
> +    VIR_FREE(dir);
>      VIR_FREE(addr);
>      return ret;
>  }

It seems this part of the change should have been squashed into 4/11 as
it's not related to this particular set of changes.... That includes the
test_sysfs modification...

Seems to be ACKable with nits resolved.

John
>  
>  static int
> +testParseStableScsiHostAddress(const void *data ATTRIBUTE_UNUSED)
> +{
> +    const char *addr = "0000:00:1f.2";
> +    unsigned int host0_unique_id = 1;
> +    unsigned int host1_unique_id = 2;
> +    char *rc0 = NULL;
> +    char *rc1 = NULL;
> +    char *dir = NULL;
> +    int ret = -1;
> +
> +    if (virAsprintf(&dir, "%s/%s", TEST_SYSFS, "bus/pci/devices") < 0) {
> +        virReportOOMError();
> +        return -1;
> +    }
> +
> +    if (!(rc0 = virParseStableScsiHostAddress(dir, addr,
> +                                              host0_unique_id)))
> +        goto cleanup;
> +
> +    if (STRNEQ(rc0, "host0"))
> +        goto cleanup;
> +
> +    if (!(rc1 = virParseStableScsiHostAddress(dir, addr,
> +                                              host1_unique_id)))
> +        goto cleanup;
> +
> +    if (STRNEQ(rc1, "host1"))
> +         goto cleanup;
> +    ret = 0;
> +cleanup:
> +    VIR_FREE(dir);
> +    VIR_FREE(rc0);
> +    VIR_FREE(rc1);
> +    return ret;
> +}
> +
> +static int
>  mymain(void)
>  {
>      int result = 0;
>  
> -    if (virAsprintf(&sysfs_devices_prefix, "%s/%s", abs_srcdir,
> -                    "sysfs/devices/") < 0) {
> +    if (virAsprintf(&test_sysfs, "%s/%s", abs_srcdir, "sysfs/") < 0) {
> +        virReportOOMError();
>          result = -1;
>          goto cleanup;
>      }
> @@ -206,9 +250,10 @@ mymain(void)
>      DO_TEST(DiskNameToIndex);
>      DO_TEST(ParseVersionString);
>      DO_TEST(FindPCIDeviceByVPD);
> +    DO_TEST(ParseStableScsiHostAddress);
>  
>  cleanup:
> -    VIR_FREE(sysfs_devices_prefix);
> +    VIR_FREE(test_sysfs);
>      return result == 0 ? EXIT_SUCCESS : EXIT_FAILURE;
>  }
>  
> 


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