On 03/11/2017 10:02 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 > > 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 <jfer...@redhat.com> >> --- >> 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. >
Right as you found out I had it in mind... Of course it's a matter of order! >> +{ >> + 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. > I'll change the name to be ValidateParse rather than ParseValidate >> { >> - 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") > > Well technically I suppose it should be virUtilValidateWWN... We could enter the theatre of the absurd though just generating patches to match some function name algorithm! John >> + 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. > > -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list