[libvirt] [PATCH v3 16/18] conf: Move/rename createVport and deleteVport
Laine Stump
laine at laine.org
Wed Mar 15 15:19:40 UTC 2017
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 ;-)
>
>
>>> }
>>> }
>>>
>>> - 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.
>>
>>
>
> [...]
>
More information about the libvir-list
mailing list