[libvirt] [PATCH v2 0/4] storage: More uniquely identify NPIV LUNs
v1: https://www.redhat.com/archives/libvir-list/2018-December/msg00562.html but no review until January, see: https://www.redhat.com/archives/libvir-list/2019-January/msg00079.html Changes since v1: * Rework code to have virStorageBackendSCSISerial use the existing virStorageFileGetSCSIKey * Then introduce and use virStorageFileGetNPIVKey in order to get a more unique key for NPIV LUNs. Follows same fallback policy as SCSI LUNs if the called *Key function either doesn't exist or fails to return a valid value. John Ferlan (4): util: Modify virStorageFileGetSCSIKey return storage: Rework virStorageBackendSCSISerial util: Introduce virStorageFileGetNPIVKey storage: Fetch a unique key for vHBA/NPIV LUNs src/libvirt_private.syms | 1 + src/storage/storage_util.c | 45 +++++++---------- src/util/virstoragefile.c | 101 +++++++++++++++++++++++++++++++++++-- src/util/virstoragefile.h | 2 + 4 files changed, 118 insertions(+), 31 deletions(-) -- 2.20.1
Alter the "real" code to return -2 on virCommandRun failure. Alter the comments and function header to describe the function and it's returns. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/util/virstoragefile.c | 21 ++++++++++++++++++--- 1 file changed, 18 insertions(+), 3 deletions(-) diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index bd4b0274df..2511511d14 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -1425,9 +1425,24 @@ int virStorageFileGetLVMKey(const char *path, } #endif + #ifdef WITH_UDEV -int virStorageFileGetSCSIKey(const char *path, - char **key) +/* virStorageFileGetSCSIKey + * @path: Path to the SCSI device + * @key: Unique key to be returned + * + * Using a udev specific function, query the @path to get and return a + * unique @key for the caller to use. + * + * Returns: + * 0 On success, with the @key filled in or @key=NULL if the + * returned string was empty. + * -1 When WITH_UDEV is undefined and a system error is reported + * -2 When WITH_UDEV is defined, but calling virCommandRun fails + */ +int +virStorageFileGetSCSIKey(const char *path, + char **key) { int status; virCommandPtr cmd = virCommandNewArgList("/lib/udev/scsi_id", @@ -1436,7 +1451,7 @@ int virStorageFileGetSCSIKey(const char *path, "--device", path, NULL ); - int ret = -1; + int ret = -2; *key = NULL; -- 2.20.1
On Fri, Jan 18, 2019 at 09:42:34AM -0500, John Ferlan wrote:
Alter the "real" code to return -2 on virCommandRun failure. Alter the comments and function header to describe the function and it's returns.
*its
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/util/virstoragefile.c | 21 ++++++++++++++++++--- 1 file changed, 18 insertions(+), 3 deletions(-)
diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index bd4b0274df..2511511d14 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -1425,9 +1425,24 @@ int virStorageFileGetLVMKey(const char *path, } #endif
+
Unrelated whitespace change
#ifdef WITH_UDEV -int virStorageFileGetSCSIKey(const char *path, - char **key) +/* virStorageFileGetSCSIKey + * @path: Path to the SCSI device + * @key: Unique key to be returned + * + * Using a udev specific function, query the @path to get and return a + * unique @key for the caller to use. + * + * Returns: + * 0 On success, with the @key filled in or @key=NULL if the + * returned string was empty. + * -1 When WITH_UDEV is undefined and a system error is reported + * -2 When WITH_UDEV is defined, but calling virCommandRun fails + */ +int +virStorageFileGetSCSIKey(const char *path, + char **key) { int status; virCommandPtr cmd = virCommandNewArgList("/lib/udev/scsi_id",
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Alter the code to use the virStorageFileGetSCSIKey helper to fetch the unique key for the SCSI disk. Alter the logic to follow the former code which would return a duplicate of @dev when either the virCommandRun succeeded, but returned an empty string or when WITH_UDEV was not true. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_util.c | 34 ++++++++-------------------------- 1 file changed, 8 insertions(+), 26 deletions(-) diff --git a/src/storage/storage_util.c b/src/storage/storage_util.c index a84ee5b600..aa1af434de 100644 --- a/src/storage/storage_util.c +++ b/src/storage/storage_util.c @@ -3758,36 +3758,18 @@ virStorageBackendRefreshLocal(virStoragePoolObjPtr pool) static char * virStorageBackendSCSISerial(const char *dev) { + int rc; char *serial = NULL; -#ifdef WITH_UDEV - virCommandPtr cmd = virCommandNewArgList( - "/lib/udev/scsi_id", - "--replace-whitespace", - "--whitelisted", - "--device", dev, - NULL - ); - - /* Run the program and capture its output */ - virCommandSetOutputBuffer(cmd, &serial); - if (virCommandRun(cmd, NULL) < 0) - goto cleanup; -#endif - if (serial && STRNEQ(serial, "")) { - char *nl = strchr(serial, '\n'); - if (nl) - *nl = '\0'; - } else { - VIR_FREE(serial); - ignore_value(VIR_STRDUP(serial, dev)); - } + rc = virStorageFileGetSCSIKey(dev, &serial); + if (rc == 0 && serial) + return serial; -#ifdef WITH_UDEV - cleanup: - virCommandFree(cmd); -#endif + if (rc == -2) + return NULL; + virResetLastError(); + ignore_value(VIR_STRDUP(serial, dev)); return serial; } -- 2.20.1
On Fri, Jan 18, 2019 at 09:42:35AM -0500, John Ferlan wrote:
Alter the code to use the virStorageFileGetSCSIKey helper to fetch the unique key for the SCSI disk. Alter the logic to follow the former code which would return a duplicate of @dev when either the virCommandRun succeeded, but returned an empty string or when WITH_UDEV was not true.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_util.c | 34 ++++++++-------------------------- 1 file changed, 8 insertions(+), 26 deletions(-)
diff --git a/src/storage/storage_util.c b/src/storage/storage_util.c index a84ee5b600..aa1af434de 100644 --- a/src/storage/storage_util.c +++ b/src/storage/storage_util.c @@ -3758,36 +3758,18 @@ virStorageBackendRefreshLocal(virStoragePoolObjPtr pool) static char * virStorageBackendSCSISerial(const char *dev) { + int rc; char *serial = NULL; -#ifdef WITH_UDEV - virCommandPtr cmd = virCommandNewArgList( - "/lib/udev/scsi_id", - "--replace-whitespace", - "--whitelisted", - "--device", dev, - NULL - ); - - /* Run the program and capture its output */ - virCommandSetOutputBuffer(cmd, &serial); - if (virCommandRun(cmd, NULL) < 0) - goto cleanup; -#endif
- if (serial && STRNEQ(serial, "")) { - char *nl = strchr(serial, '\n'); - if (nl) - *nl = '\0'; - } else { - VIR_FREE(serial); - ignore_value(VIR_STRDUP(serial, dev)); - } + rc = virStorageFileGetSCSIKey(dev, &serial); + if (rc == 0 && serial) + return serial;
-#ifdef WITH_UDEV - cleanup: - virCommandFree(cmd); -#endif + if (rc == -2) + return NULL;
+ virResetLastError();
Every virReportError call logs the error into the configured log outputs and sets the thread-local error object. This only resets the error object, there's no way to unlog the error. If it's expected operation, we should not log an error in the first place. Jano
+ ignore_value(VIR_STRDUP(serial, dev)); return serial; }
-- 2.20.1
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 1/29/19 10:14 AM, Ján Tomko wrote:
On Fri, Jan 18, 2019 at 09:42:35AM -0500, John Ferlan wrote:
Alter the code to use the virStorageFileGetSCSIKey helper to fetch the unique key for the SCSI disk. Alter the logic to follow the former code which would return a duplicate of @dev when either the virCommandRun succeeded, but returned an empty string or when WITH_UDEV was not true.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_util.c | 34 ++++++++-------------------------- 1 file changed, 8 insertions(+), 26 deletions(-)
diff --git a/src/storage/storage_util.c b/src/storage/storage_util.c index a84ee5b600..aa1af434de 100644 --- a/src/storage/storage_util.c +++ b/src/storage/storage_util.c @@ -3758,36 +3758,18 @@ virStorageBackendRefreshLocal(virStoragePoolObjPtr pool) static char * virStorageBackendSCSISerial(const char *dev) { + int rc; char *serial = NULL; -#ifdef WITH_UDEV - virCommandPtr cmd = virCommandNewArgList( - "/lib/udev/scsi_id", - "--replace-whitespace", - "--whitelisted", - "--device", dev, - NULL - ); - - /* Run the program and capture its output */ - virCommandSetOutputBuffer(cmd, &serial); - if (virCommandRun(cmd, NULL) < 0) - goto cleanup; -#endif
- if (serial && STRNEQ(serial, "")) { - char *nl = strchr(serial, '\n'); - if (nl) - *nl = '\0'; - } else { - VIR_FREE(serial); - ignore_value(VIR_STRDUP(serial, dev)); - } + rc = virStorageFileGetSCSIKey(dev, &serial); + if (rc == 0 && serial) + return serial;
-#ifdef WITH_UDEV - cleanup: - virCommandFree(cmd); -#endif + if (rc == -2) + return NULL;
+ virResetLastError();
Every virReportError call logs the error into the configured log outputs and sets the thread-local error object.
This only resets the error object, there's no way to unlog the error. If it's expected operation, we should not log an error in the first place.
It's not necessarily expected that we cannot run the command because WITH_UDEV is undefined; however, leaving an error message that you can recover from is something that many code paths use virResetLastError without the need to alter called methods to tell them not to report an error such as this. Would you "prefer" to see a 3rd parameter to virStorageFileGetSCSIKey (such as @ignoreError) which is only used to not log the ENOSYS? And do you want to see that code? John
Jano
+ ignore_value(VIR_STRDUP(serial, dev)); return serial; }
-- 2.20.1
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 1/29/19 10:14 AM, Ján Tomko wrote:
On Fri, Jan 18, 2019 at 09:42:35AM -0500, John Ferlan wrote:
Alter the code to use the virStorageFileGetSCSIKey helper to fetch the unique key for the SCSI disk. Alter the logic to follow the former code which would return a duplicate of @dev when either the virCommandRun succeeded, but returned an empty string or when WITH_UDEV was not true.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_util.c | 34 ++++++++-------------------------- 1 file changed, 8 insertions(+), 26 deletions(-)
diff --git a/src/storage/storage_util.c b/src/storage/storage_util.c index a84ee5b600..aa1af434de 100644 --- a/src/storage/storage_util.c +++ b/src/storage/storage_util.c @@ -3758,36 +3758,18 @@ virStorageBackendRefreshLocal(virStoragePoolObjPtr pool) static char * virStorageBackendSCSISerial(const char *dev) { + int rc; char *serial = NULL; -#ifdef WITH_UDEV - virCommandPtr cmd = virCommandNewArgList( - "/lib/udev/scsi_id", - "--replace-whitespace", - "--whitelisted", - "--device", dev, - NULL - ); - - /* Run the program and capture its output */ - virCommandSetOutputBuffer(cmd, &serial); - if (virCommandRun(cmd, NULL) < 0) - goto cleanup; -#endif
- if (serial && STRNEQ(serial, "")) { - char *nl = strchr(serial, '\n'); - if (nl) - *nl = '\0'; - } else { - VIR_FREE(serial); - ignore_value(VIR_STRDUP(serial, dev)); - } + rc = virStorageFileGetSCSIKey(dev, &serial); + if (rc == 0 && serial) + return serial;
-#ifdef WITH_UDEV - cleanup: - virCommandFree(cmd); -#endif + if (rc == -2) + return NULL;
+ virResetLastError();
Every virReportError call logs the error into the configured log outputs and sets the thread-local error object.
This only resets the error object, there's no way to unlog the error. If it's expected operation, we should not log an error in the first place.
Jano
So does the attached resolve the concern/issue you have? Tks, John
On Wed, Jan 30, 2019 at 11:55:10AM -0500, John Ferlan wrote:
On 1/29/19 10:14 AM, Ján Tomko wrote:
On Fri, Jan 18, 2019 at 09:42:35AM -0500, John Ferlan wrote:
Alter the code to use the virStorageFileGetSCSIKey helper to fetch the unique key for the SCSI disk. Alter the logic to follow the former code which would return a duplicate of @dev when either the virCommandRun succeeded, but returned an empty string or when WITH_UDEV was not true.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_util.c | 34 ++++++++-------------------------- 1 file changed, 8 insertions(+), 26 deletions(-)
diff --git a/src/storage/storage_util.c b/src/storage/storage_util.c index a84ee5b600..aa1af434de 100644 --- a/src/storage/storage_util.c +++ b/src/storage/storage_util.c @@ -3758,36 +3758,18 @@ virStorageBackendRefreshLocal(virStoragePoolObjPtr pool) static char * virStorageBackendSCSISerial(const char *dev) { + int rc; char *serial = NULL; -#ifdef WITH_UDEV - virCommandPtr cmd = virCommandNewArgList( - "/lib/udev/scsi_id", - "--replace-whitespace", - "--whitelisted", - "--device", dev, - NULL - ); - - /* Run the program and capture its output */ - virCommandSetOutputBuffer(cmd, &serial); - if (virCommandRun(cmd, NULL) < 0) - goto cleanup; -#endif
- if (serial && STRNEQ(serial, "")) { - char *nl = strchr(serial, '\n'); - if (nl) - *nl = '\0'; - } else { - VIR_FREE(serial); - ignore_value(VIR_STRDUP(serial, dev)); - } + rc = virStorageFileGetSCSIKey(dev, &serial); + if (rc == 0 && serial) + return serial;
-#ifdef WITH_UDEV - cleanup: - virCommandFree(cmd); -#endif + if (rc == -2) + return NULL;
+ virResetLastError();
Every virReportError call logs the error into the configured log outputs and sets the thread-local error object.
This only resets the error object, there's no way to unlog the error. If it's expected operation, we should not log an error in the first place.
Jano
So does the attached resolve the concern/issue you have?
Tks,
John
From 6f9385248a9e78cc3619d02e4adf3205cbad874d Mon Sep 17 00:00:00 2001 From: John Ferlan <jferlan@redhat.com> Date: Tue, 29 Jan 2019 16:44:01 -0500 Subject: [PATCH] Squash with previous
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/locking/lock_driver_lockd.c | 2 +- src/storage/storage_util.c | 3 +-- src/util/virstoragefile.c | 10 +++++++--- src/util/virstoragefile.h | 3 ++- 4 files changed, 11 insertions(+), 7 deletions(-)
diff --git a/src/locking/lock_driver_lockd.c b/src/locking/lock_driver_lockd.c index 16fce551c3..4ffa92fc9b 100644 --- a/src/locking/lock_driver_lockd.c +++ b/src/locking/lock_driver_lockd.c @@ -531,7 +531,7 @@ static int virLockManagerLockDaemonAddResource(virLockManagerPtr lock, if (STRPREFIX(name, "/dev") && driver->scsiLockSpaceDir) { VIR_DEBUG("Trying to find an SCSI ID for %s", name); - if (virStorageFileGetSCSIKey(name, &newName) < 0) + if (virStorageFileGetSCSIKey(name, &newName, false) < 0) goto cleanup;
if (newName) { diff --git a/src/storage/storage_util.c b/src/storage/storage_util.c index d8fd76f6b6..85f1cbb57d 100644 --- a/src/storage/storage_util.c +++ b/src/storage/storage_util.c @@ -3782,14 +3782,13 @@ virStorageBackendSCSISerial(const char *dev) int rc; char *serial = NULL;
- rc = virStorageFileGetSCSIKey(dev, &serial); + rc = virStorageFileGetSCSIKey(dev, &serial, true); if (rc == 0 && serial) return serial;
if (rc == -2) return NULL;
- virResetLastError(); ignore_value(VIR_STRDUP(serial, dev)); return serial; } diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 2a38a08241..39c8511bef 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -1429,6 +1429,7 @@ int virStorageFileGetLVMKey(const char *path, /* virStorageFileGetSCSIKey * @path: Path to the SCSI device * @key: Unique key to be returned + * @ignoreError: Used to not report ENOSYS * * Using a udev specific function, query the @path to get and return a * unique @key for the caller to use. @@ -1441,7 +1442,8 @@ int virStorageFileGetLVMKey(const char *path, */ int virStorageFileGetSCSIKey(const char *path, - char **key) + char **key, + bool ignoreError ATTRIBUTE_UNUSED) { int status; virCommandPtr cmd = virCommandNewArgList("/lib/udev/scsi_id", @@ -1481,9 +1483,11 @@ virStorageFileGetSCSIKey(const char *path, } #else int virStorageFileGetSCSIKey(const char *path, - char **key ATTRIBUTE_UNUSED) + char **key ATTRIBUTE_UNUSED, + bool ignoreError) { - virReportSystemError(ENOSYS, _("Unable to get SCSI key for %s"), path); + if (!ignoreError) + virReportSystemError(ENOSYS, _("Unable to get SCSI key for %s"), path); return -1; } #endif diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h index 1d6161a2c7..03837e9e58 100644 --- a/src/util/virstoragefile.h +++ b/src/util/virstoragefile.h @@ -390,7 +390,8 @@ bool virStorageIsRelative(const char *backing); int virStorageFileGetLVMKey(const char *path, char **key); int virStorageFileGetSCSIKey(const char *path, - char **key); + char **key, + bool ignoreError);
void virStorageAuthDefFree(virStorageAuthDefPtr def); virStorageAuthDefPtr virStorageAuthDefCopy(const virStorageAuthDef *src);
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
The vHBA/NPIV LUNs created via the udev processing of the VPORT_CREATE command end up using the same serial value as seen/generated by the /lib/udev/scsi_id as returned during virStorageFileGetSCSIKey. Therefore, in order to generate a unique enough key to be used when adding the LUN as a volume during virStoragePoolObjAddVol a more unique key needs to be generated for an NPIV volume. The problem is illustrated by the following example, where scsi_host5 is a vHBA used with the following LUNs: $ lsscsi -tg ... [5:0:4:0] disk fc:0x5006016844602198,0x101f00 /dev/sdh /dev/sg23 [5:0:5:0] disk fc:0x5006016044602198,0x102000 /dev/sdi /dev/sg24 ... Calling virStorageFileGetSCSIKey would return: /lib/udev/scsi_id --device /dev/sdh --whitelisted --replace-whitespace /dev/sdh 350060160c460219850060160c4602198 /lib/udev/scsi_id --device /dev/sdh --whitelisted --replace-whitespace /dev/sdi 350060160c460219850060160c4602198 Note that althrough /dev/sdh and /dev/sdi are separate LUNs, they end up with the same serial number used for the vol->key value. When virStoragePoolFCRefreshThread calls virStoragePoolObjAddVol the second LUN fails to be added with the following message getting logged: virHashAddOrUpdateEntry:341 : internal error: Duplicate key To resolve this, virStorageFileGetNPIVKey will use a similar call sequence as virStorageFileGetSCSIKey, except that it will add the "--export" option to the call. This results in more detailed output which needs to be parsed in order to formulate a unique enough key to be used. In order to be unique enough, the returned value will concatenate the target port as returned in the "ID_TARGET_PORT" field from the command to the "ID_SERIAL" value. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/libvirt_private.syms | 1 + src/util/virstoragefile.c | 80 +++++++++++++++++++++++++++++++++++++++ src/util/virstoragefile.h | 2 + 3 files changed, 83 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index c3d6306809..bdc2877a9f 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2861,6 +2861,7 @@ virStorageFileGetMetadata; virStorageFileGetMetadataFromBuf; virStorageFileGetMetadataFromFD; virStorageFileGetMetadataInternal; +virStorageFileGetNPIVKey; virStorageFileGetRelativeBackingPath; virStorageFileGetSCSIKey; virStorageFileGetUniqueIdentifier; diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 2511511d14..759d0625b6 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -1490,6 +1490,86 @@ int virStorageFileGetSCSIKey(const char *path, #endif +#ifdef WITH_UDEV +/* virStorageFileGetNPIVKey + * @path: Path to the NPIV device + * @key: Unique key to be returned + * + * Using a udev specific function, query the @path to get and return a + * unique @key for the caller to use. Unlike the GetSCSIKey method, an + * NPIV LUN is uniquely identified by it's ID_TARGET_PORT value. + * + * Returns: + * 0 On success, with the @key filled in or @key=NULL if the + * returned output string didn't have the data we need to + * formulate a unique key value + * -1 When WITH_UDEV is undefined and a system error is reported + * -2 When WITH_UDEV is defined, but calling virCommandRun fails + */ +int +virStorageFileGetNPIVKey(const char *path, + char **key) +{ + int status; + VIR_AUTOFREE(char *) outbuf = NULL; + const char *serial; + const char *port; + virCommandPtr cmd = virCommandNewArgList("/lib/udev/scsi_id", + "--replace-whitespace", + "--whitelisted", + "--export", + "--device", path, + NULL + ); + int ret = -2; + + *key = NULL; + + /* Run the program and capture its output */ + virCommandSetOutputBuffer(cmd, &outbuf); + if (virCommandRun(cmd, &status) < 0) + goto cleanup; + + /* Explicitly check status == 0, rather than passing NULL + * to virCommandRun because we don't want to raise an actual + * error in this scenario, just return a NULL key. + */ + if (status == 0 && *outbuf && + (serial = strstr(outbuf, "ID_SERIAL=")) && + (port = strstr(outbuf, "ID_TARGET_PORT="))) { + char *serial_eq = strchr(serial, '='); + char *serial_nl = strchr(serial, '\n'); + char *port_eq = strchr(port, '='); + char *port_nl = strchr(port, '\n'); + + if (serial_eq) + serial = serial_eq + 1; + if (serial_nl) + *serial_nl = '\0'; + if (port_eq) + port = port_eq + 1; + if (port_nl) + *port_nl = '\0'; + + ignore_value(virAsprintf(key, "%s_PORT%s", serial, port)); + } + + ret = 0; + + cleanup: + virCommandFree(cmd); + + return ret; +} +#else +int virStorageFileGetNPIVKey(const char *path, + char **key ATTRIBUTE_UNUSED) +{ + virReportSystemError(ENOSYS, _("Unable to get NPIV key for %s"), path); + return -1; +} +#endif + /** * virStorageFileParseBackingStoreStr: * @str: backing store specifier string to parse diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h index 1d6161a2c7..bc94bae676 100644 --- a/src/util/virstoragefile.h +++ b/src/util/virstoragefile.h @@ -391,6 +391,8 @@ int virStorageFileGetLVMKey(const char *path, char **key); int virStorageFileGetSCSIKey(const char *path, char **key); +int virStorageFileGetNPIVKey(const char *path, + char **key); void virStorageAuthDefFree(virStorageAuthDefPtr def); virStorageAuthDefPtr virStorageAuthDefCopy(const virStorageAuthDef *src); -- 2.20.1
On 1/18/19 3:42 PM, John Ferlan wrote:
The vHBA/NPIV LUNs created via the udev processing of the VPORT_CREATE command end up using the same serial value as seen/generated by the /lib/udev/scsi_id as returned during virStorageFileGetSCSIKey. Therefore, in order to generate a unique enough key to be used when adding the LUN as a volume during virStoragePoolObjAddVol a more unique key needs to be generated for an NPIV volume.
The problem is illustrated by the following example, where scsi_host5 is a vHBA used with the following LUNs:
$ lsscsi -tg ... [5:0:4:0] disk fc:0x5006016844602198,0x101f00 /dev/sdh /dev/sg23 [5:0:5:0] disk fc:0x5006016044602198,0x102000 /dev/sdi /dev/sg24 ...
Calling virStorageFileGetSCSIKey would return:
/lib/udev/scsi_id --device /dev/sdh --whitelisted --replace-whitespace /dev/sdh 350060160c460219850060160c4602198 /lib/udev/scsi_id --device /dev/sdh --whitelisted --replace-whitespace /dev/sdi 350060160c460219850060160c4602198
Note that althrough /dev/sdh and /dev/sdi are separate LUNs, they end up with the same serial number used for the vol->key value. When virStoragePoolFCRefreshThread calls virStoragePoolObjAddVol the second LUN fails to be added with the following message getting logged:
virHashAddOrUpdateEntry:341 : internal error: Duplicate key
To resolve this, virStorageFileGetNPIVKey will use a similar call sequence as virStorageFileGetSCSIKey, except that it will add the "--export" option to the call. This results in more detailed output which needs to be parsed in order to formulate a unique enough key to be used. In order to be unique enough, the returned value will concatenate the target port as returned in the "ID_TARGET_PORT" field from the command to the "ID_SERIAL" value.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/libvirt_private.syms | 1 + src/util/virstoragefile.c | 80 +++++++++++++++++++++++++++++++++++++++ src/util/virstoragefile.h | 2 + 3 files changed, 83 insertions(+)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index c3d6306809..bdc2877a9f 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2861,6 +2861,7 @@ virStorageFileGetMetadata; virStorageFileGetMetadataFromBuf; virStorageFileGetMetadataFromFD; virStorageFileGetMetadataInternal; +virStorageFileGetNPIVKey; virStorageFileGetRelativeBackingPath; virStorageFileGetSCSIKey; virStorageFileGetUniqueIdentifier; diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 2511511d14..759d0625b6 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -1490,6 +1490,86 @@ int virStorageFileGetSCSIKey(const char *path, #endif
+#ifdef WITH_UDEV +/* virStorageFileGetNPIVKey + * @path: Path to the NPIV device + * @key: Unique key to be returned + * + * Using a udev specific function, query the @path to get and return a + * unique @key for the caller to use. Unlike the GetSCSIKey method, an + * NPIV LUN is uniquely identified by it's ID_TARGET_PORT value. + * + * Returns: + * 0 On success, with the @key filled in or @key=NULL if the + * returned output string didn't have the data we need to + * formulate a unique key value + * -1 When WITH_UDEV is undefined and a system error is reported + * -2 When WITH_UDEV is defined, but calling virCommandRun fails + */ +int +virStorageFileGetNPIVKey(const char *path, + char **key) +{ + int status; + VIR_AUTOFREE(char *) outbuf = NULL; + const char *serial; + const char *port; + virCommandPtr cmd = virCommandNewArgList("/lib/udev/scsi_id", + "--replace-whitespace", + "--whitelisted", + "--export", + "--device", path, + NULL + ); + int ret = -2; + + *key = NULL; + + /* Run the program and capture its output */ + virCommandSetOutputBuffer(cmd, &outbuf); + if (virCommandRun(cmd, &status) < 0) + goto cleanup; + + /* Explicitly check status == 0, rather than passing NULL + * to virCommandRun because we don't want to raise an actual + * error in this scenario, just return a NULL key. + */ + if (status == 0 && *outbuf && + (serial = strstr(outbuf, "ID_SERIAL=")) && + (port = strstr(outbuf, "ID_TARGET_PORT="))) { + char *serial_eq = strchr(serial, '='); + char *serial_nl = strchr(serial, '\n'); + char *port_eq = strchr(port, '='); + char *port_nl = strchr(port, '\n'); + + if (serial_eq) + serial = serial_eq + 1; + if (serial_nl) + *serial_nl = '\0'; + if (port_eq) + port = port_eq + 1; + if (port_nl) + *port_nl = '\0';
This looks cleaner IMO: # define ID_SERIAL "ID_SERIAL=" # define ID_TARGET_PORT "ID_TARGET_PORT=" and then the if() body: char *tmp; serial += strlen(ID_SERIAL); port += strlen(ID_TARGET_PORT); if ((tmp = strchr(serial, '\n'))) *tmp = '\0'; if ((tmp = strchr(port, '\n'))) *tmp = '\0'; followed by virAsprintf() you already have there. But I don't care that much. Michal
On Tue, Jan 29, 2019 at 03:54:42PM +0100, Michal Privoznik wrote:
On 1/18/19 3:42 PM, John Ferlan wrote:
The vHBA/NPIV LUNs created via the udev processing of the VPORT_CREATE command end up using the same serial value as seen/generated by the /lib/udev/scsi_id as returned during virStorageFileGetSCSIKey. Therefore, in order to generate a unique enough key to be used when adding the LUN as a volume during virStoragePoolObjAddVol a more unique key needs to be generated for an NPIV volume.
The problem is illustrated by the following example, where scsi_host5 is a vHBA used with the following LUNs:
$ lsscsi -tg ... [5:0:4:0] disk fc:0x5006016844602198,0x101f00 /dev/sdh /dev/sg23 [5:0:5:0] disk fc:0x5006016044602198,0x102000 /dev/sdi /dev/sg24 ...
Calling virStorageFileGetSCSIKey would return:
/lib/udev/scsi_id --device /dev/sdh --whitelisted --replace-whitespace /dev/sdh 350060160c460219850060160c4602198 /lib/udev/scsi_id --device /dev/sdh --whitelisted --replace-whitespace /dev/sdi 350060160c460219850060160c4602198
Note that althrough /dev/sdh and /dev/sdi are separate LUNs, they end up with the same serial number used for the vol->key value. When virStoragePoolFCRefreshThread calls virStoragePoolObjAddVol the second LUN fails to be added with the following message getting logged:
virHashAddOrUpdateEntry:341 : internal error: Duplicate key
To resolve this, virStorageFileGetNPIVKey will use a similar call sequence as virStorageFileGetSCSIKey, except that it will add the "--export" option to the call. This results in more detailed output which needs to be parsed in order to formulate a unique enough key to be used. In order to be unique enough, the returned value will concatenate the target port as returned in the "ID_TARGET_PORT" field from the command to the "ID_SERIAL" value.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/libvirt_private.syms | 1 + src/util/virstoragefile.c | 80 +++++++++++++++++++++++++++++++++++++++ src/util/virstoragefile.h | 2 + 3 files changed, 83 insertions(+)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index c3d6306809..bdc2877a9f 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2861,6 +2861,7 @@ virStorageFileGetMetadata; virStorageFileGetMetadataFromBuf; virStorageFileGetMetadataFromFD; virStorageFileGetMetadataInternal; +virStorageFileGetNPIVKey; virStorageFileGetRelativeBackingPath; virStorageFileGetSCSIKey; virStorageFileGetUniqueIdentifier; diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 2511511d14..759d0625b6 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -1490,6 +1490,86 @@ int virStorageFileGetSCSIKey(const char *path, #endif +#ifdef WITH_UDEV +/* virStorageFileGetNPIVKey + * @path: Path to the NPIV device + * @key: Unique key to be returned + * + * Using a udev specific function, query the @path to get and return a + * unique @key for the caller to use. Unlike the GetSCSIKey method, an + * NPIV LUN is uniquely identified by it's ID_TARGET_PORT value.
*its
+ * + * Returns: + * 0 On success, with the @key filled in or @key=NULL if the + * returned output string didn't have the data we need to + * formulate a unique key value
[0]
+ * -1 When WITH_UDEV is undefined and a system error is reported + * -2 When WITH_UDEV is defined, but calling virCommandRun fails + */ +int +virStorageFileGetNPIVKey(const char *path, + char **key) +{ + int status; + VIR_AUTOFREE(char *) outbuf = NULL; + const char *serial; + const char *port; + virCommandPtr cmd = virCommandNewArgList("/lib/udev/scsi_id", + "--replace-whitespace", + "--whitelisted", + "--export", + "--device", path, + NULL + ); + int ret = -2; + + *key = NULL; + + /* Run the program and capture its output */ + virCommandSetOutputBuffer(cmd, &outbuf); + if (virCommandRun(cmd, &status) < 0) + goto cleanup; + + /* Explicitly check status == 0, rather than passing NULL + * to virCommandRun because we don't want to raise an actual + * error in this scenario, just return a NULL key. + */ + if (status == 0 && *outbuf && + (serial = strstr(outbuf, "ID_SERIAL=")) && + (port = strstr(outbuf, "ID_TARGET_PORT="))) { + char *serial_eq = strchr(serial, '='); + char *serial_nl = strchr(serial, '\n'); + char *port_eq = strchr(port, '='); + char *port_nl = strchr(port, '\n'); + + if (serial_eq) + serial = serial_eq + 1; + if (serial_nl) + *serial_nl = '\0'; + if (port_eq) + port = port_eq + 1; + if (port_nl) + *port_nl = '\0';
For my one SCSI device, scsi_id happily returns some ID= variables with no values. To satisfy [0], something like: if (*port == '\0' || *serial == '\0') { VIR_FREE(*key); goto cleanup; } could be needed. String processing in C is fun!
This looks cleaner IMO:
# define ID_SERIAL "ID_SERIAL=" # define ID_TARGET_PORT "ID_TARGET_PORT="
and then the if() body: char *tmp;
serial += strlen(ID_SERIAL); port += strlen(ID_TARGET_PORT);
if ((tmp = strchr(serial, '\n'))) *tmp = '\0';
if ((tmp = strchr(port, '\n'))) *tmp = '\0';
followed by virAsprintf() you already have there. But I don't care that much.
Michal
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
https://bugzilla.redhat.com/show_bug.cgi?id=1657468 Commit be1bb6c95 changed the way volumes were stored from a forward linked list to a hash table. In doing so, it required that each vol object would have 3 unique values as keys into tables - key, name, and path. Due to how vHBA/NPIV LUNs are created/used this resulted in a failure to utilize all the LUN's found during processing. During virStorageBackendSCSINewLun processing fetch the key (or serial value) for NPIV LUN's using virStorageFileGetNPIVKey which will formulate a more unique key based on the serial value and the port for the LUN. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_util.c | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/src/storage/storage_util.c b/src/storage/storage_util.c index aa1af434de..089eca01bf 100644 --- a/src/storage/storage_util.c +++ b/src/storage/storage_util.c @@ -3756,12 +3756,16 @@ virStorageBackendRefreshLocal(virStoragePoolObjPtr pool) static char * -virStorageBackendSCSISerial(const char *dev) +virStorageBackendSCSISerial(const char *dev, + bool isNPIV) { int rc; char *serial = NULL; - rc = virStorageFileGetSCSIKey(dev, &serial); + if (isNPIV) + rc = virStorageFileGetNPIVKey(dev, &serial); + else + rc = virStorageFileGetSCSIKey(dev, &serial); if (rc == 0 && serial) return serial; @@ -3859,7 +3863,10 @@ virStorageBackendSCSINewLun(virStoragePoolObjPtr pool, VIR_STORAGE_VOL_READ_NOERROR)) < 0) goto cleanup; - if (!(vol->key = virStorageBackendSCSISerial(vol->target.path))) + vol->key = virStorageBackendSCSISerial(vol->target.path, + (def->source.adapter.type == + VIR_STORAGE_ADAPTER_TYPE_FC_HOST)); + if (!vol->key) goto cleanup; def->capacity += vol->target.capacity; -- 2.20.1
On Fri, Jan 18, 2019 at 09:42:37AM -0500, John Ferlan wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1657468
Commit be1bb6c95 changed the way volumes were stored from a forward linked list to a hash table. In doing so, it required that each vol object would have 3 unique values as keys into tables - key, name, and path. Due to how vHBA/NPIV LUNs are created/used this resulted in a failure to utilize all the LUN's found during processing.
During virStorageBackendSCSINewLun processing fetch the key (or serial value) for NPIV LUN's using virStorageFileGetNPIVKey which will formulate a more unique key based on the serial value and the port for the LUN.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_util.c | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
ping? Tks - John On 1/18/19 9:42 AM, John Ferlan wrote:
v1: https://www.redhat.com/archives/libvir-list/2018-December/msg00562.html
but no review until January, see: https://www.redhat.com/archives/libvir-list/2019-January/msg00079.html
Changes since v1:
* Rework code to have virStorageBackendSCSISerial use the existing virStorageFileGetSCSIKey
* Then introduce and use virStorageFileGetNPIVKey in order to get a more unique key for NPIV LUNs. Follows same fallback policy as SCSI LUNs if the called *Key function either doesn't exist or fails to return a valid value.
John Ferlan (4): util: Modify virStorageFileGetSCSIKey return storage: Rework virStorageBackendSCSISerial util: Introduce virStorageFileGetNPIVKey storage: Fetch a unique key for vHBA/NPIV LUNs
src/libvirt_private.syms | 1 + src/storage/storage_util.c | 45 +++++++---------- src/util/virstoragefile.c | 101 +++++++++++++++++++++++++++++++++++-- src/util/virstoragefile.h | 2 + 4 files changed, 118 insertions(+), 31 deletions(-)
On 1/18/19 3:42 PM, John Ferlan wrote:
v1: https://www.redhat.com/archives/libvir-list/2018-December/msg00562.html
but no review until January, see: https://www.redhat.com/archives/libvir-list/2019-January/msg00079.html
Changes since v1:
* Rework code to have virStorageBackendSCSISerial use the existing virStorageFileGetSCSIKey
* Then introduce and use virStorageFileGetNPIVKey in order to get a more unique key for NPIV LUNs. Follows same fallback policy as SCSI LUNs if the called *Key function either doesn't exist or fails to return a valid value.
John Ferlan (4): util: Modify virStorageFileGetSCSIKey return storage: Rework virStorageBackendSCSISerial util: Introduce virStorageFileGetNPIVKey storage: Fetch a unique key for vHBA/NPIV LUNs
src/libvirt_private.syms | 1 + src/storage/storage_util.c | 45 +++++++---------- src/util/virstoragefile.c | 101 +++++++++++++++++++++++++++++++++++-- src/util/virstoragefile.h | 2 + 4 files changed, 118 insertions(+), 31 deletions(-)
ACK Michal
participants (3)
-
John Ferlan -
Ján Tomko -
Michal Privoznik