On 03/15/2017 10:08 AM, John Ferlan wrote: > > > On 03/12/2017 06:35 PM, Laine Stump wrote: >> On 03/10/2017 04:10 PM, John Ferlan wrote: >>> Move the bulk of the code to the node_device_conf and rename to >>> virNodeDevice{Create|Delete}Vport. >>> >>> Alter the create algorithm slightly in order to return the name of >>> the scsi_host vHBA that was created. The existing code would fetch >>> the name and if it exists would start a thread looking for the LUNs; >>> however, in no way did it validate the device was created for the >>> storage pool even though it would be used thereafter. >>> >>> This slight change allows the code that checks if the node device >>> by wwnn/wwpn already exists and return that name. This fixes a >>> second yet seen issue that if the nodedev was present, but the >>> parent by name wasn't provided (perhaps parent by wwnn/wwpn or >>> by fabric_name), then a failure would be returned. For this path >>> it shouldn't be an error - we should just be happy that something >>> else is managing the device and we don't have to create/delete it. >>> >>> The end result is that the startVport code can now just start the >>> thread once it gets a non NULL name back. >>> >>> Signed-off-by: John Ferlan <jferlan@xxxxxxxxxx> >>> --- >>> src/conf/node_device_conf.c | 211 +++++++++++++++++++++++++++++++++++++ >>> src/conf/node_device_conf.h | 9 ++ >>> src/libvirt_private.syms | 2 + >>> src/storage/storage_backend_scsi.c | 192 +++------------------------------ >>> 4 files changed, 235 insertions(+), 179 deletions(-) >>> > > [...] > >>> - >>> - /* Since we're creating the vHBA, then we need to manage removing it >>> + /* Since we've created the vHBA, then we 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. >>> @@ -345,28 +244,20 @@ createVport(virConnectPtr conn, >> >> >> Right in this space the old code would set managed = YES and call virStoragePoolSaveConfig(). That is now done *after calling virVHBAManageVport() (whatever *that* does) and a few other things from post-saveconfig that have been moed into virNodeDeviceCreateVport(). I just want a confirmation that this change in ordering won't cause a problem... >> > > Right it was intentional and (famous last words) I don't think there's > going to be a problem in the change of ordering... > > One of the things virNodeDeviceCreateVport now does is ensure that the > vHBA was created (whether via using virNodeDeviceCreateXML success or > after the virVHBAManageVport(CREATE) ensuring that the new scsi_hostN by > FCHost wwnn/wwpn can be found and if not, attempt a DELETE. > > If you consider the "current" code - it would save the XML, then attempt > the CREATE, but not reset the config file if that failed. I suppose I > could have split across multiple patches too. Of course that's why my > commit message is extra wordy ;-) > > >>> } >>> } >>> >>> - if (virVHBAManageVport(parent_host, fchost->wwpn, fchost->wwnn, >>> - VPORT_CREATE) < 0) >>> - goto cleanup; >>> - >>> - virWaitForDevices(); >>> - >>> /* Creating our own VPORT didn't leave enough time to find any LUN's, >>> * so, let's create a thread whose job it is to call the FindLU's with >>> * retry logic set to true. If the thread isn't created, then no big >>> * deal since it's still possible to refresh the pool later. >>> */ >>> - if ((name = virVHBAGetHostByWWN(NULL, fchost->wwnn, fchost->wwpn))) { >>> - if (VIR_ALLOC(cbdata) == 0) { >>> - memcpy(cbdata->pool_uuid, def->uuid, VIR_UUID_BUFLEN); >>> - VIR_STEAL_PTR(cbdata->fchost_name, name); >>> - >>> - if (virThreadCreate(&thread, false, virStoragePoolFCRefreshThread, >>> - cbdata) < 0) { >>> - /* Oh well - at least someone can still refresh afterwards */ >>> - VIR_DEBUG("Failed to create FC Pool Refresh Thread"); >>> - virStoragePoolFCRefreshDataFree(cbdata); >>> - } >> >> Okay, since a failure of virVHBAGetHostByWWN() in virNodeDeviceCreateVport() would have resulted in an early return, it's okay to move all of this outside the conditional (which no longer exists!) >> > > Right - although if desired I can split this patch up into (at least) > two to make it more obvious. Nope. It was all decipherable as it is. Of course if you're looking to get your patch count up, then... > > Tks - > > John > >> >> Everything looks like a simple hatchet and stitch job to me - ACK. >> >> > > [...] > -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list