On 06/07/2013 01:03 PM, Osier Yang wrote: Consider: The SCSI host number is not a static value that should be used as a unique identifier. The value can change based on SCSI kernel module reloads or after a system reboot. This patch will introduce a mechanism to uniquely identify the SCSI host based upon the parent and unique_id attributes generated as part of the kernel file system path. The new XML format will be: s/scsi/SCSI The "parent" and "name" attributes are mutually exclusive to identify the SCSI host number. Use of the "parent" attribute will be the preferred mechanism. Oh joy. I see no change to docs/formatstorage.html - the examples there need to be updated and the descriptions of fields/attributes need to be changed. Of real importance would be that for 'scsi_host' one must choose to use either "name" or "parent" and "unique_id" to describe things; however, it seems that use of "parent" is preferred since it provides a better way to uniquely identify things after reboots or SCSI kernel module reloads. I would expect to see an example of the scsi_host entry and perhaps some details about how someone could "generate" the parent name based on the expectations of the code. That is the parent name seems to have a fairly specific format - that format is derived using a udev or hal backend style (I don't know what those are, btw). So as a consumer of this code - where would I look for each style and how would I generate the parent value in order to abide by the rules that the code will use. [1] If I read the .rng file correctly, I think you only care about the 'unique_id' if "parent" is found. If someone supplies "name" and 'unique_id' that appears to be legal to this code, although illogical - that is: <adapter type='scsi_host' name="host0" unique_id='5'> So either you keep this code as is or add a check below after the if "!name && !parent" and "name && parent" for "if name and has_unique_id, then illegal xml In the grand scheme of things - this is irrelevant. It seems you should just be checking if it's provided regardless of validity (there's no validation of wwnn, wwpn, and parent below). That is, no need for 'rc' and if the return of virXPathUInt is successful, eg, == 0, then has_unique_id = true. s/specified/is specified s/for/for the/ The error message should read "Both 'name' and 'parent' cannot be specified for the 'scsi_host' adapter." [1] See note from above regarding 'name' and 'unique_id' being set. Why no check for "has_unique_id" being set before printing? If not provided on, this would result in a unique_id=0, which probably is not right... John

There is POSIX calls to walk through direcotry tree, nftw(3), but there is no way to allow one to pass user data to the callback: int nftw(const char *dirpath, int (*fn) (const char *fpath, const struct stat *sb, int typeflag, struct FTW *ftwbuf), int nopenfd, int flags); Assuming a simple requirement: one wants to simply find out all the files which have name "vendor" and it has a specific value, under a directory, It's not possible to achieve with nftw(3). To solve the requirements like this, this new util function comes. int virTraverseDirectory(const char *dirpath, int depth, unsigned int flags, virTraverseDirectoryCallback cb, virTraverseDirectorySkipCallback skipcb, void *opaque, char ***filepaths); * Which allow to traverse a directory limited to specified @depth (relative to the @dirpath) * Two callbacks are provided, callback @cb is to handle the file path passed to it, with the user data @opaque; callback @skipcb is to allow one to make the rules to skip the passed file path. For example, one can define regex patterns in @skipcb to skip all the file path which matches it. * @cb should return 0 to indicate the file path is successfully handled, otherwise -1 should be returned. * Like @cb, @skipcb also should return 0 to indiate the file path will be skipped to handle, otherwise -1 should be returned. * If passed @filepath is not NULL, it will be filled with the file paths which are successfully handled by @cb. * @flags can be used to control the general behaviour of the traversing. E.g. Don't follow symbol links. A simple example: /* * utiltest.c: Prints all the file paths whose basename is * "vendor" under directory "./testdir". */ static int traverseCallback(const char *path, void *opaque) { const char *name = opaque; char *p = NULL; if ((p = strrchr(path, '/'))) p++; if (STRNEQ(p, name)) return -1; return 0; } static int findVendor(void) { char **filepaths = NULL; unsigned int flags = 0; const char *name = "vendor"; int n; int i; flags |= VIR_TRAVERSE_DIRECTORY_FALL_THROUGH | VIR_TRAVERSE_DIRECTORY_IGNORE_HIDDEN_FILES ; if ((n = virTraverseDirectory("./testdir", 2, flags, traverseCallback, NULL, (void *)name, &filepaths)) < 0) return -1; for (i = 0; i < n; i++) printf("Match%d: %s\n", i, filepaths[i]); for (i = 0; i < n; i++) VIR_FREE(filepaths[i]); VIR_FREE(filepaths); return 0; } Tree view of "./testdir": % tree -a ./testdir/ ./testdir/ |-- device |-- dir1 | |-- device | |-- dir2 | | |-- device | | |-- dir3 | | | `-- vendor | | `-- vendor | |-- .dir7 | | `-- vendor | |-- dir8 -> ../dir4 | `-- vendor |-- dir4 | |-- device | |-- dir5 | | |-- device | | `-- vendor | `-- vendor `-- vendor 7 directories, 12 files % ./utiltest Match0: ./testdir/dir1/dir2/vendor Match1: ./testdir/dir4/dir5/vendor Only "vendor" in second depth are returned (flag *_FALL_THROUGH took effect), and the symbol link is not followed. "virTraverseDirectory" is implemented using recursion method, which is generally not good for performance and memory use, and even may eats up the file descriptors. But since we allow to specify the tree "depth", and I don't think libvirt has use cases which need to traverse a very deep directory tree. And on the other hand, Implementing it using either iteration or stack will be much less readable. Later patches will take use of virTraverseDirectory, and tests will be added. --- src/libvirt_private.syms | 1 + src/util/virutil.c | 151 ++++++++++++++++++++++++++++++++++++++++++++++- src/util/virutil.h | 52 ++++++++++++++++ 3 files changed, 203 insertions(+), 1 deletion(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 1ea7467..fe182e8 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1965,6 +1965,7 @@ virSetNonBlock; virSetUIDGID; virSetUIDGIDWithCaps; virStrIsPrint; +virTraverseDirectory; virValidateWWN; diff --git a/src/util/virutil.c b/src/util/virutil.c index c5246bc..c1938f9 100644 --- a/src/util/virutil.c +++ b/src/util/virutil.c @@ -2048,7 +2048,6 @@ virFindFCHostCapableVport(const char *sysfs_prefix ATTRIBUTE_UNUSED) virReportSystemError(ENOSYS, "%s", _("Not supported on this platform")); return NULL; } - #endif /* __linux__ */ /** @@ -2070,3 +2069,153 @@ virCompareLimitUlong(unsigned long long a, unsigned long b) return -1; } + +/* + * virTraverseDirectory: + * @dirpath: Directory path, (relative or absolute) + * @depth: The max directory tree depth for traversing + * @cb: Callback to handle file (not directory) during traversing + * @skibcb: Callback to define rules to skip entry + * @opaque: User data to pass to @cb + * @filepaths: Pointer to array to store the file paths, the file + * path passed to @cb is stored into @filepaths as long + * as @cb returns 0. + * + * Traverse a directory tree into specified @depth, handing the file + * with callback in the process. Caller must free @filepaths and + * its elements after use. + * + * Returns the number of file paths successfully handled by @cb on + * success, with @fillpaths (if passed @fillpath is not NULL) filled, + * or -1 on failure. + */ +int +virTraverseDirectory(const char *dirpath, + int depth, + unsigned int flags, + virTraverseDirectoryCallback cb, + virTraverseDirectorySkipCallback skipcb, + void *opaque, + char ***filepaths) +{ + struct dirent *entry = NULL; + DIR *dir = NULL; + char *fpath = NULL; + struct stat st; + int nfilepaths = 0; + int ret = -1; + int i; + + if (depth < 0) + return 0; + + if (filepaths) + *filepaths = NULL; + + if (!(dir = opendir(dirpath))) { + virReportSystemError(errno, + _("Failed to open directory '%s'"), + dirpath); + return -1; + } + + while ((entry = readdir(dir))) { + /* Always Ignore "." and ".." */ + if (STREQ(entry->d_name, ".") || STREQ(entry->d_name, "..")) + continue; + + /* Ignore hidden files if it's required */ + if (flags & VIR_TRAVERSE_DIRECTORY_IGNORE_HIDDEN_FILES && + entry->d_name[0] == '.') + continue; + + if (virAsprintf(&fpath, "%s/%s", dirpath, entry->d_name) < 0) { + virReportOOMError(); + goto error; + } + + /* Skip to handle the entry as long as @skipcb returns 0 for it */ + if (skipcb && skipcb(fpath, opaque) == 0) { + VIR_FREE(fpath); + continue; + } + + if (lstat(fpath, &st) < 0) { + virReportSystemError(errno, + _("Failed to stat '%s'"), + fpath); + goto error; + } + + /* Don't follow symbol link unless it's explicitly required */ + if (S_ISLNK(st.st_mode) && + !(flags & VIR_TRAVERSE_DIRECTORY_FOLLOW_LINK)) { + VIR_FREE(fpath); + continue; + } + + if (S_ISDIR(st.st_mode)) { + char **tmp = NULL; + int rc; + + if ((rc = virTraverseDirectory(fpath, depth - 1, flags, cb, skipcb, + opaque, filepaths ? &tmp : NULL)) < 0) + goto error; + + if (rc > 0) { + /* Copy the filepaths returned by recursive call */ + if (filepaths) { + if (VIR_REALLOC_N((*filepaths), nfilepaths + rc) < 0) { + for (i = 0; i < rc; i++) + VIR_FREE(tmp[i]); + VIR_FREE(tmp); + virReportOOMError(); + goto error; + } + + for (i = 0; i < rc; i++) + (*filepaths)[nfilepaths + i] = tmp[i]; + } + nfilepaths += rc; + } + } else { + /* Don't handle the file unless it's the last depth */ + if ((flags & VIR_TRAVERSE_DIRECTORY_FALL_THROUGH) && + depth != 0) { + VIR_FREE(fpath); + continue; + } + + if (cb(fpath, opaque) == 0) { + if (filepaths) { + if (VIR_REALLOC_N((*filepaths), nfilepaths + 1) < 0) { + virReportOOMError(); + goto error; + } + + if (VIR_STRDUP((*filepaths)[nfilepaths], fpath) < 0) + goto error; + + } + nfilepaths++; + } + } + + VIR_FREE(fpath); + } + + ret = nfilepaths; + +cleanup: + closedir(dir); + return ret; + +error: + VIR_FREE(fpath); + if (filepaths) { + for (i = 0; i < nfilepaths; i++) + VIR_FREE((*filepaths)[i]); + VIR_FREE(*filepaths); + } + goto cleanup; +} diff --git a/src/util/virutil.h b/src/util/virutil.h index 280a18d..6c46f23 100644 --- a/src/util/virutil.h +++ b/src/util/virutil.h @@ -166,4 +166,56 @@ char *virFindFCHostCapableVport(const char *sysfs_prefix); int virCompareLimitUlong(unsigned long long a, unsigned long b); +/** + * virTraverseDirectoryCallback: + * @fpath: file path to handle + * @opaque: User data to pass to the callback + * + * Callback for "virTraverseDirectory". + * + * Returns 0 on success, -1 on failure. + */ +typedef int (*virTraverseDirectoryCallback)(const char *fpath, + void *opaque); + +/** + * virTraverseDirectorySkipCallback: + * @fpath: file path to handle + * @opaque: User data to pass to the callback + * + * Callback to control whether virTraverseDirectory should ship + * @fpath E.g. Skipping files whose name match a regex pattern. + * + * Return 0 to let "virTraverseDirectory" skip handling the @fpath, + * -1 to not skip. + */ +typedef int (*virTraverseDirectorySkipCallback)(const char *fpath, + void *opaque); + +/** + * virTraverseDirectoryFlags: + * + * Except virTraverseDirectorySkipCallback, one can use these flags to + * control the general behaviour of virTraverseDirectory + */ +enum { + /* Follow symbol links */ + VIR_TRAVERSE_DIRECTORY_FOLLOW_LINK = 1 << 0, + + /* Ignore hidden files or directory */ + VIR_TRAVERSE_DIRECTORY_IGNORE_HIDDEN_FILES = 1 << 1, + + /* Don't handle files unless it's in the last depth */ + VIR_TRAVERSE_DIRECTORY_FALL_THROUGH = 1 << 2, +} virTraverseDirectoryFlags; + +int virTraverseDirectory(const char *dirpath, + int depth, + unsigned int flags, + virTraverseDirectoryCallback cb, + virTraverseDirectorySkipCallback skipcb, + void *opaque, + char ***filepaths); + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(3) + #endif /* __VIR_UTIL_H__ */ -- 1.8.1.4

On 19/06/13 21:44, John Ferlan wrote: hm... I reviewed it times before posting it. :( I think there are some use cases won't care about the file paths it processed, it's a future consideration though, I don't use it in that way currently. Hm, what virFileDeleteTree does should be able to achieve with virTraverseDirectory, that's the use case which doesn't have to care about the returned file paths I think. Not sure, I know FALL_THROUGH is not a good name, but I don't see MATCH_DEPTH_ONLY makes it better either.. :-), since whether it's matching or anything else, completely depends on what @cb does. yes.. I made too many typos..

On 06/07/2013 01:03 PM, Osier Yang wrote: 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. 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 Since there's a VIR_FREE() on this below: static char *sysfs_devices_prefix = NULL; Since we're reusing this: VIR_FREE(addr); Seems to be ACK-able with the nits I've noted fixed. John

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'/> --- 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); + 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; -#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; } 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; } -- 1.8.1.4

On 19/06/13 23:32, John Ferlan wrote: 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. 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. 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.

On 06/07/2013 01:03 PM, Osier Yang wrote: ACK, but probably is something that doesn't need to be in this series or done at all since the changes don't affect code paths... John if you expand the scope of this - the next line is the cleanup: label inside the WITH_UDEV ifdef, so in reality this does nothing since serial is returned immediately thereafter and would be NULL when the return is < 0. This one is another unnecessary check since NULL would be the value of name if the return value < 0

On 06/07/2013 01:03 PM, Osier Yang wrote: Given the following code, the call to Split could use 4 (or 5) for the max... Not that it matters. so the optional unique_id isn't so optional after all for a couple of code paths. Issuing a message here could be confusing. Has this been tested within an environment where unique_id isn't specified? You need a VIR_FREE(addr); here John

The string can be padded either on the left (@from_right=false) or right (@from_right=true). --- src/libvirt_private.syms | 1 + src/util/virstring.c | 38 ++++++++++++++++++++++++++++++++++++++ src/util/virstring.h | 6 ++++++ tests/utiltest.c | 28 ++++++++++++++++++++++++++++ 4 files changed, 73 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index ce39cc6..27fb0b5 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1822,6 +1822,7 @@ virStringArrayHasString; virStringFreeList; virStringJoin; virStringListLength; +virStringPad; virStringSplit; virStrncpy; virStrndup; diff --git a/src/util/virstring.c b/src/util/virstring.c index 1937f82..498daab 100644 --- a/src/util/virstring.c +++ b/src/util/virstring.c @@ -608,3 +608,41 @@ size_t virStringListLength(char **strings) return i; } + +char * +virStringPad(const char *src, + char padchar, + unsigned int length, + bool from_right) +{ + virBuffer buf = VIR_BUFFER_INITIALIZER; + int len; + int i; + + len = strlen(src); + + if (len > length) + return NULL; + + if (!from_right) { + for (i = 0; i < length - len; i++) { + virBufferAsprintf(&buf, "%c", padchar); + } + + virBufferAsprintf(&buf, "%s", src); + } else { + virBufferAsprintf(&buf, "%s", src); + + for (i = 0; i < length - len; i++) { + virBufferAsprintf(&buf, "%c", padchar); + } + } + + if (virBufferError(&buf)) { + virBufferFreeAndReset(&buf); + virReportOOMError(); + return NULL; + } + + return virBufferContentAndReset(&buf); +} diff --git a/src/util/virstring.h b/src/util/virstring.h index 34ffae1..5809167 100644 --- a/src/util/virstring.h +++ b/src/util/virstring.h @@ -166,4 +166,10 @@ int virStrndup(char **dest, const char *src, ssize_t n, bool report, int domcode size_t virStringListLength(char **strings); +char *virStringPad(const char *src, + char padchar, + unsigned int length, + bool from_right) + ATTRIBUTE_NONNULL(1); + #endif /* __VIR_STRING_H__ */ diff --git a/tests/utiltest.c b/tests/utiltest.c index 41fdd7e..d567ecc 100644 --- a/tests/utiltest.c +++ b/tests/utiltest.c @@ -226,6 +226,33 @@ cleanup: } static int +testStringPad(const void *data ATTRIBUTE_UNUSED) +{ + const char *str = "1ff2"; + char *lpadstr = NULL; + char *rpadstr = NULL; + int ret = -1; + + if (!(lpadstr = virStringPad(str, '0', 7, false))) + return -1; + + if (STRNEQ(lpadstr, "0001ff2")) + goto cleanup; + + if (!(rpadstr = virStringPad(str, '0', 7, true))) + goto cleanup; + + if (STRNEQ(rpadstr, "1ff2000")) + goto cleanup; + + ret = 0; +cleanup: + VIR_FREE(lpadstr); + VIR_FREE(rpadstr); + return ret; +} + +static int mymain(void) { int result = 0; @@ -251,6 +278,7 @@ mymain(void) DO_TEST(ParseVersionString); DO_TEST(FindPCIDeviceByVPD); DO_TEST(ParseStableScsiHostAddress); + DO_TEST(StringPad); cleanup: VIR_FREE(test_sysfs); -- 1.8.1.4

On 20/06/13 01:11, John Ferlan wrote: Oh, sure, I didn't notify the virBufferAddChar Osier

On 08/06/13 01:03, Osier Yang wrote: With the attached diff squashed in:

On 20/06/13 02:28, John Ferlan wrote: Okay. I don't think so, indicate it requires 2 or 4 separators doesn't give the user what we expect clearly. That's why I use "duplicate" error messages, even if + if (!strchr(parent, '_') && + !strchr(parent, ':')) { is false, we still have to let the user know what the format we expect for "parent" attribute. glibc says atoi is absolete, and since it's not required to do any error checking, strtol is recommended. In libvirt, we have wrapper for strtol: virStrTo*. But I don't see it's better than using virStringPad if converting the string into int using virStringTo*. We have to check the return value of virStringTo* anyway here, because the user could input crazy data, e.g. 1234566789101112:01:02:02 Osier

On 06/07/2013 01:03 PM, Osier Yang wrote: It is up to the caller to free the memory returned. ...to here isn't necessary... and if you keep it, then the loop below goes from 1 to npaths since 0 is already covered... if (!smallest_path || (rc < smallest)) { smallest_path = paths[i]; smallest = rc } The (!smallest_path) is only necessary if you cut as noted above, but the resetting of smallest to rc would seem to be required in either case. The buf = NULL is superfluous John

--- src/storage/storage_backend_scsi.c | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/src/storage/storage_backend_scsi.c b/src/storage/storage_backend_scsi.c index 5635f73..da6a5dd 100644 --- a/src/storage/storage_backend_scsi.c +++ b/src/storage/storage_backend_scsi.c @@ -760,18 +760,14 @@ getAdapterName(virStoragePoolSourceAdapter adapter) if (adapter.type != VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_FC_HOST) { if (adapter.data.scsi_host.parent) { - if (!adapter.data.scsi_host.has_unique_id) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("'unique_id' is not specified for " - "scsi_host adapter")); + if (!(addr = getScsiHostParentAddress(adapter.data.scsi_host.parent))) return NULL; - } else { - if (!(addr = getScsiHostParentAddress(adapter.data.scsi_host.parent))) - return NULL; + if (!adapter.data.scsi_host.has_unique_id) + name = virGuessStableScsiHostName(NULL, addr); + else name = virParseStableScsiHostAddress(NULL, addr, adapter.data.scsi_host.unique_id); - } } else if (adapter.data.scsi_host.name) { if (VIR_STRDUP(name, adapter.data.scsi_host.name) < 0) return NULL; -- 1.8.1.4

On 20/06/13 03:00, John Ferlan wrote: 7/10's purpose is to find the scsi host by specified "parent" and "unique_id", but omitted "unique_id" was not supported. That says it matters.