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 - I guess I saw it as something "after" defining parent, parent_wwnn/wwpn, or parent_fabric_wwn - when adding those I should have put them after parent. Yeah, I know could have been a separate patch.
virBufferAsprintf(buf, " wwnn='%s' wwpn='%s'/>\n", fchost->wwnn, fchost->wwpn); @@ -322,14 +317,14 @@ virStorageAdapterSCSIHostFormat(virBufferPtr buf,
[...]
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 6a2bdf2..8a9e71b 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -883,8 +883,6 @@ virStoragePoolObjSaveDef; virStoragePoolObjUnlock; virStoragePoolSaveConfig; virStoragePoolSaveState; -virStoragePoolSourceAdapterTypeFromString; -virStoragePoolSourceAdapterTypeToString;
These are no longer used globally? (Just making sure)
Nope - replaced by virStorageAdapterType{From|To}String which are only local to storage_adapter_conf.c... That's the VIR_ENUM_IMPL change.
virStoragePoolSourceClear; virStoragePoolSourceDeviceClear; virStoragePoolSourceFindDuplicate;
[...]
diff --git a/src/storage/storage_backend_scsi.c b/src/storage/storage_backend_scsi.c index 77a51ff..ff17409 100644 --- a/src/storage/storage_backend_scsi.c +++ b/src/storage/storage_backend_scsi.c @@ -176,12 +176,12 @@ virStoragePoolFCRefreshThread(void *opaque) }
static char * -getAdapterName(virStoragePoolSourceAdapterPtr adapter) +getAdapterName(virStorageAdapterPtr adapter) { char *name = NULL; char *parentaddr = NULL;
- if (adapter->type == VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_SCSI_HOST) { + if (adapter->type == VIR_STORAGE_ADAPTER_TYPE_SCSI_HOST) { virStorageAdapterSCSIHostPtr scsi_host = &adapter->data.scsi_host;
if (scsi_host->has_parent) { @@ -197,7 +197,9 @@ getAdapterName(virStoragePoolSourceAdapterPtr adapter) } else { ignore_value(VIR_STRDUP(name, scsi_host->name)); } - } else if (adapter->type == VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_FC_HOST) { + } + + if (adapter->type == VIR_STORAGE_ADAPTER_TYPE_FC_HOST) {
If you're just getting rid of the "} else" in order to shorten the line, then I'd say leave it in...
OK
virStorageAdapterFCHostPtr fchost = &adapter->data.fchost;
if (!(name = virVHBAGetHostByWWN(NULL, fchost->wwnn, fchost->wwpn))) { @@ -451,7 +453,7 @@ virStorageBackendSCSICheckPool(virStoragePoolObjPtr pool, * the adapter based on might be not created yet. */ if (pool->def->source.adapter.type == - VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_FC_HOST) { + VIR_STORAGE_ADAPTER_TYPE_FC_HOST) { virResetLastError(); return 0; } else { @@ -505,24 +507,24 @@ virStorageBackendSCSIRefreshPool(virConnectPtr conn ATTRIBUTE_UNUSED, return ret; }
[...]
ACK.
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. Tks - John