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

Re: [libvirt] [PATCH] storage: More uniquely identify NPIV LUNs

On Tue, Jan 15, 2019 at 12:15:36PM -0500, John Ferlan wrote:

On 1/15/19 11:32 AM, Ján Tomko wrote:
On Tue, Dec 18, 2018 at 05:46:53PM -0500, John Ferlan wrote:
+    virCommandSetOutputBuffer(cmd, &target_port);
+    if (virCommandRun(cmd, NULL) < 0)
+        goto cleanup;
+    if (target_port && STRNEQ(target_port, "") &&
+        (id = strstr(target_port, "ID_TARGET_PORT="))) {
+        char *nl = strchr(id, '\n');
+        if (nl)
+            *nl = '\0';
+        id = strrchr(id, '=');
+        memmove(target_port, id + 1, strlen(id));
+    } else {
+        virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+                       _("unable to uniquely identify target port for
+                       dev);

The other usage of /lib/udev/scsi_id oddly guarded by WITH_UDEV actually
provides a fallback in case we're compiling on a system without udev.

I honestly cannot imagine this working without udev. Whether having NPIV
volumes exposed "could" happen with node_device_hal is, well, highly
doubtful.  Maybe someone would be brave enough to remove the hal code.

There is difference between a system with working udev (I assume that
would be equivalent to having /lib/udev/scsi_id) and a system with
WITH_UDEV, i.e. having devel headers for libudev.

A compile guard here is not necessary, since the virCommand APIs are
compiled unconditionally and I think the specific error message we get
(scsi_id command not found) is better than the vague error message.
Also, it's less code.

If you need to use #ifdef, conditionally compiling different functions
makes the control flow easier to follow.

Updating the existing virStorageBackendSCSISerial to parse output with
the --export option is possible, but affects more than just NPIV, so
this patch attempts to limit the scope.

Erroring out anyway makes the whole #ifdef even more pointless.

Well... If one doesn't error out then it leads to the problem seen
hidden in the weeds of the commit message, a:

   virHashAddOrUpdateEntry:341 : internal error: Duplicate key

In the absence of the "WITH_UDEV" guards, the error would be reported
by virCommandRun.

is issued during Refresh processing (virStoragePoolFCRefreshThread)
which is (and must be) done in thread. It has to be done in a thread
because sync'ing with udev in order to "wait" for any NPIV LUNs to be
generated during storage pool creation is not an acceptable option
especially since we don't know how many exist.

So what we have now is any multiport volume (e.g. NPIV) having only one
volume (more than likely port=1) being reported/added to the storage
pool's volumes list.

If you'd prefer to see the same fallback as virStorageBackendSCSISerial,
then I'm not opposed to that, but I'm also not clear from your response
if that's what you'd prefer/expect/accept.

In both cases (AFAIK we only build SCSI and ISCSI drivers on Linux),
I think the fallback is pointless due to the minimum of users that would
be using it.


Please be more specific how you you'd like to see this code organized
such that you won't feel so lost in the weeds.  And yes, the processing
of an NPIV device is very much so like a Rube Goldberg project. Your
link also has a remarkable similarity to the mouse trap game
[https://en.wikipedia.org/wiki/Mouse_Trap_(game)] ;-).

Tks -


Attachment: signature.asc
Description: PGP signature

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