Re: [PATCH v2 3/4] util: Introduce virStorageFileGetNPIVKey

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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

[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]

  Powered by Linux