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

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(a)redhat.com&gt; >> --- >> 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

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"?

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

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.