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@xxxxxxxxxx> --- 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@xxxxxxxxxx> Jano
Attachment:
signature.asc
Description: PGP signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
