[libvirt] [PATCH v3 16/18] conf: Move/rename createVport and deleteVport

[John Ferlan]

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 at redhat.com>
>> ---
>>  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.

Tks -

John

> 
> Everything looks like a simple hatchet and stitch job to me - ACK.
> 
> 

[...]