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

[Ján Tomko]

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;
>>> +#endif
>>> +
>>> +    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
>>> '%s'"),
>>> +                       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.

Jano

>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 -
>
>John
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20190117/2607a2ad/attachment-0001.sig>