8:44 p.m.
On 19/06/13 23:32, John Ferlan wrote:
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.
No, the util will use pci address like "0000:00:1f:2", which is translated
from either udev backend style (e.g. pci_0000_00_1f_2), or HAL backend
style (e.g. pci_8086_01e3). Later patch do the translation. The utils are
just designed general enough to be able to work with each of the parent
styles.
I should clarify it more in commit log though.
> ---
> 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.
The value returned from virTraverseDirectory is either <=0 or > 0, note that
there are checkings for npaths < 0 and npaths ==0 right before. So I don't
see any confusing here.
> + 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...
I will prefer to keep it here, the reason for changing it is we have two
different
sysfs directories after the new test testParseStableScsiHostAddress is
introduced, which is straightforward than changing it in 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;
> }
>
>