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

On 03/10/2017 04:10 PM, John Ferlan wrote: > Rather than have lots of ugly inline code create helpers to try and s/code create/code, create/ (it took me a second to parse it - at first my brain wanted to understand that there had been ugly inline code that was creating helpers (whatever *that* might mean!) > make things more readable. So this is being done just because the functions are each "too long", not because these new functions are going to be called from multiple places. Seems kind of unnecessary, since they don't even have the upside of shortening long variable names and repeated pointer dereferences (which of course will be optimized out anyway)... > While creating the helpers realign the code > as necessary. > > Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; > --- > src/conf/storage_adapter_conf.c | 194 +++++++++++++++++++++++----------------- > 1 file changed, 114 insertions(+), 80 deletions(-) > > diff --git a/src/conf/storage_adapter_conf.c b/src/conf/storage_adapter_conf.c > index 4f5b665..a2d4a3a 100644 > --- a/src/conf/storage_adapter_conf.c > +++ b/src/conf/storage_adapter_conf.c > @@ -37,16 +37,23 @@ VIR_ENUM_IMPL(virStoragePoolSourceAdapter, > "default", "scsi_host", "fc_host") > > > +static void > +virStorageAdapterFCHostClear(virStoragePoolSourceAdapterPtr adapter) I *think* Dan's new function naming guidelines had said that functions could be either virSubjectVerbObject or virSubjectObjectVerb, so I guess this is okay (although I think I may slightly prefer virStorageAdapterClearFCHost() since the object being operated on is a virStorage[PoolSource]AdapterPtr). > +{ > + VIR_FREE(adapter->data.fchost.wwnn); > + VIR_FREE(adapter->data.fchost.wwpn); > + VIR_FREE(adapter->data.fchost.parent); > + VIR_FREE(adapter->data.fchost.parent_wwnn); > + VIR_FREE(adapter->data.fchost.parent_wwpn); > + VIR_FREE(adapter->data.fchost.parent_fabric_wwn); > +} > + > + > void > virStorageAdapterClear(virStoragePoolSourceAdapterPtr adapter) > { > if (adapter->type == VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_FC_HOST) { > - VIR_FREE(adapter->data.fchost.wwnn); > - VIR_FREE(adapter->data.fchost.wwpn); > - VIR_FREE(adapter->data.fchost.parent); > - VIR_FREE(adapter->data.fchost.parent_wwnn); > - VIR_FREE(adapter->data.fchost.parent_wwpn); > - VIR_FREE(adapter->data.fchost.parent_fabric_wwn); > + virStorageAdapterFCHostClear(adapter); > } else if (adapter->type == > VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_SCSI_HOST) { > VIR_FREE(adapter->data.scsi_host.name); > @@ -54,6 +61,40 @@ virStorageAdapterClear(virStoragePoolSourceAdapterPtr adapter) > } > > > +static int > +virStorageAdapterFCHostParseXML(xmlNodePtr node, > + virStoragePoolSourcePtr source) Hmmm, or maybe these functions could take a virStorageFCHostPtr (if only such a thing existed, rather than it being an anonymous struct inside an anonymous union inside a virStoragePoolSourceAdapter :-/ ) In the end I guess I'm okay with the names you've given and leaving well enough alone with the struct.

> +{ > + char *managed = NULL; > + > + source->adapter.data.fchost.parent = virXMLPropString(node, "parent"); > + if ((managed = virXMLPropString(node, "managed"))) { > + source->adapter.data.fchost.managed = > + virTristateBoolTypeFromString(managed); > + if (source->adapter.data.fchost.managed < 0) { > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, > + _("unknown fc_host managed setting '%s'"), > + managed); > + VIR_FREE(managed); > + return -1; > + } > + } > + > + source->adapter.data.fchost.parent_wwnn = > + virXMLPropString(node, "parent_wwnn"); > + source->adapter.data.fchost.parent_wwpn = > + virXMLPropString(node, "parent_wwpn"); > + source->adapter.data.fchost.parent_fabric_wwn = > + virXMLPropString(node, "parent_fabric_wwn"); > + > + source->adapter.data.fchost.wwpn = virXMLPropString(node, "wwpn"); > + source->adapter.data.fchost.wwnn = virXMLPropString(node, "wwnn"); > + > + VIR_FREE(managed); > + return 0; > +} > + > + > int > virStorageAdapterParseXML(virStoragePoolSourcePtr source, > xmlNodePtr node, > @@ -62,7 +103,6 @@ virStorageAdapterParseXML(virStoragePoolSourcePtr source, > int ret = -1; > xmlNodePtr relnode = ctxt->node; > char *adapter_type = NULL; > - char *managed = NULL; > > ctxt->node = node; > > @@ -77,29 +117,8 @@ virStorageAdapterParseXML(virStoragePoolSourcePtr source, > > if (source->adapter.type == > VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_FC_HOST) { > - source->adapter.data.fchost.parent = > - virXMLPropString(node, "parent"); > - managed = virXMLPropString(node, "managed"); > - if (managed) { > - source->adapter.data.fchost.managed = > - virTristateBoolTypeFromString(managed); > - if (source->adapter.data.fchost.managed < 0) { > - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, > - _("unknown fc_host managed setting '%s'"), > - managed); > - goto cleanup; > - } > - } > - > - source->adapter.data.fchost.parent_wwnn = > - virXMLPropString(node, "parent_wwnn"); > - source->adapter.data.fchost.parent_wwpn = > - virXMLPropString(node, "parent_wwpn"); > - source->adapter.data.fchost.parent_fabric_wwn = > - virXMLPropString(node, "parent_fabric_wwn"); > - > - source->adapter.data.fchost.wwpn = virXMLPropString(node, "wwpn"); > - source->adapter.data.fchost.wwnn = virXMLPropString(node, "wwnn"); > + if (virStorageAdapterFCHostParseXML(node, source) < 0) > + goto cleanup; > } else if (source->adapter.type == > VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_SCSI_HOST) { > > @@ -171,56 +190,63 @@ virStorageAdapterParseXML(virStoragePoolSourcePtr source, > cleanup: > ctxt->node = relnode; > VIR_FREE(adapter_type); > - VIR_FREE(managed); > return ret; > } > > > -int > -virStorageAdapterParseValidate(virStoragePoolDefPtr ret) > +static int > +virStorageAdapterFCHostParseValidate(virStoragePoolDefPtr ret) Please get rid of "Parse" in this name though.

> { > - if (!ret->source.adapter.type) { > + if (!ret->source.adapter.data.fchost.wwnn || > + !ret->source.adapter.data.fchost.wwpn) { > virReportError(VIR_ERR_XML_ERROR, "%s", > - _("missing storage pool source adapter")); > + _("'wwnn' and 'wwpn' must be specified for adapter " > + "type 'fchost'")); > return -1; > } > > - if (ret->source.adapter.type == > - VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_FC_HOST) { > - if (!ret->source.adapter.data.fchost.wwnn || > - !ret->source.adapter.data.fchost.wwpn) { > - virReportError(VIR_ERR_XML_ERROR, "%s", > - _("'wwnn' and 'wwpn' must be specified for adapter " > - "type 'fchost'")); > - return -1; > - } > + if (!virValidateWWN(ret->source.adapter.data.fchost.wwnn) || > + !virValidateWWN(ret->source.adapter.data.fchost.wwpn)) > + return -1; > > - if (!virValidateWWN(ret->source.adapter.data.fchost.wwnn) || > - !virValidateWWN(ret->source.adapter.data.fchost.wwpn)) > - return -1; > + if ((ret->source.adapter.data.fchost.parent_wwnn && > + !ret->source.adapter.data.fchost.parent_wwpn) || > + (!ret->source.adapter.data.fchost.parent_wwnn && > + ret->source.adapter.data.fchost.parent_wwpn)) { > + virReportError(VIR_ERR_XML_ERROR, "%s", > + _("must supply both parent_wwnn and " > + "parent_wwpn not just one or the other")); > + return -1; > + } > > - if ((ret->source.adapter.data.fchost.parent_wwnn && > - !ret->source.adapter.data.fchost.parent_wwpn) || > - (!ret->source.adapter.data.fchost.parent_wwnn && > - ret->source.adapter.data.fchost.parent_wwpn)) { > - virReportError(VIR_ERR_XML_ERROR, "%s", > - _("must supply both parent_wwnn and " > - "parent_wwpn not just one or the other")); > - return -1; > - } > + if (ret->source.adapter.data.fchost.parent_wwnn && > + !(ret->source.adapter.data.fchost.parent_wwnn)) It's a separate issue, but maybe this function should be named virWWNValidate (even though there is no object called "virWWN")

> + return -1; > > - if (ret->source.adapter.data.fchost.parent_wwnn && > - !virValidateWWN(ret->source.adapter.data.fchost.parent_wwnn)) > - return -1; > + if (ret->source.adapter.data.fchost.parent_wwpn && > + !virValidateWWN(ret->source.adapter.data.fchost.parent_wwpn)) > + return -1; > > - if (ret->source.adapter.data.fchost.parent_wwpn && > - !virValidateWWN(ret->source.adapter.data.fchost.parent_wwpn)) > - return -1; > + if (ret->source.adapter.data.fchost.parent_fabric_wwn && > + !virValidateWWN(ret->source.adapter.data.fchost.parent_fabric_wwn)) > + return -1; > > - if (ret->source.adapter.data.fchost.parent_fabric_wwn && > - !virValidateWWN(ret->source.adapter.data.fchost.parent_fabric_wwn)) > - return -1; > + return 0; > +} > > + > +int > +virStorageAdapterParseValidate(virStoragePoolDefPtr ret) > +{ > + if (!ret->source.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); > } else if (ret->source.adapter.type == > VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_SCSI_HOST) { > if (!ret->source.adapter.data.scsi_host.name && > @@ -244,6 +270,28 @@ virStorageAdapterParseValidate(virStoragePoolDefPtr ret) > } > > > +static void > +virStorageAdapterFCHostFormat(virBufferPtr buf, > + virStoragePoolSourcePtr src) > +{ > + virBufferEscapeString(buf, " parent='%s'", > + src->adapter.data.fchost.parent); > + if (src->adapter.data.fchost.managed) > + virBufferAsprintf(buf, " managed='%s'", > + virTristateBoolTypeToString(src->adapter.data.fchost.managed)); > + virBufferEscapeString(buf, " parent_wwnn='%s'", > + src->adapter.data.fchost.parent_wwnn); > + virBufferEscapeString(buf, " parent_wwpn='%s'", > + src->adapter.data.fchost.parent_wwpn); > + virBufferEscapeString(buf, " parent_fabric_wwn='%s'", > + src->adapter.data.fchost.parent_fabric_wwn); > + > + virBufferAsprintf(buf, " wwnn='%s' wwpn='%s'/>\n", > + src->adapter.data.fchost.wwnn, > + src->adapter.data.fchost.wwpn); > +} > + > + > void > virStorageAdapterFormat(virBufferPtr buf, > virStoragePoolSourcePtr src) > @@ -252,21 +300,7 @@ virStorageAdapterFormat(virBufferPtr buf, > virStoragePoolSourceAdapterTypeToString(src->adapter.type)); > > if (src->adapter.type == VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_FC_HOST) { > - virBufferEscapeString(buf, " parent='%s'", > - src->adapter.data.fchost.parent); > - if (src->adapter.data.fchost.managed) > - virBufferAsprintf(buf, " managed='%s'", > - virTristateBoolTypeToString(src->adapter.data.fchost.managed)); > - virBufferEscapeString(buf, " parent_wwnn='%s'", > - src->adapter.data.fchost.parent_wwnn); > - virBufferEscapeString(buf, " parent_wwpn='%s'", > - src->adapter.data.fchost.parent_wwpn); > - virBufferEscapeString(buf, " parent_fabric_wwn='%s'", > - src->adapter.data.fchost.parent_fabric_wwn); > - > - virBufferAsprintf(buf, " wwnn='%s' wwpn='%s'/>\n", > - src->adapter.data.fchost.wwnn, > - src->adapter.data.fchost.wwpn); > + virStorageAdapterFCHostFormat(buf, src); > } else if (src->adapter.type == > VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_SCSI_HOST) { > if (src->adapter.data.scsi_host.name) { ACK with "Parse" removed from the one validation functions name.