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

[John Ferlan]

On 03/15/2017 11:19 AM, Laine Stump wrote:
> 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 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 ;-)
>>
>>

Upon further reflection - I've flip-flopped - I'll do the save first and
then the create. Not sure I can remember or even recreate why the order
is as it is, so let's not mess with (ahem) success...

>>>>          }
>>>>      }
>>>>  
>>>> -    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...
> 

Nah - I was originally hoping for the leap of faith by just having 1
patch which is now 12 and could have been more.

Still in order to be true to having one patch be purely code motion - I
did insert a patch which just moves the createVport/deleteVport and
renames it.  Then the followup patch will do the ensure the device was
created logic.

So patches up through here have been ACK'd and I'll separate out and
push later on today... Then I'll rework the rest of the patches.

John