On 07/19/2017 07:38 AM, Erik Skultety wrote: > 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 think you're becoming the new vHBA expert ;-) In this case, I tell them to go see figure one or go read the docs. From the storage pool page, managed description: "For configurations that do not provide an already created vHBA from a 'virsh nodedev-create', libvirt will set this property to "yes"." > > 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. > The wwnn/wwpn are the wwnn/wwpn used to create the vHBA. Typically, a SAN admin will "assign" the pair (there's some specific rules about them). If not provided for a storage pool, then it's an XML error. For a nodedev it's possible to have libvirt generate a wwnn/wwpn, which yes, is quite confusing. In doing so libvirt uses a specific base and adds to it (see virRandomGenerateWWN and cover at least one eye). I agree in general about the "complexity" thing, but as time has gone on requests for new ways to determine which parent to use has caused code bloat. Complexity is a time factored calculation. When originally implemented using "host#" for parent was perfectly fine, but then someone realized that it should be "scsi_host#". Then someone said, that "scsi_host#" wasn't good enough because it can change between reboots. So parent by wwnn/wwpn was added. During the discussions for that someone else said what about using the fabric_wwn in order to find a parent. Oh and the "future" holds being able to define multiple vHBA's because it's felt migration of domains using vHBA pools is going to need more than one vHBA because on the target host using the same wwnn/wwpn won't work (although I have doubts here, but haven't had the cycles to investigate). If you're interested, go read the tail end of the wiki (http://wiki.libvirt.org/page/NPIV_in_libvirt) - it'll show a bit of the history of how things looked at one time. TBH: Sometimes I think QE reads the code and figures out a way to create bugs based on assumptions the code makes rather than making sure the intended use cases "work properly". Hence this whole need to know whether the parent is the HBA or the vHBA. Why would *anyone* provide the HBA parent wwnn/wwpn when it's perfectly fine to create a storage pool of the HBA without it via: <adapter type='scsi_host' name='scsi_host3'/> but no, someone wants to be inventive and think/believe: <adapter type='fc_host' [parent='scsi_host3'] wwnn='HBA_wwnn' wwpn='HBA_wwpn'/> should work as well (where HBA_wwnn/wwpn are the wwnn/wwpn of scsi_host3 in this example). The second XML isn't illegal, but because scsi_host3 has vHBA characteristics (in this case the ability to create vports), thus it passes certain tests and "could" be a different, but legal way to essentially define the HBA as the storage pool. Wouldn't anyone use it this way - probably not as there's no reason since the vHBA LUN's won't be available. It's a digression + tirade ;-)... >> >> 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. > While I understand your point, I disagree. Purely moving the code doesn't work as the code in node_device_conf returns @name and code in storage_backend_scsi returns -1/0, so some amount of massaging the code is going to be required anyway. May as well get it right in one shot rather than confuse a future git bisector even more. I can change it to the snippet you show below, but I feel it should be all in one patch. Thanks for jumping in (and becoming the new expert) - John >> 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@xxxxxxxxxx> >> --- >> 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 -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
