Re: [libvirt] [PATCH v3 03/18] conf: Split out storage pool source adapter helpers

On 03/10/2017 04:10 PM, John Ferlan wrote: > Split out the code that munges through the storage pool adapter into > helpers - it's about to be moved into it's own source file. > > This is purely code motion at this point. > > Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; > --- > src/conf/storage_conf.c | 455 ++++++++++++++++++++++++++---------------------- > 1 file changed, 243 insertions(+), 212 deletions(-) > > diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c > index 8e3b175..1993d3a 100644 > --- a/src/conf/storage_conf.c > +++ b/src/conf/storage_conf.c > @@ -463,6 +463,128 @@ virStoragePoolObjRemove(virStoragePoolObjListPtr pools, > } > > static int > +virStoragePoolDefParseSourceAdapter(virStoragePoolSourcePtr source, > + xmlXPathContextPtr ctxt) > +{ > + int ret = -1; > + char *adapter_type = NULL; > + char *managed = NULL; > + > + if ((adapter_type = virXPathString("string(./adapter/@type)", ctxt))) { > + if ((source->adapter.type = > + virStoragePoolSourceAdapterTypeFromString(adapter_type)) <= 0) { > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, > + _("Unknown pool adapter type '%s'"), > + adapter_type); > + goto cleanup; > + } > + > + if (source->adapter.type == > + VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_FC_HOST) { > + source->adapter.data.fchost.parent = > + virXPathString("string(./adapter/@parent)", ctxt); > + managed = virXPathString("string(./adapter/@managed)", ctxt); > + 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 = > + virXPathString("string(./adapter/@parent_wwnn)", ctxt); > + source->adapter.data.fchost.parent_wwpn = > + virXPathString("string(./adapter/@parent_wwpn)", ctxt); > + source->adapter.data.fchost.parent_fabric_wwn = > + virXPathString("string(./adapter/@parent_fabric_wwn)", ctxt); > + > + source->adapter.data.fchost.wwpn = > + virXPathString("string(./adapter/@wwpn)", ctxt); > + source->adapter.data.fchost.wwnn = > + virXPathString("string(./adapter/@wwnn)", ctxt); > + } else if (source->adapter.type == > + VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_SCSI_HOST) { > + > + source->adapter.data.scsi_host.name = > + virXPathString("string(./adapter/@name)", ctxt); > + if (virXPathNode("./adapter/parentaddr", ctxt)) { > + xmlNodePtr addrnode = virXPathNode("./adapter/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")); > + goto cleanup; > + } > + source->adapter.data.scsi_host.has_parent = true; > + if (virPCIDeviceAddressParseXML(addrnode, addr) < 0) > + goto cleanup; > + if ((virXPathInt("string(./adapter/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")); > + goto cleanup; > + } > + } > + } > + } else { > + char *wwnn = NULL; > + char *wwpn = NULL; > + char *parent = NULL; > + > + /* "type" was not specified in the XML, so we must verify that > + * "wwnn", "wwpn", "parent", or "parentaddr" are also not in the > + * XML. If any are found, then we cannot just use "name" alone". > + */ > + wwnn = virXPathString("string(./adapter/@wwnn)", ctxt); > + wwpn = virXPathString("string(./adapter/@wwpn)", ctxt); > + parent = virXPathString("string(./adapter/@parent)", ctxt); > + > + if (wwnn || wwpn || parent) { > + VIR_FREE(wwnn); > + VIR_FREE(wwpn); > + VIR_FREE(parent); > + virReportError(VIR_ERR_XML_ERROR, "%s", > + _("Use of 'wwnn', 'wwpn', and 'parent' attributes " > + "requires use of the adapter 'type'")); > + goto cleanup; > + } > + > + if (virXPathNode("./adapter/parentaddr", ctxt)) { > + virReportError(VIR_ERR_XML_ERROR, "%s", > + _("Use of 'parent' element requires use " > + "of the adapter 'type'")); > + goto cleanup; > + } > + > + /* To keep back-compat, 'type' is not required to specify > + * for scsi_host adapter. > + */ > + if ((source->adapter.data.scsi_host.name = > + virXPathString("string(./adapter/@name)", ctxt))) > + source->adapter.type = > + VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_SCSI_HOST; > + } > + > + ret = 0; > + > + cleanup: > + VIR_FREE(adapter_type); > + VIR_FREE(managed); > + return ret; > +} > + > + > +static int > virStoragePoolDefParseSource(xmlXPathContextPtr ctxt, > virStoragePoolSourcePtr source, > int pool_type, > @@ -476,8 +598,6 @@ virStoragePoolDefParseSource(xmlXPathContextPtr ctxt, > virStorageAuthDefPtr authdef = NULL; > char *name = NULL; > char *port = NULL; > - char *adapter_type = NULL; > - char *managed = NULL; > int n; > > relnode = ctxt->node; > @@ -583,110 +703,8 @@ virStoragePoolDefParseSource(xmlXPathContextPtr ctxt, > VIR_STRDUP(source->dir, "/") < 0) > goto cleanup; > > - if ((adapter_type = virXPathString("string(./adapter/@type)", ctxt))) { > - if ((source->adapter.type = > - virStoragePoolSourceAdapterTypeFromString(adapter_type)) <= 0) { > - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, > - _("Unknown pool adapter type '%s'"), > - adapter_type); > - goto cleanup; > - } > - > - if (source->adapter.type == > - VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_FC_HOST) { > - source->adapter.data.fchost.parent = > - virXPathString("string(./adapter/@parent)", ctxt); > - managed = virXPathString("string(./adapter/@managed)", ctxt); > - 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 = > - virXPathString("string(./adapter/@parent_wwnn)", ctxt); > - source->adapter.data.fchost.parent_wwpn = > - virXPathString("string(./adapter/@parent_wwpn)", ctxt); > - source->adapter.data.fchost.parent_fabric_wwn = > - virXPathString("string(./adapter/@parent_fabric_wwn)", ctxt); > - > - source->adapter.data.fchost.wwpn = > - virXPathString("string(./adapter/@wwpn)", ctxt); > - source->adapter.data.fchost.wwnn = > - virXPathString("string(./adapter/@wwnn)", ctxt); > - } else if (source->adapter.type == > - VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_SCSI_HOST) { > - > - source->adapter.data.scsi_host.name = > - virXPathString("string(./adapter/@name)", ctxt); > - if (virXPathNode("./adapter/parentaddr", ctxt)) { > - xmlNodePtr addrnode = virXPathNode("./adapter/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")); > - goto cleanup; > - } > - source->adapter.data.scsi_host.has_parent = true; > - if (virPCIDeviceAddressParseXML(addrnode, addr) < 0) > - goto cleanup; > - if ((virXPathInt("string(./adapter/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")); > - goto cleanup; > - } > - } > - } > - } else { > - char *wwnn = NULL; > - char *wwpn = NULL; > - char *parent = NULL; > - > - /* "type" was not specified in the XML, so we must verify that > - * "wwnn", "wwpn", "parent", or "parentaddr" are also not in the > - * XML. If any are found, then we cannot just use "name" alone". > - */ > - wwnn = virXPathString("string(./adapter/@wwnn)", ctxt); > - wwpn = virXPathString("string(./adapter/@wwpn)", ctxt); > - parent = virXPathString("string(./adapter/@parent)", ctxt); > - > - if (wwnn || wwpn || parent) { > - VIR_FREE(wwnn); > - VIR_FREE(wwpn); > - VIR_FREE(parent); > - virReportError(VIR_ERR_XML_ERROR, "%s", > - _("Use of 'wwnn', 'wwpn', and 'parent' attributes " > - "requires use of the adapter 'type'")); > - goto cleanup; > - } > - > - if (virXPathNode("./adapter/parentaddr", ctxt)) { > - virReportError(VIR_ERR_XML_ERROR, "%s", > - _("Use of 'parent' element requires use " > - "of the adapter 'type'")); > - goto cleanup; > - } > - > - /* To keep back-compat, 'type' is not required to specify > - * for scsi_host adapter. > - */ > - if ((source->adapter.data.scsi_host.name = > - virXPathString("string(./adapter/@name)", ctxt))) > - source->adapter.type = > - VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_SCSI_HOST; > - } > + if (virStoragePoolDefParseSourceAdapter(source, ctxt) < 0) > + goto cleanup; > > if ((authnode = virXPathNode("./auth", ctxt))) { > if (!(authdef = virStorageAuthDefParse(node->doc, authnode))) > @@ -711,8 +729,6 @@ virStoragePoolDefParseSource(xmlXPathContextPtr ctxt, > > VIR_FREE(port); > VIR_FREE(nodeset); > - VIR_FREE(adapter_type); > - VIR_FREE(managed); > virStorageAuthDefFree(authdef); > return ret; > } > @@ -831,6 +847,74 @@ virStorageDefParsePerms(xmlXPathContextPtr ctxt, > return ret; > } > > +static int > +virStoragePoolSourceAdapterParseValidate(virStoragePoolDefPtr ret) (looking ahead, I see that at least some of this discussion is moot because you're moving/renaming the same stuff again in later patches, so take it for what its worth...)

Since function naming is a hot topic these days.... The first new function you created operates on a virStoragePoolSourcePtr and is named: virStoragePoolDef Parse SourceAdapter This function operates on a virStoragePoolDefPtr and is named: virStoragePoolSourceAdapter Parse Validate So they are inconsistent about the subject of the function name vs. the argument that is passed. In the first case, it looks like all uses of the virStoragePoolSourcePtr "source" actually use "source->adapter", so you could change the function to take a virStoragePoolSourceAdapter as its arg (and then either leave it in virSubjectVerbObject format as it is, or change it to virStoragePoolSourceAdapterParse()). In the 2nd case, I think the "Parse" part is incorrect, since it's only Validating the data, not parsing it, and also, every use of the virStoragePoolDefPtr ("ret") is actually using "ret->source.adapter", so again the arg could be changed to match what's implied in the function name. But maybe you're just doing this to make it easier to verify that it's plain code motion (and that definitely *was* a help :-)).

ACK based on the assumption that the arguments and function names will be made more appropriate in later patches.