On 06/07/2013 01:03 PM, Osier Yang wrote:

The SCSI host number is not stable on Linux platform, the number can be changed after a system rebooting or scsi kernel modules reloaded. To have a stable address for the scsi_host adapter of scsi pool, this introduces new XMLs like:

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:

<adapter type='scsi_host' parent='pci_0000_00_1f_2' unique_id='5'/>

Where "parent" is the parent device of the scsi host, it should be consistent with the name style what node device driver uses (Either udev backend style or HAL backend style), or the PCI address in format "domain:bus:slot:function" format. "unique_id" is the number exposed by sysfs. E.g:

% cat /sys/bus/pci/devices/0000:00:1f.2/ata5/host4/scsi_host/host4/unique_id 5

The attribute "parent" is required, attribute "unique_id" is optional, if it's omitted, the scsi host which has smallest unique_id under the

s/scsi/SCSI

"parent" device will be used.

"parent" and the old "name" attribute are exclusive, since they are both to indicate scsi host number.

The "parent" and "name" attributes are mutually exclusive to identify the SCSI host number. Use of the "parent" attribute will be the preferred mechanism.

This only supports to parse and format the XMLs. Later patch will add code to find out the scsi host number via "parent" and "unique_id";

Oh joy.

--- docs/schemas/basictypes.rng | 20 ++++++-- src/conf/storage_conf.c | 57 +++++++++++++++++++--- src/conf/storage_conf.h | 3 ++ .../pool-scsi-type-scsi-host-stable.xml | 15 ++++++ .../pool-scsi-type-scsi-host-stable.xml | 18 +++++++ tests/storagepoolxml2xmltest.c | 1 + 6 files changed, 104 insertions(+), 10 deletions(-) create mode 100644 tests/storagepoolxml2xmlin/pool-scsi-type-scsi-host-stable.xml create mode 100644 tests/storagepoolxml2xmlout/pool-scsi-type-scsi-host-stable.xml

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.

diff --git a/docs/schemas/basictypes.rng b/docs/schemas/basictypes.rng index 34c2254..3bdf923 100644 --- a/docs/schemas/basictypes.rng +++ b/docs/schemas/basictypes.rng @@ -347,9 +347,23 @@ <value>scsi_host</value> </attribute> </optional> - <attribute name='name'> - <text/> - </attribute> + <choice> + <group> + <attribute name='name'> + <text/> + </attribute> + </group> + <group> + <attribute name='parent'> + <text/> + </attribute> + <optional> + <attribute name='unique_id'> + <ref name='unsignedInt'/> + </attribute> + </optional> + </group> + </choice> </group> <group> <attribute name='type'> diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index a44e021..d7901af 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -320,6 +320,7 @@ virStoragePoolSourceAdapterClear(virStoragePoolSourceAdapter adapter) } else if (adapter.type == VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_SCSI_HOST) { VIR_FREE(adapter.data.scsi_host.name); + VIR_FREE(adapter.data.scsi_host.parent); } }

@@ -613,6 +614,8 @@ virStoragePoolDefParseSource(xmlXPathContextPtr ctxt, source->dir = virXPathString("string(./dir/@path)", ctxt);

if ((adapter_type = virXPathString("string(./adapter/@type)", ctxt))) { + int rc; + if ((source->adapter.type = virStoragePoolSourceAdapterTypeTypeFromString(adapter_type)) <= 0) { virReportError(VIR_ERR_XML_ERROR, @@ -633,23 +636,47 @@ virStoragePoolDefParseSource(xmlXPathContextPtr ctxt, VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_SCSI_HOST) { source->adapter.data.scsi_host.name = virXPathString("string(./adapter/@name)", ctxt); + source->adapter.data.scsi_host.parent = + virXPathString("string(./adapter/@parent)", ctxt); + + if ((rc = virXPathUInt("string(./adapter/@unique_id)", ctxt, + &source->adapter.data.scsi_host.unique_id)) < -1) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("Invalid unique_id for scsi source adapter")); + goto cleanup; + } else if (rc == 0) { + source->adapter.data.scsi_host.has_unique_id = true; + }

[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

} } else { char *wwnn = NULL; char *wwpn = NULL; char *parent = NULL; + bool has_unique_id = false; + int rc; + + rc = virXPathUInt("string(./adapter/@unique_id)", ctxt, + &source->adapter.data.scsi_host.unique_id); + + if (rc < -1) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("Invalid unique_id for scsi source adapter")); + goto cleanup;

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.

+ } else if (rc == 0) { + has_unique_id = true; + }

wwnn = virXPathString("string(./adapter/@wwnn)", ctxt); wwpn = virXPathString("string(./adapter/@wwpn)", ctxt); parent = virXPathString("string(./adapter/@parent)", ctxt);

- if (wwnn || wwpn || parent) { + if (wwnn || wwpn || parent || has_unique_id) { VIR_FREE(wwnn); VIR_FREE(wwpn); VIR_FREE(parent); virReportError(VIR_ERR_XML_ERROR, "%s", - _("Use of 'wwnn', 'wwpn', and 'parent' attributes " - "requires the 'fc_host' adapter 'type'")); + _("Use of 'wwnn', 'wwpn', 'parent' and 'unique_id' " + "attributes requires the adapter 'type' specified"));

s/specified/is specified

goto cleanup; }

@@ -927,9 +954,19 @@ virStoragePoolDefParseXML(xmlXPathContextPtr ctxt) goto error; } else if (ret->source.adapter.type == VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_SCSI_HOST) { - if (!ret->source.adapter.data.scsi_host.name) { + if (!ret->source.adapter.data.scsi_host.name && + !ret->source.adapter.data.scsi_host.parent) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("Either 'name' or 'parent' must be specified " + "for 'scsi_host' adapter"));

s/for/for the/

+ goto error; + } + + if (ret->source.adapter.data.scsi_host.name && + ret->source.adapter.data.scsi_host.parent) { virReportError(VIR_ERR_XML_ERROR, "%s", - _("missing storage pool source adapter name")); + _("'name' and 'parent' are exclusive " + "for 'scsi_host' adapter"));

The error message should read "Both 'name' and 'parent' cannot be specified for the 'scsi_host' adapter."

goto error; }

[1] See note from above regarding 'name' and 'unique_id' being set.

} @@ -1084,8 +1121,14 @@ virStoragePoolSourceFormat(virBufferPtr buf, src->adapter.data.fchost.wwpn); } else if (src->adapter.type == VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_SCSI_HOST) { - virBufferAsprintf(buf," name='%s'/>\n", - src->adapter.data.scsi_host.name); + if (src->adapter.data.scsi_host.name) { + virBufferAsprintf(buf," name='%s'/>\n", + src->adapter.data.scsi_host.name); + } else { + virBufferAsprintf(buf," parent='%s' unique_id='%u'/>\n", + src->adapter.data.scsi_host.parent, + src->adapter.data.scsi_host.unique_id); + }

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

} }

diff --git a/src/conf/storage_conf.h b/src/conf/storage_conf.h index 823d5af..801e41c 100644 --- a/src/conf/storage_conf.h +++ b/src/conf/storage_conf.h @@ -239,6 +239,9 @@ struct _virStoragePoolSourceAdapter { union { struct { char *name; + char *parent; + unsigned int unique_id; + bool has_unique_id; } scsi_host; struct { char *parent; diff --git a/tests/storagepoolxml2xmlin/pool-scsi-type-scsi-host-stable.xml b/tests/storagepoolxml2xmlin/pool-scsi-type-scsi-host-stable.xml new file mode 100644 index 0000000..fdf4002 --- /dev/null +++ b/tests/storagepoolxml2xmlin/pool-scsi-type-scsi-host-stable.xml @@ -0,0 +1,15 @@ +<pool type="scsi"> + <name>hba0</name> + <uuid>e9392370-2917-565e-692b-d057f46512d6</uuid> + <source> + <adapter type='scsi_host' parent='scsi_host' unique_id="5"/> + </source> + <target> + <path>/dev/disk/by-path</path> + <permissions> + <mode>0700</mode> + <owner>0</owner> + <group>0</group> + </permissions> + </target> +</pool> diff --git a/tests/storagepoolxml2xmlout/pool-scsi-type-scsi-host-stable.xml b/tests/storagepoolxml2xmlout/pool-scsi-type-scsi-host-stable.xml new file mode 100644 index 0000000..49d1cec --- /dev/null +++ b/tests/storagepoolxml2xmlout/pool-scsi-type-scsi-host-stable.xml @@ -0,0 +1,18 @@ +<pool type='scsi'> + <name>hba0</name> + <uuid>e9392370-2917-565e-692b-d057f46512d6</uuid> + <capacity unit='bytes'>0</capacity> + <allocation unit='bytes'>0</allocation> + <available unit='bytes'>0</available> + <source> + <adapter type='scsi_host' parent='scsi_host' unique_id='5'/> + </source> + <target> + <path>/dev/disk/by-path</path> + <permissions> + <mode>0700</mode> + <owner>0</owner> + <group>0</group> + </permissions> + </target> +</pool> diff --git a/tests/storagepoolxml2xmltest.c b/tests/storagepoolxml2xmltest.c index 0376e63..a9d36c5 100644 --- a/tests/storagepoolxml2xmltest.c +++ b/tests/storagepoolxml2xmltest.c @@ -97,6 +97,7 @@ mymain(void) DO_TEST("pool-iscsi-multiiqn"); DO_TEST("pool-iscsi-vendor-product"); DO_TEST("pool-sheepdog"); + DO_TEST("pool-scsi-type-scsi-host-stable");

return ret==0 ? EXIT_SUCCESS : EXIT_FAILURE; }

On 19/06/13 21:44, John Ferlan wrote:

On 06/07/2013 01:03 PM, Osier Yang wrote:

...

There is POSIX calls to walk through direcotry tree, nftw(3), but s/is/are s/direcotry/directory s/tree/trees or s/tree/a tree/

hm... I reviewed it times before posting it. :(

...

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. Not sure the last sentence is necessary

...

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. What use would a NULL filepath be? The whole purpose of the code is to generate a list of files with a specific pattern. The end result may be an empty list, but passing NULL

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.

...

* @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 s/skibcb/skipcb

...

+ * @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 s/handing/handling

...

+ * 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, s/@fillpaths/@filepaths (there's 2 of them)

...

+ * or -1 on failure. + */ NOTE: If you had run 'make -C tests valgrind' as part of your process you would have seen there's a memory leak here and the 'utiltest' fails. The output from my run has the following footprints repeated a few times:

==29216== 8 bytes in 1 blocks are definitely lost in loss record 2 of 39 ==29216== at 0x4A0887C: malloc (vg_replace_malloc.c:270) ==29216== by 0x4A089F0: realloc (vg_replace_malloc.c:662) ==29216== by 0x4C6C7EE: virReallocN (viralloc.c:184) ==29216== by 0x4CBC251: virTraverseDirectory (virutil.c:2592) ==29216== by 0x4CBC384: virTraverseDirectory (virutil.c:2562) ==29216== by 0x4CBC384: virTraverseDirectory (virutil.c:2562) ==29216== by 0x4CBCC7D: virFindPCIDeviceByVPD (virutil.c:2084) ==29216== by 0x40202D: testFindPCIDeviceByVPD (utiltest.c:175) ==29216== by 0x40326F: virtTestRun (testutils.c:158) ==29216== by 0x401D20: mymain (utiltest.c:305) ==29216== by 0x4038AA: virtTestMain (testutils.c:722) ==29216== by 0x37C1021A04: (below main) (libc-start.c:225) ==29216== ==29216== 8 bytes in 1 blocks are definitely lost in loss record 3 of 39 ==29216== at 0x4A0887C: malloc (vg_replace_malloc.c:270) ==29216== by 0x4A089F0: realloc (vg_replace_malloc.c:662) ==29216== by 0x4C6C7EE: virReallocN (viralloc.c:184) ==29216== by 0x4CBC3B4: virTraverseDirectory (virutil.c:2569) ==29216== by 0x4CBC384: virTraverseDirectory (virutil.c:2562) ==29216== by 0x4CBCC7D: virFindPCIDeviceByVPD (virutil.c:2084) ==29216== by 0x40202D: testFindPCIDeviceByVPD (utiltest.c:175) ==29216== by 0x40326F: virtTestRun (testutils.c:158) ==29216== by 0x401D20: mymain (utiltest.c:305) ==29216== by 0x4038AA: virtTestMain (testutils.c:722) ==29216== by 0x37C1021A04: (below main) (libc-start.c:225)

Beyond that the traversal seems OK to me - I think you're covering the basics with hidden files and links. I do remember a recent discussion regarding virFileDeleteTree() which may be useful to revisit to ensure nothing else is forgotten.

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.

...

+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'"), technically failed to 'lstat'

...

+ 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; + } Perhaps the valgrind error is that the else clause here or the one for "if (filepaths)" in the (rc > 0) condition.

In either case, tmp wouldn't be free()'d properly.

If you required filepaths to be non NULL - I believe the whole mess/issue would be irrelevant...

...

+ } 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, Better named "MATCH_DEPTH_ONLY"? FALL_THROUGH just has other

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.

...

+} 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) Not sure "3" is what you mean - perhaps you meant 4 (eg, cb).

yes.. I made too many typos..

It also stands to reason filepaths shouldn't be NULL. I guess I just don't see the reason to return just a count, but perhaps there's a use case I'm not thinking about or will be apparent in future patches.

John

...

+ #endif /* __VIR_UTIL_H__ */

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; }

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

On 06/07/2013 01:03 PM, Osier Yang wrote: 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; }

On 06/07/2013 01:03 PM, Osier Yang wrote:

--- src/storage/storage_backend_scsi.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)

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

diff --git a/src/storage/storage_backend_scsi.c b/src/storage/storage_backend_scsi.c index 0a79e6c..13c498d 100644 --- a/src/storage/storage_backend_scsi.c +++ b/src/storage/storage_backend_scsi.c @@ -188,7 +188,12 @@ virStorageBackendSCSISerial(const char *dev) *nl = '\0'; } else { VIR_FREE(serial); - ignore_value(VIR_STRDUP(serial, dev)); + if (VIR_STRDUP(serial, dev) < 0) +#ifdef WITH_UDEV + goto cleanup; +#else + return NULL; +#endif }

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.

#ifdef WITH_UDEV @@ -624,7 +629,8 @@ getAdapterName(virStoragePoolSourceAdapter adapter) char *name = NULL;

if (adapter.type != VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_FC_HOST) { - ignore_value(VIR_STRDUP(name, adapter.data.scsi_host.name)); + if (VIR_STRDUP(name, adapter.data.scsi_host.name) < 0) + return NULL;

This one is another unnecessary check since NULL would be the value of name if the return value < 0

return name; }

On 06/07/2013 01:03 PM, Osier Yang wrote:

This takes use of the two utils introduced in previous patches.

Node device HAL backend represents PCI device like PCI_8086_2922, (I.E PCI_$vendor_$product), to get the PCI address, we have to traverse /sys/devices/ to find it out.

And to get the current scsi host number assigned by the system for the scsi host device. We need to traverse /sys/bus/pci/devices/ with the found PCI address by getScsiHostParentAddress, and the specified unique_id.

Later patch will allow to omit the "unique_id", by using the scsi host which has the smallest unique_id. --- src/storage/storage_backend_scsi.c | 81 +++++++++++++++++++++++++++++++++++++- 1 file changed, 79 insertions(+), 2 deletions(-)

diff --git a/src/storage/storage_backend_scsi.c b/src/storage/storage_backend_scsi.c index 13c498d..a77b9ae 100644 --- a/src/storage/storage_backend_scsi.c +++ b/src/storage/storage_backend_scsi.c @@ -591,6 +591,66 @@ out: return retval; }

+/* + * Node device udev backend and HAL backend represent PCI + * device in different style. Udev backend represents it like + * pci_0000_00_1f_2, and HAL backend represents it like + * pci_8086_2922. + * + * Covert the value of "parent" into PCI device address string + * (e.g. 0000:00:1f:2). Return the PCI address as string on + * success, or -1 on failure. + */ +static char * +getScsiHostParentAddress(const char *parent) +{ + char **tokens = NULL; + char *vendor = NULL; + char *product = NULL; + char *ret = NULL; + int len; + + if (!strchr(parent, '_')) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("'parent' of scsi_host adapter must " + "be consistent with name of node device")); + return NULL; + } + + if (!(tokens = virStringSplit(parent, "_", 0))) + return NULL;

Given the following code, the call to Split could use 4 (or 5) for the max... Not that it matters.

+ + len = virStringListLength(tokens); + + switch (len) { + case 4: + if (!(ret = virStringJoin((const char **)(&tokens[1]), ":"))) + goto cleanup; + break; + + case 2: + vendor = tokens[1]; + product = tokens[2]; + if (!(ret = virFindPCIDeviceByVPD(NULL, vendor, product))) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Unable to find PCI device with vendor '%s' " + "product '%s'"), vendor, product); + goto cleanup; + } + + break; + default: + virReportError(VIR_ERR_XML_ERROR, "%s", + _("'parent' of scsi_host adapter must " + "be consistent with name of node device")); + goto cleanup; + } + +cleanup: + virStringFreeList(tokens); + return ret; +} + static int getHostNumber(const char *adapter_name, unsigned int *result) @@ -627,10 +687,27 @@ static char * getAdapterName(virStoragePoolSourceAdapter adapter) { char *name = NULL; + char *addr = NULL;

if (adapter.type != VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_FC_HOST) { - if (VIR_STRDUP(name, adapter.data.scsi_host.name) < 0) - return NULL; + 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"));

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?

+ return NULL; + } else { + if (!(addr = getScsiHostParentAddress(adapter.data.scsi_host.parent))) + return NULL; + + name = virParseStableScsiHostAddress(NULL, addr, + adapter.data.scsi_host.unique_id);

You need a VIR_FREE(addr); here John

+ } + } else if (adapter.data.scsi_host.name) { + if (VIR_STRDUP(name, adapter.data.scsi_host.name) < 0) + return NULL; + } + return name; }

On 20/06/13 01:11, John Ferlan wrote:

On 06/07/2013 01:03 PM, Osier Yang wrote:

...

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; } + This needs a description, params, returns, etc. section. To me padding usually denotes adding something to the end of a string

I would like to understand the value of "length" (which I believe is a size_t rather than a unsigned int, right?

I think most importantly - caller is expected to free the returned buffer, right?

...

+char * +virStringPad(const char *src, + char padchar, + unsigned int length, + bool from_right) +{ + virBuffer buf = VIR_BUFFER_INITIALIZER; + int len; + int i;

It seems you're trying to create a virBufferPrependChar() to go along with the existing virBufferAddChar() or virBufferAddLit().

Essentially you're creating a string buffer of "length - len" filled with 'padchar' and then prepending or appending it onto the existing string.

I guess I'm somewhat surprised this hasn't already been done.

In any case, I would have expected something as follows (error conditions aside):

if APPPEND (eg, change 1234 into 12340000) virBufferAdd(&buf, src); for (i = len; i < length; i++) virBufferAddChar(&buf, padchar);

Oh, sure, I didn't notify the virBufferAddChar Osier

On 08/06/13 01:03, Osier Yang wrote:

To be more flexible, except allowing to specify 'parent' with name produced by node device udev/HAL backends, this supports to specify 'parent' with PCI address directly (e.g. 0000:00:1f:2). The specified address will be padded if it's not consistent with what sysfs exposed. (e.g 0:0:2:2 will be padded to 0000:00:02:2). --- src/storage/storage_backend_scsi.c | 117 +++++++++++++++++++++++++++++-------- 1 file changed, 93 insertions(+), 24 deletions(-)

diff --git a/src/storage/storage_backend_scsi.c b/src/storage/storage_backend_scsi.c index a77b9ae..5635f73 100644 --- a/src/storage/storage_backend_scsi.c +++ b/src/storage/storage_backend_scsi.c @@ -604,51 +604,120 @@ out: static char * getScsiHostParentAddress(const char *parent) { + virBuffer buf = VIR_BUFFER_INITIALIZER; char **tokens = NULL; char *vendor = NULL; char *product = NULL; char *ret = NULL; - int len; + int length; + int i;

- if (!strchr(parent, '_')) { + if (!strchr(parent, '_') && + !strchr(parent, ':')) { virReportError(VIR_ERR_XML_ERROR, "%s", - _("'parent' of scsi_host adapter must " - "be consistent with name of node device")); + _("'parent' of scsi_host adapter must be " + "either consistent with name of mode " + "device, or in domain:bus:slot:function " + "format")); return NULL; }

- if (!(tokens = virStringSplit(parent, "_", 0))) - return NULL; + if (strchr(parent, '_')) { + if (!(tokens = virStringSplit(parent, "_", 0))) + return NULL;

- len = virStringListLength(tokens); + length = virStringListLength(tokens); + + switch (length) { + case 4: + if (!(ret = virStringJoin((const char **)(&tokens[1]), ":"))) + goto cleanup; + break;

- switch (len) { - case 4: - if (!(ret = virStringJoin((const char **)(&tokens[1]), ":"))) + case 2: + vendor = tokens[1]; + product = tokens[2]; + if (!(ret = virFindPCIDeviceByVPD(NULL, vendor, product))) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Unable to find PCI device with vendor '%s' " + "product '%s'"), vendor, product); + goto cleanup; + } + + break; + default: + virReportError(VIR_ERR_XML_ERROR, "%s", + _("'parent' of scsi_host adapter must be " + "either consistent with name of mode " + "device, or in domain:bus:slot:function " + "format")); goto cleanup; - break; + } + } else if (strchr(parent, ':')) { + char *padstr = NULL; + + if (!(tokens = virStringSplit(parent, ":", 0))) + return NULL;

- case 2: - vendor = tokens[1]; - product = tokens[2]; - if (!(ret = virFindPCIDeviceByVPD(NULL, vendor, product))) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Unable to find PCI device with vendor '%s' " - "product '%s'"), vendor, product); + length = virStringListLength(tokens); + + if (length != 4) { + virReportError(VIR_ERR_XML_ERROR, + _("invalid PCI address for scsi_host " + "'parent' '%s'"), parent); goto cleanup; }

- break; - default: - virReportError(VIR_ERR_XML_ERROR, "%s", - _("'parent' of scsi_host adapter must " - "be consistent with name of node device")); - goto cleanup; + for (i = 0; i < length; i++) { + if (strlen(tokens[i]) == 0) { + virReportError(VIR_ERR_XML_ERROR, + _("invalid PCI address for scsi_host " + "'parent' '%s'"), parent); + goto cleanup; + } + } + + /* sysfs always exposes the PCI address like "0000:00:1f:2", + * this do the padding if the address prodived by user is + * not padded (e.g. 0:0:2:0). + */ + if (strlen(tokens[0]) != 4) { + if (!(padstr = virStringPad(tokens[0], '0', 4, false))) + goto cleanup; + + virBufferAsprintf(&buf, "%s", padstr); + VIR_FREE(padstr); + } else { + virBufferAsprintf(&buf, "%s", tokens[0]); + } + + for (i = 1; i < 3; i++) { + if (strlen(tokens[i]) != 2) { + if (!(padstr = virStringPad(tokens[i], '0', 2, false))) + goto error; + virBufferAsprintf(&buf, "%s", padstr); With the attached diff squashed in:

On 20/06/13 02:28, John Ferlan wrote:

On 06/07/2013 01:16 PM, Osier Yang wrote:

...

On 08/06/13 01:03, Osier Yang wrote:

...

To be more flexible, except allowing to specify 'parent' with name produced by node device udev/HAL backends, this supports to specify 'parent' with PCI address directly (e.g. 0000:00:1f:2). The specified address will be padded if it's not consistent with what sysfs exposed. (e.g 0:0:2:2 will be padded to 0000:00:02:2). --- src/storage/storage_backend_scsi.c | 117 +++++++++++++++++++++++++++++-------- 1 file changed, 93 insertions(+), 24 deletions(-)

diff --git a/src/storage/storage_backend_scsi.c b/src/storage/storage_backend_scsi.c index a77b9ae..5635f73 100644 --- a/src/storage/storage_backend_scsi.c +++ b/src/storage/storage_backend_scsi.c @@ -604,51 +604,120 @@ out: static char * getScsiHostParentAddress(const char *parent) { + virBuffer buf = VIR_BUFFER_INITIALIZER; char **tokens = NULL; char *vendor = NULL; char *product = NULL; char *ret = NULL; - int len; + int length; + int i; - if (!strchr(parent, '_')) { + if (!strchr(parent, '_') && + !strchr(parent, ':')) { virReportError(VIR_ERR_XML_ERROR, "%s", - _("'parent' of scsi_host adapter must " - "be consistent with name of node device")); + _("'parent' of scsi_host adapter must be " + "either consistent with name of mode " + "device, or in domain:bus:slot:function " + "format")); "name of the node device or in"

not

"name of mode device, or in"

Okay.

...

...

return NULL; } - if (!(tokens = virStringSplit(parent, "_", 0))) - return NULL; + if (strchr(parent, '_')) { + if (!(tokens = virStringSplit(parent, "_", 0))) + return NULL; - len = virStringListLength(tokens); + length = virStringListLength(tokens); + + switch (length) { + case 4: + if (!(ret = virStringJoin((const char **)(&tokens[1]), ":"))) + goto cleanup; + break; - switch (len) { - case 4: - if (!(ret = virStringJoin((const char **)(&tokens[1]), ":"))) + case 2: + vendor = tokens[1]; + product = tokens[2]; + if (!(ret = virFindPCIDeviceByVPD(NULL, vendor, product))) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Unable to find PCI device with vendor '%s' " + "product '%s'"), vendor, product); + goto cleanup; + } + + break; + default: + virReportError(VIR_ERR_XML_ERROR, "%s", + _("'parent' of scsi_host adapter must be " + "either consistent with name of mode " + "device, or in domain:bus:slot:function " + "format"));

Duplicated error message - same issues as before, plus I think you need to consider determining which of the two places you got the error. That is if we see that message, then did we get an error because there wasn't a "_" or ":" in the name or (in this case) because the address was malformed since we expected only 2 or 4 numbers with a specific separator but found more or less. In this case, I would think you could just indicate the parent %s is malformed, requires only 2 or 4 separators.

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.

...

...

goto cleanup; - break; + } + } else if (strchr(parent, ':')) { + char *padstr = NULL; + + if (!(tokens = virStringSplit(parent, ":", 0))) + return NULL; - case 2: - vendor = tokens[1]; - product = tokens[2]; - if (!(ret = virFindPCIDeviceByVPD(NULL, vendor, product))) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Unable to find PCI device with vendor '%s' " - "product '%s'"), vendor, product); + length = virStringListLength(tokens); + + if (length != 4) { + virReportError(VIR_ERR_XML_ERROR, + _("invalid PCI address for scsi_host " + "'parent' '%s'"), parent); goto cleanup; } - break; - default: - virReportError(VIR_ERR_XML_ERROR, "%s", - _("'parent' of scsi_host adapter must " - "be consistent with name of node device")); - goto cleanup; + for (i = 0; i < length; i++) { + if (strlen(tokens[i]) == 0) { + virReportError(VIR_ERR_XML_ERROR, + _("invalid PCI address for scsi_host " + "'parent' '%s'"), parent); + goto cleanup; + } + } + + /* sysfs always exposes the PCI address like "0000:00:1f:2", + * this do the padding if the address prodived by user is

s/prodived/provided

...

...

+ * not padded (e.g. 0:0:2:0). + */ + if (strlen(tokens[0]) != 4) { + if (!(padstr = virStringPad(tokens[0], '0', 4, false))) + goto cleanup; + + virBufferAsprintf(&buf, "%s", padstr); + VIR_FREE(padstr); + } else { + virBufferAsprintf(&buf, "%s", tokens[0]); + } + + for (i = 1; i < 3; i++) { + if (strlen(tokens[i]) != 2) { + if (!(padstr = virStringPad(tokens[i], '0', 2, false))) + goto error; + virBufferAsprintf(&buf, "%s", padstr); I think the following syntax will avoid any sort of virStringPad() and whatever is going on above

virBufferAsprintf(&buf, "%04x:02x:02x:%s", atoi(tokens[0]), atoi(tokens[1]), atoi(tokens[2]), tokens[3]);

Assuming of course that each field is a base16 value and atoi() is "OK" to use here...

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:

Not really guessing, it returns host name of the scsi host which has smallest unique_id. --- src/libvirt_private.syms | 1 + src/util/virutil.c | 122 +++++++++++++++++++++++++++++++++++++++++++++++ src/util/virutil.h | 4 ++ tests/utiltest.c | 27 +++++++++++ 4 files changed, 154 insertions(+)

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 27fb0b5..ec85079 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1948,6 +1948,7 @@ virGetUserDirectory; virGetUserID; virGetUserName; virGetUserRuntimeDirectory; +virGuessStableScsiHostName; virHexToBin; virIndexToDiskName; virIsCapableFCHost; diff --git a/src/util/virutil.c b/src/util/virutil.c index a80574f..7f36e27 100644 --- a/src/util/virutil.c +++ b/src/util/virutil.c @@ -2256,6 +2256,120 @@ cleanup: } return ret; } + +static int +virGuessStableScsiHostNameCallback(const char *fpath, + void *opaque) +{ + const char *filename = opaque; + char *p = NULL; + + p = strrchr(fpath, '/'); + p++; + + if (STREQ(p, filename)) + return 0; + return -1; +} + +/** + * virGuessStableScsiHostName: + * @sysfs_prefix: The directory path where starts to traverse, defaults + * to SYSFS_BUS_PCI_DEVICES. + * @addr: The parent's PCI address + * + * Returns the host name (e.g. host10) of the scsi host whose parent + * has address @addr, and "unique_id" has the smallest value on success. + * Or NULL on failure.

It is up to the caller to free the memory returned.

+ */ +char * +virGuessStableScsiHostName(const char *sysfs_prefix, + const char *address) +{ + const char *prefix = sysfs_prefix ? sysfs_prefix : SYSFS_BUS_PCI_DEVICES; + unsigned int flags = 0; + char **paths = NULL; + int npaths = 0; + char *smallest_path = NULL; + unsigned int smallest; + char *dir = NULL; + char *buf = NULL; + char *p1 = NULL; + char *p2 = NULL; + char *ret = NULL; + int i; + + const char *filename = "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, + virGuessStableScsiHostNameCallback, + NULL, (void *)filename, &paths)) < 0) + goto cleanup; +

From here...

+ smallest_path = paths[0]; + if (virFileReadAll(paths[0], 1024, &buf) < 0) + goto cleanup; + + if ((p1 = strchr(buf, '\n'))) + *p1 = '\0'; + + if (virStrToLong_ui(buf, NULL, 10, &smallest) < 0) + goto cleanup; + + VIR_FREE(buf); + buf = NULL; ...to here isn't necessary... and if you keep it, then the loop below goes from 1 to npaths since 0 is already covered...

+ + for (i = 0; i < npaths; i++) { + unsigned int rc; + + if (virFileReadAll(paths[i], 1024, &buf) < 0) + goto cleanup; + + if ((p1 = strchr(buf, '\n'))) + *p1 = '\0'; + + if (virStrToLong_ui(buf, NULL, 10, &rc) < 0) + goto cleanup; + + if (rc < smallest) + smallest_path = paths[i];

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.

+ + VIR_FREE(buf); + buf = NULL;

The buf = NULL is superfluous John

+ } + + if (!(p1 = strstr(smallest_path, "scsi_host"))) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Unexpected path '%s' for PCI address " + "'%s' unique_id '%u'"), smallest_path, + address, smallest); + 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); + VIR_FREE(buf); + 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, @@ -2326,6 +2440,14 @@ virParseStableScsiHostAddress(const char *sysfs_prefix, virReportSystemError(ENOSYS, "%s", _("Not supported on this platform")); return NULL; } + +char * +virGuessStableScsiHostName(const char *sysfs_prefix, + const char *address) +{ + 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 2747cf1..10b3f6f 100644 --- a/src/util/virutil.h +++ b/src/util/virutil.h @@ -226,7 +226,11 @@ char *virFindPCIDeviceByVPD(const char *sysfs_prefix, char *virParseStableScsiHostAddress(const char *sysfs_prefix, const char *address, unsigned int unique_id) + ATTRIBUTE_NONNULL(2);

+char * +virGuessStableScsiHostName(const char *sysfs_prefix, + const char *address) ATTRIBUTE_NONNULL(2);

#endif /* __VIR_UTIL_H__ */ diff --git a/tests/utiltest.c b/tests/utiltest.c index d567ecc..0cfcead 100644 --- a/tests/utiltest.c +++ b/tests/utiltest.c @@ -226,6 +226,32 @@ cleanup: }

static int +testGuessStableScsiHostName(const void *data ATTRIBUTE_UNUSED) +{ + const char *addr = "0000:00:1f.2"; + char *dir = NULL; + char *rc = NULL; + int ret = -1; + + if (virAsprintf(&dir, "%s/%s", TEST_SYSFS, "bus/pci/devices") < 0) { + virReportOOMError(); + return -1; + } + + if (!(rc = virGuessStableScsiHostName(dir, addr))) + goto cleanup; + + if (STRNEQ(rc, "host2")) + goto cleanup; + + ret = 0; +cleanup: + VIR_FREE(dir); + VIR_FREE(rc); + return ret; +} + +static int testStringPad(const void *data ATTRIBUTE_UNUSED) { const char *str = "1ff2"; @@ -278,6 +304,7 @@ mymain(void) DO_TEST(ParseVersionString); DO_TEST(FindPCIDeviceByVPD); DO_TEST(ParseStableScsiHostAddress); + DO_TEST(GuessStableScsiHostName); DO_TEST(StringPad);

cleanup:

On 20/06/13 03:00, John Ferlan wrote:

On 06/07/2013 01:03 PM, Osier Yang wrote:

...

--- 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))) I don't understand the code churn that takes place for getAdapterName - is it really required to make the change in 7/11 especially since you're just undoing it here? That is between patches 7-10 if getAdapterName() didn't support the parent fields, would it matter?

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.

...

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);

Still need to VIR_FREE(addr);

John

...

- } } else if (adapter.data.scsi_host.name) { if (VIR_STRDUP(name, adapter.data.scsi_host.name) < 0) return NULL;