Re: [libvirt] [PATCH v3 08/18] conf: Extract SCSI adapter type processing into their own helpers

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

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