9:02 p.m.
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>
---
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 &&
+ !virValidateWWN(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.