8:57 a.m.
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(a)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(a)redhat.com>
Jano