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

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.

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

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.

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