On Tue, Jul 18, 2017 at 11:10:38AM -0400, John Ferlan wrote: > https://bugzilla.redhat.com/show_bug.cgi?id=1472277 > > Commit id '106930aaa' altered the order of checking for an existing > vHBA (e.g something created via nodedev-create functionality outside > of the storage pool logic) which inadvertantly broke the code to > decide whether to alter/force the fchost->managed field to be 'yes' > because the storage pool will be managing the created vHBA in order > to ensure when the storage pool is destroyed that the vHBA is also > destroyed. Right, I agree with this - unless the user explicitly states they want the pre-created vHBA to be managed, we don't force any setting. I was wondering though, what about a use case when the user wants the vHBA to be auto-created, but non-managed at the same time...(yeah, I know I'm stretching it a bit with these unlikely scenarios, but then, you'd surely agree, you've already seen some of similar sort and one can never expect what creative ways of defining the XML are there to be found :))

I was also about to point out in the previous version, that I didn't like how complex virNodeDeviceCreateVport was getting - do wwnn and wwpn relate to a vHBA at all or is it a regular HBA, does the vHBA exist already or should we actually create and manage it.

> > This patch moves the check (and checkParent helper) for an existing > vHBA back into the createVport in storage_backend_scsi. It also > restores the logic back to what commit id '79ab0935' had with As I'm writing below, if you want to go for a partial revert, this patch needs to be split in 2, it's never a good idea to move code segments and doing adjustments to it at the same time.

> respect to using "if (fchost->parent && !checkParent(...))" and > returns immediately. The changes made by commit id '08c0ea16f' > are only necessary to run the virStoragePoolFCRefreshThread when > a vHBA was really created because there's a timing lag such that > the refreshPool call made after a startPool from storagePoolCreate* > wouldn't necessarily find LUNs, but the thread would. For an already > existing vHBA, using the thread is unnecessary since the vHBA already > exists and the lag to configure the LUNs wouldn't exist. > > Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; > --- > src/conf/node_device_conf.c | 55 ------------------------------------ > src/storage/storage_backend_scsi.c | 57 ++++++++++++++++++++++++++++++++++++++ > 2 files changed, 57 insertions(+), 55 deletions(-) > > diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c > index 503b129..9c0ffa5 100644 > --- a/src/conf/node_device_conf.c > +++ b/src/conf/node_device_conf.c > @@ -2259,48 +2259,6 @@ virNodeDeviceGetParentName(virConnectPtr conn, > } > > > -/* > - * Using the host# name found via wwnn/wwpn lookup in the fc_host > - * sysfs tree to get the parent 'scsi_host#' to ensure it matches. > - */ > -static bool > -checkParent(virConnectPtr conn, > - const char *name, > - const char *parent_name) > -{ > - char *scsi_host_name = NULL; > - char *vhba_parent = NULL; > - bool retval = false; > - > - VIR_DEBUG("conn=%p, name=%s, parent_name=%s", conn, name, parent_name); > - > - /* autostarted pool - assume we're OK */ > - if (!conn) > - return true; > - > - if (virAsprintf(&scsi_host_name, "scsi_%s", name) < 0) > - goto cleanup; > - > - if (!(vhba_parent = virNodeDeviceGetParentName(conn, scsi_host_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); > - goto cleanup; > - } > - > - retval = true; > - > - cleanup: > - VIR_FREE(vhba_parent); > - VIR_FREE(scsi_host_name); > - return retval; > -} > - > - > /** > * @conn: Connection pointer > * @fchost: Pointer to vHBA adapter > @@ -2326,19 +2284,6 @@ virNodeDeviceCreateVport(virConnectPtr conn, > VIR_DEBUG("conn=%p, parent='%s', wwnn='%s' wwpn='%s'", > conn, NULLSTR(fchost->parent), fchost->wwnn, fchost->wwpn); > > - /* If we find an existing HBA/vHBA within the fc_host sysfs > - * using the wwnn/wwpn, then a nodedev is already created for > - * this pool and we don't have to create the vHBA > - */ > - 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)) > - VIR_FREE(name); > - > - return name; > - } > - > if (fchost->parent) { > if (VIR_STRDUP(parent_hoststr, fchost->parent) < 0) > goto cleanup; > diff --git a/src/storage/storage_backend_scsi.c b/src/storage/storage_backend_scsi.c > index f7378d3..aa0fb99 100644 > --- a/src/storage/storage_backend_scsi.c > +++ b/src/storage/storage_backend_scsi.c > @@ -211,6 +211,48 @@ getAdapterName(virStorageAdapterPtr adapter) > } > > > +/* > + * Using the host# name found via wwnn/wwpn lookup in the fc_host > + * sysfs tree to get the parent 'scsi_host#' to ensure it matches. > + */ > +static bool > +checkParent(virConnectPtr conn, > + const char *name, > + const char *parent_name) > +{ > + char *scsi_host_name = NULL; > + char *vhba_parent = NULL; > + bool retval = false; > + > + VIR_DEBUG("conn=%p, name=%s, parent_name=%s", conn, name, parent_name); > + > + /* autostarted pool - assume we're OK */ > + if (!conn) > + return true; > + > + if (virAsprintf(&scsi_host_name, "scsi_%s", name) < 0) > + goto cleanup; > + > + if (!(vhba_parent = virNodeDeviceGetParentName(conn, scsi_host_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); > + goto cleanup; > + } > + > + retval = true; > + > + cleanup: > + VIR_FREE(vhba_parent); > + VIR_FREE(scsi_host_name); > + return retval; > +} > + > + > static int > createVport(virConnectPtr conn, > virStoragePoolDefPtr def, > @@ -226,6 +268,21 @@ createVport(virConnectPtr conn, > conn, NULLSTR(configFile), NULLSTR(fchost->parent), > fchost->wwnn, fchost->wwpn); > > + /* If we find an existing HBA/vHBA within the fc_host sysfs > + * using the wwnn/wwpn, then a nodedev is already created for > + * this pool and we don't have to create the vHBA > + */ > + if ((name = virVHBAGetHostByWWN(NULL, fchost->wwnn, fchost->wwpn))) { > + ret = 0; > + > + /* 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)) > + ret = -1; > + > + VIR_FREE(name); > + return ret; Firstly, this patch is trying to do 2 things at once - a partial revert of commit and then altering a check to fix a BZ, it's easier for a reviewer to both track and read, I also think it will potentially make anyone who's going to later look at the git history in the future happier. Secondly, @ret is already initialized to -1 when entering ^this block and I don't find it that much readable when you potentially set it twice and do something that is already handled in the cleanup section. Since we return 0 either when the vHBA exist AND no parent to be checked was provided or when both the name and the parent which satisfies the sanity check was provided I suggest squashing the following in instead: diff --git a/src/storage/storage_backend_scsi.c b/src/storage/storage_backend_scsi.c index aa0fb992d..d75553f1b 100644 --- a/src/storage/storage_backend_scsi.c +++ b/src/storage/storage_backend_scsi.c @@ -273,15 +273,13 @@ createVport(virConnectPtr conn, * this pool and we don't have to create the vHBA */ if ((name = virVHBAGetHostByWWN(NULL, fchost->wwnn, fchost->wwpn))) { - ret = 0; /* 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)) - ret = -1; + if (!fchost->parent || checkParent(conn, name, fchost->parent)) + ret = 0; - VIR_FREE(name); - return ret; + goto cleanup; } /* Since we're creating the vHBA, then we need to manage removing it Other than this, the patch looks reasonable, I'll try a few scenarios in the meantime. Erik > + } > > /* Since we're creating the vHBA, then we need to manage removing it > * as well. Since we need this setting to "live" through a libvirtd

> diff --git a/src/storage/storage_backend_scsi.c b/src/storage/storage_backend_scsi.c > index 365ab77..d225e4f 100644 > --- a/src/storage/storage_backend_scsi.c > +++ b/src/storage/storage_backend_scsi.c > @@ -220,6 +220,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; > @@ -230,20 +231,52 @@ 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); Under what circumstances can parent be a ^^^^vHBA??

> 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)) { I truly find it pointless to try to do any kind of parent validation on HBA device. Can a HBA device actually have a scsi_host parent? If not, then we should only validate whether the device given by its wwnn and wwpn exist (we already know that since we're here) is vport capable and then if the 'parent' attribute has been defined, then return an error about invalid configuration, rather than trying to compare parent names in that matter. Erik