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
Attachment:
signature.asc
Description: PGP signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
