On Fri, Jan 18, 2019 at 09:42:34AM -0500, John Ferlan wrote: *its Unrelated whitespace change Reviewed-by: Ján Tomko <jtomko(a)redhat.com&gt; Jano

On Fri, Jan 18, 2019 at 09:42:35AM -0500, John Ferlan wrote: 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

On 1/29/19 10:14 AM, Ján Tomko wrote: So does the attached resolve the concern/issue you have? Tks, John

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(a)redhat.com&gt; --- 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 Tue, Jan 29, 2019 at 03:54:42PM +0100, Michal Privoznik wrote: *its [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! Reviewed-by: Ján Tomko <jtomko(a)redhat.com&gt; Jano