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
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2026 Red Hat, Inc.