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