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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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



[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]
  Powered by Linux