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@xxxxxxxxxx>
>>> ---
>>> 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.
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
