[libvirt] [PATCH v3 14/18] conf: Convert virStoragePoolSourceAdapter to virStorageAdapter

[Laine Stump]

On 03/15/2017 09:33 AM, John Ferlan wrote:
> 
> 
> On 03/12/2017 05:53 PM, Laine Stump wrote:
>> On 03/10/2017 04:10 PM, John Ferlan wrote:
>>> Move the virStoragePoolSourceAdapter from storage_conf.h and rename
>>> to virStorageAdapter.
>>>
>>> Continue with code realignment for brevity and flow.
>>>
>>> Signed-off-by: John Ferlan <jferlan at redhat.com>
>>> ---
>>>  src/conf/storage_adapter_conf.c    | 71 ++++++++++++++++++--------------------
>>>  src/conf/storage_adapter_conf.h    | 51 ++++++++++++++++++++++++---
>>>  src/conf/storage_conf.c            | 32 ++++++++---------
>>>  src/conf/storage_conf.h            | 44 ++---------------------
>>>  src/libvirt_private.syms           |  2 --
>>>  src/phyp/phyp_driver.c             |  3 +-
>>>  src/storage/storage_backend_scsi.c | 18 +++++-----
>>>  src/test/test_driver.c             |  5 ++-
>>>  8 files changed, 109 insertions(+), 117 deletions(-)
>>>
>>> diff --git a/src/conf/storage_adapter_conf.c b/src/conf/storage_adapter_conf.c
>>> index 6efe5ae..53c07c7 100644
>>> --- a/src/conf/storage_adapter_conf.c
>>> +++ b/src/conf/storage_adapter_conf.c
>>> @@ -19,7 +19,7 @@
> 
> [...]
> 
>>>  int
>>> -virStorageAdapterParseValidate(virStoragePoolDefPtr ret)
>>> +virStorageAdapterParseValidate(virStorageAdapterPtr adapter)
>>>  {
>>> -    if (!ret->source.adapter.type) {
>>> +    if (!adapter->type) {
>>>          virReportError(VIR_ERR_XML_ERROR, "%s",
>>>                         _("missing storage pool source adapter"));
>>>          return -1;
>>>      }
>>>  
>>> -    if (ret->source.adapter.type ==
>>> -        VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_FC_HOST)
>>> -        return virStorageAdapterFCHostParseValidate(&ret->source.adapter.data.fchost);
>>> +    if (adapter->type == VIR_STORAGE_ADAPTER_TYPE_FC_HOST)
>>> +        return virStorageAdapterFCHostParseValidate(&adapter->data.fchost);
>>>  
>>> -    if (ret->source.adapter.type ==
>>> -        VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_SCSI_HOST)
>>> -        return virStorageAdapterSCSIHostParseValidate(&ret->source.adapter.data.scsi_host);
>>> +    if (adapter->type == VIR_STORAGE_ADAPTER_TYPE_SCSI_HOST)
>>> +        return virStorageAdapterSCSIHostParseValidate(&adapter->data.scsi_host);
>>>  
>>>      return 0;
>>>  }
>>> @@ -285,13 +280,13 @@ virStorageAdapterFCHostFormat(virBufferPtr buf,
>>>                                virStorageAdapterFCHostPtr fchost)
>>>  {
>>>      virBufferEscapeString(buf, " parent='%s'", fchost->parent);
>>> -    if (fchost->managed)
>>> -        virBufferAsprintf(buf, " managed='%s'",
>>> -                          virTristateBoolTypeToString(fchost->managed));
>>>      virBufferEscapeString(buf, " parent_wwnn='%s'", fchost->parent_wwnn);
>>>      virBufferEscapeString(buf, " parent_wwpn='%s'", fchost->parent_wwpn);
>>>      virBufferEscapeString(buf, " parent_fabric_wwn='%s'",
>>>                            fchost->parent_fabric_wwn);
>>> +    if (fchost->managed != VIR_TRISTATE_BOOL_ABSENT)
>>> +        virBufferAsprintf(buf, " managed='%s'",
>>> +                          virTristateBoolTypeToString(fchost->managed));
>>
>> No test cases that are tripped up by this change in order? (Not saying there need to be, just wondering...)
>>
> 
> No - the managed would probably only appear as "managed='no'", although
> managed='yes' is the de facto default and essentially determines whether
> or not to destroy the vHBA when the pool is destroyed.
> 
> I can move it if really desired/required 


Nah, order is supposed to not matter, and any code that relies on the
order of the elements is broken and needs to be fixed. That's the only
reason I thought of the unit tests.


> 
> Before I go off and push all this - just want to double check if you
> desire to see the adjustments in a v4 series or do you feel comfortable
> enough with a "summary"?

The summary is fine.

> 
> 1. The virStorageAdapterPtr and friends had no changes to their names.
> 
> 2. The function/helper names are now:
> 
> virStorageAdapterClearFCHost
> virStorageAdapterClear
> virStorageAdapterParseXMLFCHost
> virStorageAdapterParseXMLSCSIHost
> virStorageAdapterParseXMLLegacy  **
> virStorageAdapterParseXML
> virStorageAdapterValidateFCHost
> virStorageAdapterValidateSCSIHost
> virStorageAdapterValidate
> virStorageAdapterFormatFCHost
> virStorageAdapterFormatSCSIHost
> virStorageAdapterFormat
> 
> ** No such thing as a short comment ;-)
> 
> 3. In patch1 rather than the single if statement - there are now two.
> The end result is the following check:
> 
>     if ((fchost->parent_wwnn && !fchost->parent_wwpn)) {
>         virReportError(VIR_ERR_XML_ERROR,
>                        _("when providing parent_wwnn='%s', the "
>                          "parent_wwpn must also be provided"),
>                        fchost->parent_wwnn);
>         return -1;
>     }
> 
>     if (!fchost->parent_wwnn && fchost->parent_wwpn) {
>         virReportError(VIR_ERR_XML_ERROR,
>                        _("when providing parent_wwpn='%s', the "
>                          "parent_wwnn must also be provided"),
>                        fchost->parent_wwpn);
>         return -1;
>     }
> 
> which I'll replicate in a separately posted patch for node_device_conf

Yeah, that sounds more understandable.

> 
> 4. Using virPCIDeviceAddressPtr in getSCSIHostNumber and getAdapterName.
> Ironically the "original" series I had passed along the
> virStorageAdapterSCSIHostPtr, but since it's been decreed that a
> src/util function cannot include a src/conf header, I had to back that off.

But virPCIDeviceAddressPtr is defined in src/util/virpci.c. There are
*other* address types that are defined in conf (e.g.
virDomainDeviceDriveAddress), but not PCI addresses.