On 03/14/2017 07:05 PM, John Ferlan wrote:
On 03/11/2017 10:14 PM, Laine Stump wrote:
On 03/10/2017 04:10 PM, John Ferlan wrote:
Rather than have lots of ugly inline code create helpers to try and make things more readable. While creating the helpers realign the code as necessary. [same opinionated commentary about this being semi-pointless code churn :-P]
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/storage_adapter_conf.c | 239 +++++++++++++++++++++++----------------- 1 file changed, 138 insertions(+), 101 deletions(-)
diff --git a/src/conf/storage_adapter_conf.c b/src/conf/storage_adapter_conf.c index a2d4a3a..a361c34 100644 --- a/src/conf/storage_adapter_conf.c +++ b/src/conf/storage_adapter_conf.c @@ -52,12 +52,11 @@ virStorageAdapterFCHostClear(virStoragePoolSourceAdapterPtr adapter) void virStorageAdapterClear(virStoragePoolSourceAdapterPtr adapter) { - if (adapter->type == VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_FC_HOST) { + if (adapter->type == VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_FC_HOST) virStorageAdapterFCHostClear(adapter); - } else if (adapter->type == - VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_SCSI_HOST) { + + if (adapter->type == VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_SCSI_HOST) VIR_FREE(adapter->data.scsi_host.name); That works fine as long as virStorageAdapterFCHostClear() doesn't modify adapter->type ;-) Yeah, I'm sure it doesn't and that it never will. I just wanted to point out the theoretical danger of changing the logic of code just to make a line shorter so you can eliminate the braces :-P
True... Of course this could become a switch too right?
With a typecast in the switch. Don't forget the typecast in the switch!
- } }
@@ -95,6 +94,83 @@ virStorageAdapterFCHostParseXML(xmlNodePtr node, }
+static int +virStorageAdapterSCSIHostParseXML(xmlNodePtr node, + xmlXPathContextPtr ctxt, + virStoragePoolSourcePtr source) This should take a virStoragePoolSourceAdapterPtr (since "scsi" is an anonymous struct inside and anonymous union inside .... [see Path 07/18].
As you found out - that was later.
+{ + source->adapter.data.scsi_host.name = + virXMLPropString(node, "name"); + if (virXPathNode("./parentaddr", ctxt)) { + xmlNodePtr addrnode = virXPathNode("./parentaddr/address", ctxt); + virPCIDeviceAddressPtr addr = + &source->adapter.data.scsi_host.parentaddr; + + if (!addrnode) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("Missing scsi_host PCI address element")); + return -1; + } + source->adapter.data.scsi_host.has_parent = true; + if (virPCIDeviceAddressParseXML(addrnode, addr) < 0) + return -1; + if ((virXPathInt("string(./parentaddr/@unique_id)", + ctxt, + &source->adapter.data.scsi_host.unique_id) < 0) || + (source->adapter.data.scsi_host.unique_id < 0)) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("Missing or invalid scsi adapter " + "'unique_id' value")); + return -1; + } + } + + return 0; +} + + +static int +virStorageAdapterLegacyParseXML(xmlNodePtr node, ?Legacy?
Well the "old" format for SCSI parsed only "name" - I had no better idea other than legacy
Okay, some sort of comment about that would be appreciated. Looks like I hadn't actually said the magic word anywhere in my original response. Now that I know the argument types are heading somewhere in later patches, I can say "ACK with the *ParseValidate* name 'fixed', and a short comment added stating what is the "Legacy" being accounted for".