On 07/18/2017 03:03 AM, Erik Skultety wrote:

On Mon, Jul 03, 2017 at 02:52:05PM -0400, John Ferlan wrote:

...

Only alter the managed setting if it wasn't provided. If someone provided 'no', then honor that rather than overwriting.

IMO this deserves to be tracked by a BZ, since the current behaviour is in contrast to what the documentation says.

ACK

Erik

I think perhaps because it "bothered me" while testing the subsequent patch, I looked at the logic again and thought, how is this right... The myopia of the work didn't cause me to go backwards through history. So if I go back to the bz that added the feature (commit id '5530f248'): https://bugzilla.redhat.com/show_bug.cgi?id=1160926 The idea is/was to set 'managed' when this code created the vHBA and not when it was already defined. IOW: If virVHBAGetHostByWWN returns @name, then "something" already exists so we're not using any of the @parent* fields (parent, parent_wwnn/wwpn, parent_fabric_name) in order to create the vHBA, thus we should leave the managed as is and return. Eventually, along came commit id '106930aaa' which altered the order of the createVport code such that this code no longer could ascertain this and the only way to "save" the vHBA is by using "no"; otherwise, we forced usage of "managed='yes'". So of course since this commit, we have way to know if we used an existing vHBA (or HBA as it turns out) or if we created one. And we've "essentially forced" the setting of "yes" which says delete the vHBA when we're done. Then as you point out in the reply to the next patch, commit id '08c0ea16f' changes the return value from a 0,-1 to name or NULL. This doesn't help us out at all in regards to whether we set managed and creates even more problems (damn these refactors, they always seem to mess something up ;-)). So while I appreciate the ACK - I think I have to SNACK this one and instead move the virVHBAGetHostByWWN check back into createVport. That also includes moving the checkParent code back. This way, we'll have a better handle on whether "something" exists. This would also fix the checkParent check to follow what commit id '79ab0935' did Fortunately (to a degree I suppose) "finding" an existing (real) vHBA for a provided adapter wwnn/wwpn is "less" of a norm as the whole purpose of all the parent* fields is because someone created a storage pool and wanted it to manage creating the vHBA as opposed to someone creating a vHBA themselves, then adding a storage pool for the sole purpose to have the LUN's available via storage pool domain XML rather than via direct access to the LUN's in non pool domain XML (if that makes sense). So while it "is" a bug, I'd say it's less likely (but not impossible) that someone will actually be using such a configuration still. This is a really, really long way to say I'm not sure I see value in creating a bz honestly. Thanks for taking me down memory lane ;-) John BTW: When all the above code was originally created, using the HBA for the wwnn/wwpn of the <adapter> for the vhba wasn't even considered. It was only someone created a vHBA via nodedev-create and that the wwnn/wwpn was a vHBA. Hence the followup patch which will make that check whether it was a vHBA or an HBA and cause failure based on that.

...

Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_backend_scsi.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/src/storage/storage_backend_scsi.c b/src/storage/storage_backend_scsi.c index f7378d3..f3e62fb 100644 --- a/src/storage/storage_backend_scsi.c +++ b/src/storage/storage_backend_scsi.c @@ -227,12 +227,12 @@ createVport(virConnectPtr conn, fchost->wwnn, fchost->wwpn);

- /* Since we're creating the vHBA, then we need to manage removing it + /* Since we're creating the vHBA, then we may need to manage removing it * as well. Since we need this setting to "live" through a libvirtd * restart, we need to save the persistent configuration. So if not - * already defined as YES, then force the issue. + * already defined as YES or NO, then force the issue. */ - if (fchost->managed != VIR_TRISTATE_BOOL_YES) { + if (fchost->managed == VIR_TRISTATE_BOOL_ABSENT) { fchost->managed = VIR_TRISTATE_BOOL_YES; if (configFile) { if (virStoragePoolSaveConfig(configFile, def) < 0) -- 2.9.4

-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list

On Mon, Jul 03, 2017 at 02:52:06PM -0400, John Ferlan wrote:

https://bugzilla.redhat.com/show_bug.cgi?id=1458708

When originally designed in commit id '42a021c1', providing the 'parent' attribute to the <adapter type='fc_host' wwnn='vHBA_wwnn' wwpn='vHBA_wwpn'/> was checked to make sure that the "parent" of the wwnn/wwpn matched that of the provided parent "just in case" someone created the node device first, then defined the storage pool using that node device afterwards. The result is to not perform the vport_create call when the scsi_host represented by the wwnn/wwpn already exists since it would fail.

Eventually someone came up with the brilliant idea to provide wwnn/wwpn of an HBA instead of a vHBA, e.g. <adapter type='fc_host' wwnn='HBA_wwnn' wwpn='HBA_wwpn'/>. This is the same as creating a storage pool backed to the HBA that just happens to also be vport (vHBA) capable. The logic to bypass the vport_create call was the same as the vHBA code since the wwn's already exist. Once that was determined to work, adding in the 'parent' attribute caused a problem for the DeleteVport code, which was fixed by commit id '2c8e30ee7e'.

The next test tried was providing a valid pair of wwns that would find the scsi_host HBA, but providing the wrong name for the 'parent' attribute. This caused a different failure because at DeleteVport time if a parent is provided it was assumed valid especially since the CreateVport code would check that by calling virVHBAPathExists.

So alter the checkParent code to first ensure that the provided parent_name is a valid HBA/vHBA, then check if the found scsi_host is an HBA or a vHBA and make the appropriate check similar to the check made during DestroyVport. This restores some checks that were "lost" during various refactorings such as commit id '79ab0935' which altered the return value logic and commit id '9fdc8c426' which moved the parent host name validity check, but neglected to add a similar check for this odd HBA configuration. As it turns out prior to this patch, the checkParent code would fail for an HBA, but that was masked by the changed return value checking logic.

Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/node_device_conf.c | 50 +++++++++++++++++++++++++++++++++++++-------- 1 file changed, 42 insertions(+), 8 deletions(-)

diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c index e5947e6..d4075b5 100644 --- a/src/conf/node_device_conf.c +++ b/src/conf/node_device_conf.c @@ -2268,6 +2268,7 @@ checkParent(virConnectPtr conn, const char *name, const char *parent_name) { + unsigned int host_num; char *scsi_host_name = NULL; char *vhba_parent = NULL; bool retval = false; @@ -2278,20 +2279,53 @@ checkParent(virConnectPtr conn, if (!conn) return true;

- if (virAsprintf(&scsi_host_name, "scsi_%s", name) < 0) + if (virSCSIHostGetNumber(parent_name, &host_num) < 0) { + virReportError(VIR_ERR_XML_ERROR, + _("parent '%s' is not properly formatted"), name); goto cleanup; + }

- if (!(vhba_parent = virNodeDeviceGetParentName(conn, scsi_host_name))) + if (!virVHBAPathExists(NULL, host_num)) { + virReportError(VIR_ERR_XML_ERROR, + _("parent '%s' is not a vHBA/HBA"), parent_name); goto cleanup; + }

- if (STRNEQ(parent_name, vhba_parent)) { - virReportError(VIR_ERR_XML_ERROR, - _("Parent attribute '%s' does not match parent '%s' " - "determined for the '%s' wwnn/wwpn lookup."), - parent_name, vhba_parent, name); + if (virAsprintf(&scsi_host_name, "scsi_%s", name) < 0) + goto cleanup; + + if (virSCSIHostGetNumber(scsi_host_name, &host_num) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("host name '%s' is not properly formatted"), name); goto cleanup; }

+ /* If scsi_host_name is vport capable, then it's an HBA, thus compare + * only against the parent_name; otherwise, as long as the scsi_host_name + * path exists, then the scsi_host_name is a vHBA in which case we need + * to compare against it's parent. */ + if (virVHBAIsVportCapable(NULL, host_num)) { + if (STRNEQ(parent_name, scsi_host_name)) { + virReportError(VIR_ERR_XML_ERROR, + _("parent HBA '%s' doesn't match the wwnn/wwpn " + "scsi_host '%s'"), + parent_name, scsi_host_name); + goto cleanup; + } + } else { + if (!(vhba_parent = virNodeDeviceGetParentName(conn, scsi_host_name))) + goto cleanup; + + if (STRNEQ(parent_name, vhba_parent)) { + virReportError(VIR_ERR_XML_ERROR, + _("parent vHBA '%s' doesn't match the wwnn/wwpn " + "scsi_host '%s' parent '%s'"), + parent_name, scsi_host_name, vhba_parent); + goto cleanup; + } + } + + retval = true;

cleanup: @@ -2333,7 +2367,7 @@ virNodeDeviceCreateVport(virConnectPtr conn, if ((name = virVHBAGetHostByWWN(NULL, fchost->wwnn, fchost->wwpn))) { /* If a parent was provided, let's make sure the 'name' we've * retrieved has the same parent. If not this will cause failure. */ - if (fchost->parent && checkParent(conn, name, fchost->parent)) + if (fchost->parent && !checkParent(conn, name, fchost->parent)) VIR_FREE(name);

This last hunk actually causes another issue to which I created a BZ prior to going through your patch (I was just testing 1/2) and instead cooked one on my own, but since you're addressing it too, I would strip this hunk to a separate patch, squeezed it in between 1 and 2 and linked https://bugzilla.redhat.com/show_bug.cgi?id=1472277 to it - I also think that since this hasn't worked as advertised since commit @08c0ea16fc, this should go to v3.2.0-maint and onward. Erik PS: I still need to do an actual review of this patch though, this was just something that I spotted right away.