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