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.
Signed-off-by: John Ferlan <jferlan@redhat.com>
---
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);
- }
}
@@ -95,6 +94,83 @@ virStorageAdapterFCHostParseXML(xmlNodePtr node,
}
+static int
+virStorageAdapterSCSIHostParseXML(xmlNodePtr node,
+ xmlXPathContextPtr ctxt,
+ virStoragePoolSourcePtr source)
+{
+ 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,
+ xmlXPathContextPtr ctxt,
+ virStoragePoolSourcePtr source)
+{
+ char *wwnn = virXMLPropString(node, "wwnn");
+ char *wwpn = virXMLPropString(node, "wwpn");
+ char *parent = virXMLPropString(node, "parent");
+
+ /* "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".
+ */
+ 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'"));
+ return -1;
+ }
+
+ if (virXPathNode("./parentaddr", ctxt)) {
+ virReportError(VIR_ERR_XML_ERROR, "%s",
+ _("Use of 'parent' element requires use "
+ "of the adapter 'type'"));
+ return -1;
+ }
+
+ /* To keep back-compat, 'type' is not required to specify
+ * for scsi_host adapter.
+ */
+ if ((source->adapter.data.scsi_host.name =
+ virXMLPropString(node, "name")))
+ source->adapter.type =
+ VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_SCSI_HOST;
+
+ return 0;
+}
+
+
int
virStorageAdapterParseXML(virStoragePoolSourcePtr source,
xmlNodePtr node,
@@ -121,68 +197,13 @@ virStorageAdapterParseXML(virStoragePoolSourcePtr source,
goto cleanup;
} else if (source->adapter.type ==
VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_SCSI_HOST) {
+ if (virStorageAdapterSCSIHostParseXML(node, ctxt, source) < 0)
+ goto cleanup;
- 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"));
- goto cleanup;
- }
- source->adapter.data.scsi_host.has_parent = true;
- if (virPCIDeviceAddressParseXML(addrnode, addr) < 0)
- goto cleanup;
- 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"));
- goto cleanup;
- }
- }
}
} else {
- char *wwnn = virXMLPropString(node, "wwnn");
- char *wwpn = virXMLPropString(node, "wwpn");
- char *parent = virXMLPropString(node, "parent");
-
- /* "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".
- */
-
- 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'"));
+ if (virStorageAdapterLegacyParseXML(node, ctxt, source) < 0)
goto cleanup;
- }
-
- if (virXPathNode("./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 =
- virXMLPropString(node, "name")))
- source->adapter.type =
- VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_SCSI_HOST;
}
ret = 0;
@@ -235,6 +256,29 @@ virStorageAdapterFCHostParseValidate(virStoragePoolDefPtr ret)
}
+static int
+virStorageAdapterSCSIHostParseValidate(virStoragePoolDefPtr ret)
+{
+ if (!ret->source.adapter.data.scsi_host.name &&
+ !ret->source.adapter.data.scsi_host.has_parent) {
+ virReportError(VIR_ERR_XML_ERROR, "%s",
+ _("Either 'name' or 'parent' must be specified "
+ "for the 'scsi_host' adapter"));
+ return -1;
+ }
+
+ if (ret->source.adapter.data.scsi_host.name &&
+ ret->source.adapter.data.scsi_host.has_parent) {
+ virReportError(VIR_ERR_XML_ERROR, "%s",
+ _("Both 'name' and 'parent' cannot be specified "
+ "for the 'scsi_host' adapter"));
+ return -1;
+ }
+
+ return 0;
+}
+
+
int
virStorageAdapterParseValidate(virStoragePoolDefPtr ret)
{
@@ -245,26 +289,12 @@ virStorageAdapterParseValidate(virStoragePoolDefPtr ret)
}
if (ret->source.adapter.type ==
- VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_FC_HOST) {
+ 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 &&
- !ret->source.adapter.data.scsi_host.has_parent) {
- virReportError(VIR_ERR_XML_ERROR, "%s",
- _("Either 'name' or 'parent' must be specified "
- "for the 'scsi_host' adapter"));
- return -1;
- }
- if (ret->source.adapter.data.scsi_host.name &&
- ret->source.adapter.data.scsi_host.has_parent) {
- virReportError(VIR_ERR_XML_ERROR, "%s",
- _("Both 'name' and 'parent' cannot be specified "
- "for the 'scsi_host' adapter"));
- return -1;
- }
- }
+ if (ret->source.adapter.type ==
+ VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_SCSI_HOST)
+ return virStorageAdapterSCSIHostParseValidate(ret);
return 0;
}
@@ -292,6 +322,30 @@ virStorageAdapterFCHostFormat(virBufferPtr buf,
}
+static void
+virStorageAdapterSCSIHostFormat(virBufferPtr buf,
+ virStoragePoolSourcePtr src)
+{
+ if (src->adapter.data.scsi_host.name) {
+ virBufferAsprintf(buf, " name='%s'/>\n",
+ src->adapter.data.scsi_host.name);
+ } else {
+ virPCIDeviceAddress addr;
+ virBufferAddLit(buf, ">\n");
+ virBufferAdjustIndent(buf, 2);
+ virBufferAsprintf(buf, "<parentaddr unique_id='%d'>\n",
+ src->adapter.data.scsi_host.unique_id);
+ virBufferAdjustIndent(buf, 2);
+ addr = src->adapter.data.scsi_host.parentaddr;
+ ignore_value(virPCIDeviceAddressFormat(buf, addr, false));
+ virBufferAdjustIndent(buf, -2);
+ virBufferAddLit(buf, "</parentaddr>\n");
+ virBufferAdjustIndent(buf, -2);
+ virBufferAddLit(buf, "</adapter>\n");
+ }
+}
+
+
void
virStorageAdapterFormat(virBufferPtr buf,
virStoragePoolSourcePtr src)
@@ -299,26 +353,9 @@ virStorageAdapterFormat(virBufferPtr buf,
virBufferAsprintf(buf, "<adapter type='%s'",
virStoragePoolSourceAdapterTypeToString(src->adapter.type));
- if (src->adapter.type == VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_FC_HOST) {
+ if (src->adapter.type == VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_FC_HOST)
virStorageAdapterFCHostFormat(buf, src);
- } else if (src->adapter.type ==
- VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_SCSI_HOST) {
- if (src->adapter.data.scsi_host.name) {
- virBufferAsprintf(buf, " name='%s'/>\n",
- src->adapter.data.scsi_host.name);
- } else {
- virPCIDeviceAddress addr;
- virBufferAddLit(buf, ">\n");
- virBufferAdjustIndent(buf, 2);
- virBufferAsprintf(buf, "<parentaddr unique_id='%d'>\n",
- src->adapter.data.scsi_host.unique_id);
- virBufferAdjustIndent(buf, 2);
- addr = src->adapter.data.scsi_host.parentaddr;
- ignore_value(virPCIDeviceAddressFormat(buf, addr, false));
- virBufferAdjustIndent(buf, -2);
- virBufferAddLit(buf, "</parentaddr>\n");
- virBufferAdjustIndent(buf, -2);
- virBufferAddLit(buf, "</adapter>\n");
- }
- }
+
+ if (src->adapter.type == VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_SCSI_HOST)
+ virStorageAdapterSCSIHostFormat(buf, src);
}
--
2.9.3
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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@redhat.com>
> ---
> 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
> - }
> }
>
>
> @@ -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].
> +{
> + 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?
> + xmlXPathContextPtr ctxt,
> + virStoragePoolSourcePtr source)
Same.
> +{
> + char *wwnn = virXMLPropString(node, "wwnn");
> + char *wwpn = virXMLPropString(node, "wwpn");
> + char *parent = virXMLPropString(node, "parent");
> +
> + /* "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".
> + */
> + 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'"));
> + return -1;
> + }
> +
> + if (virXPathNode("./parentaddr", ctxt)) {
> + virReportError(VIR_ERR_XML_ERROR, "%s",
> + _("Use of 'parent' element requires use "
> + "of the adapter 'type'"));
> + return -1;
> + }
> +
> + /* To keep back-compat, 'type' is not required to specify
> + * for scsi_host adapter.
> + */
> + if ((source->adapter.data.scsi_host.name =
> + virXMLPropString(node, "name")))
> + source->adapter.type =
> + VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_SCSI_HOST;
> +
> + return 0;
> +}
> +
> +
> int
> virStorageAdapterParseXML(virStoragePoolSourcePtr source,
> xmlNodePtr node,
> @@ -121,68 +197,13 @@ virStorageAdapterParseXML(virStoragePoolSourcePtr source,
> goto cleanup;
> } else if (source->adapter.type ==
> VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_SCSI_HOST) {
> + if (virStorageAdapterSCSIHostParseXML(node, ctxt, source) < 0)
> + goto cleanup;
>
> - 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"));
> - goto cleanup;
> - }
> - source->adapter.data.scsi_host.has_parent = true;
> - if (virPCIDeviceAddressParseXML(addrnode, addr) < 0)
> - goto cleanup;
> - 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"));
> - goto cleanup;
> - }
> - }
> }
> } else {
> - char *wwnn = virXMLPropString(node, "wwnn");
> - char *wwpn = virXMLPropString(node, "wwpn");
> - char *parent = virXMLPropString(node, "parent");
> -
> - /* "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".
> - */
> -
> - 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'"));
> + if (virStorageAdapterLegacyParseXML(node, ctxt, source) < 0)
> goto cleanup;
> - }
> -
> - if (virXPathNode("./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 =
> - virXMLPropString(node, "name")))
> - source->adapter.type =
> - VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_SCSI_HOST;
> }
>
> ret = 0;
> @@ -235,6 +256,29 @@ virStorageAdapterFCHostParseValidate(virStoragePoolDefPtr ret)
> }
>
>
> +static int
> +virStorageAdapterSCSIHostParseValidate(virStoragePoolDefPtr ret)
Again with the "ParseValidate". I see validation, but I don't see any parsing. (yeah, I know it's just because the calling function is named virBlahParseValidate - that needs to be fixed too)
> +{
> + if (!ret->source.adapter.data.scsi_host.name &&
> + !ret->source.adapter.data.scsi_host.has_parent) {
> + virReportError(VIR_ERR_XML_ERROR, "%s",
> + _("Either 'name' or 'parent' must be specified "
> + "for the 'scsi_host' adapter"));
> + return -1;
> + }
> +
> + if (ret->source.adapter.data.scsi_host.name &&
> + ret->source.adapter.data.scsi_host.has_parent) {
> + virReportError(VIR_ERR_XML_ERROR, "%s",
> + _("Both 'name' and 'parent' cannot be specified "
> + "for the 'scsi_host' adapter"));
> + return -1;
> + }
> +
> + return 0;
> +}
> +
> +
> int
> virStorageAdapterParseValidate(virStoragePoolDefPtr ret)
> {
> @@ -245,26 +289,12 @@ virStorageAdapterParseValidate(virStoragePoolDefPtr ret)
> }
>
> if (ret->source.adapter.type ==
> - VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_FC_HOST) {
> + 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 &&
> - !ret->source.adapter.data.scsi_host.has_parent) {
> - virReportError(VIR_ERR_XML_ERROR, "%s",
> - _("Either 'name' or 'parent' must be specified "
> - "for the 'scsi_host' adapter"));
> - return -1;
> - }
>
> - if (ret->source.adapter.data.scsi_host.name &&
> - ret->source.adapter.data.scsi_host.has_parent) {
> - virReportError(VIR_ERR_XML_ERROR, "%s",
> - _("Both 'name' and 'parent' cannot be specified "
> - "for the 'scsi_host' adapter"));
> - return -1;
> - }
> - }
> + if (ret->source.adapter.type ==
> + VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_SCSI_HOST)
> + return virStorageAdapterSCSIHostParseValidate(ret);
>
> return 0;
> }
> @@ -292,6 +322,30 @@ virStorageAdapterFCHostFormat(virBufferPtr buf,
> }
>
>
> +static void
> +virStorageAdapterSCSIHostFormat(virBufferPtr buf,
> + virStoragePoolSourcePtr src)
I didn't mention it for a couple of the functions up above, but in general *all* of these need to change their arg to be as specific as possible (which I guess is virStoragePoolSourceAdapterPtr)
> +{
> + if (src->adapter.data.scsi_host.name) {
> + virBufferAsprintf(buf, " name='%s'/>\n",
> + src->adapter.data.scsi_host.name);
> + } else {
> + virPCIDeviceAddress addr;
> + virBufferAddLit(buf, ">\n");
> + virBufferAdjustIndent(buf, 2);
> + virBufferAsprintf(buf, "<parentaddr unique_id='%d'>\n",
> + src->adapter.data.scsi_host.unique_id);
> + virBufferAdjustIndent(buf, 2);
> + addr = src->adapter.data.scsi_host.parentaddr;
> + ignore_value(virPCIDeviceAddressFormat(buf, addr, false));
> + virBufferAdjustIndent(buf, -2);
> + virBufferAddLit(buf, "</parentaddr>\n");
> + virBufferAdjustIndent(buf, -2);
> + virBufferAddLit(buf, "</adapter>\n");
> + }
> +}
> +
> +
> void
> virStorageAdapterFormat(virBufferPtr buf,
> virStoragePoolSourcePtr src)
> @@ -299,26 +353,9 @@ virStorageAdapterFormat(virBufferPtr buf,
> virBufferAsprintf(buf, "<adapter type='%s'",
> virStoragePoolSourceAdapterTypeToString(src->adapter.type));
>
> - if (src->adapter.type == VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_FC_HOST) {
> + if (src->adapter.type == VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_FC_HOST)
> virStorageAdapterFCHostFormat(buf, src);
> - } else if (src->adapter.type ==
> - VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_SCSI_HOST) {
> - if (src->adapter.data.scsi_host.name) {
> - virBufferAsprintf(buf, " name='%s'/>\n",
> - src->adapter.data.scsi_host.name);
> - } else {
> - virPCIDeviceAddress addr;
> - virBufferAddLit(buf, ">\n");
> - virBufferAdjustIndent(buf, 2);
> - virBufferAsprintf(buf, "<parentaddr unique_id='%d'>\n",
> - src->adapter.data.scsi_host.unique_id);
> - virBufferAdjustIndent(buf, 2);
> - addr = src->adapter.data.scsi_host.parentaddr;
> - ignore_value(virPCIDeviceAddressFormat(buf, addr, false));
> - virBufferAdjustIndent(buf, -2);
> - virBufferAddLit(buf, "</parentaddr>\n");
> - virBufferAdjustIndent(buf, -2);
> - virBufferAddLit(buf, "</adapter>\n");
> - }
> - }
> +
> + if (src->adapter.type == VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_SCSI_HOST)
> + virStorageAdapterSCSIHostFormat(buf, src);
> }
I think this patch is okay if the args are changed to virStoragePoolSourceAdapterPtr, "Parse" is removed from the Validate function's name, and the use of "Legacy" in the one function is explained in a comment.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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@redhat.com>
>> ---
>> 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
>> + xmlXPathContextPtr ctxt,
>> + virStoragePoolSourcePtr source)
>
> Same.
>
>> +{
>> + char *wwnn = virXMLPropString(node, "wwnn");
>> + char *wwpn = virXMLPropString(node, "wwpn");
>> + char *parent = virXMLPropString(node, "parent");
>> +
>> + /* "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".
>> + */
>> + 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'"));
>> + return -1;
>> + }
>> +
>> + if (virXPathNode("./parentaddr", ctxt)) {
>> + virReportError(VIR_ERR_XML_ERROR, "%s",
>> + _("Use of 'parent' element requires use "
>> + "of the adapter 'type'"));
>> + return -1;
>> + }
>> +
>> + /* To keep back-compat, 'type' is not required to specify
>> + * for scsi_host adapter.
>> + */
>> + if ((source->adapter.data.scsi_host.name =
>> + virXMLPropString(node, "name")))
>> + source->adapter.type =
>> + VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_SCSI_HOST;
>> +
>> + return 0;
>> +}
>> +
>> +
>> int
>> virStorageAdapterParseXML(virStoragePoolSourcePtr source,
>> xmlNodePtr node,
>> @@ -121,68 +197,13 @@ virStorageAdapterParseXML(virStoragePoolSourcePtr source,
>> goto cleanup;
>> } else if (source->adapter.type ==
>> VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_SCSI_HOST) {
>> + if (virStorageAdapterSCSIHostParseXML(node, ctxt, source) < 0)
>> + goto cleanup;
>>
>> - 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"));
>> - goto cleanup;
>> - }
>> - source->adapter.data.scsi_host.has_parent = true;
>> - if (virPCIDeviceAddressParseXML(addrnode, addr) < 0)
>> - goto cleanup;
>> - 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"));
>> - goto cleanup;
>> - }
>> - }
>> }
>> } else {
>> - char *wwnn = virXMLPropString(node, "wwnn");
>> - char *wwpn = virXMLPropString(node, "wwpn");
>> - char *parent = virXMLPropString(node, "parent");
>> -
>> - /* "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".
>> - */
>> -
>> - 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'"));
>> + if (virStorageAdapterLegacyParseXML(node, ctxt, source) < 0)
>> goto cleanup;
>> - }
>> -
>> - if (virXPathNode("./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 =
>> - virXMLPropString(node, "name")))
>> - source->adapter.type =
>> - VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_SCSI_HOST;
>> }
>>
>> ret = 0;
>> @@ -235,6 +256,29 @@ virStorageAdapterFCHostParseValidate(virStoragePoolDefPtr ret)
>> }
>>
>>
>> +static int
>> +virStorageAdapterSCSIHostParseValidate(virStoragePoolDefPtr ret)
>
> Again with the "ParseValidate". I see validation, but I don't see any parsing. (yeah, I know it's just because the calling function is named virBlahParseValidate - that needs to be fixed too)
>
Similar I'll modify to ValidateParse
John
>
>> +{
>> + if (!ret->source.adapter.data.scsi_host.name &&
>> + !ret->source.adapter.data.scsi_host.has_parent) {
>> + virReportError(VIR_ERR_XML_ERROR, "%s",
>> + _("Either 'name' or 'parent' must be specified "
>> + "for the 'scsi_host' adapter"));
>> + return -1;
>> + }
>> +
>> + if (ret->source.adapter.data.scsi_host.name &&
>> + ret->source.adapter.data.scsi_host.has_parent) {
>> + virReportError(VIR_ERR_XML_ERROR, "%s",
>> + _("Both 'name' and 'parent' cannot be specified "
>> + "for the 'scsi_host' adapter"));
>> + return -1;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +
>> int
>> virStorageAdapterParseValidate(virStoragePoolDefPtr ret)
>> {
>> @@ -245,26 +289,12 @@ virStorageAdapterParseValidate(virStoragePoolDefPtr ret)
>> }
>>
>> if (ret->source.adapter.type ==
>> - VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_FC_HOST) {
>> + 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 &&
>> - !ret->source.adapter.data.scsi_host.has_parent) {
>> - virReportError(VIR_ERR_XML_ERROR, "%s",
>> - _("Either 'name' or 'parent' must be specified "
>> - "for the 'scsi_host' adapter"));
>> - return -1;
>> - }
>>
>> - if (ret->source.adapter.data.scsi_host.name &&
>> - ret->source.adapter.data.scsi_host.has_parent) {
>> - virReportError(VIR_ERR_XML_ERROR, "%s",
>> - _("Both 'name' and 'parent' cannot be specified "
>> - "for the 'scsi_host' adapter"));
>> - return -1;
>> - }
>> - }
>> + if (ret->source.adapter.type ==
>> + VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_SCSI_HOST)
>> + return virStorageAdapterSCSIHostParseValidate(ret);
>>
>> return 0;
>> }
>> @@ -292,6 +322,30 @@ virStorageAdapterFCHostFormat(virBufferPtr buf,
>> }
>>
>>
>> +static void
>> +virStorageAdapterSCSIHostFormat(virBufferPtr buf,
>> + virStoragePoolSourcePtr src)
>
> I didn't mention it for a couple of the functions up above, but in general *all* of these need to change their arg to be as specific as possible (which I guess is virStoragePoolSourceAdapterPtr)
>
>> +{
>> + if (src->adapter.data.scsi_host.name) {
>> + virBufferAsprintf(buf, " name='%s'/>\n",
>> + src->adapter.data.scsi_host.name);
>> + } else {
>> + virPCIDeviceAddress addr;
>> + virBufferAddLit(buf, ">\n");
>> + virBufferAdjustIndent(buf, 2);
>> + virBufferAsprintf(buf, "<parentaddr unique_id='%d'>\n",
>> + src->adapter.data.scsi_host.unique_id);
>> + virBufferAdjustIndent(buf, 2);
>> + addr = src->adapter.data.scsi_host.parentaddr;
>> + ignore_value(virPCIDeviceAddressFormat(buf, addr, false));
>> + virBufferAdjustIndent(buf, -2);
>> + virBufferAddLit(buf, "</parentaddr>\n");
>> + virBufferAdjustIndent(buf, -2);
>> + virBufferAddLit(buf, "</adapter>\n");
>> + }
>> +}
>> +
>> +
>> void
>> virStorageAdapterFormat(virBufferPtr buf,
>> virStoragePoolSourcePtr src)
>> @@ -299,26 +353,9 @@ virStorageAdapterFormat(virBufferPtr buf,
>> virBufferAsprintf(buf, "<adapter type='%s'",
>> virStoragePoolSourceAdapterTypeToString(src->adapter.type));
>>
>> - if (src->adapter.type == VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_FC_HOST) {
>> + if (src->adapter.type == VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_FC_HOST)
>> virStorageAdapterFCHostFormat(buf, src);
>> - } else if (src->adapter.type ==
>> - VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_SCSI_HOST) {
>> - if (src->adapter.data.scsi_host.name) {
>> - virBufferAsprintf(buf, " name='%s'/>\n",
>> - src->adapter.data.scsi_host.name);
>> - } else {
>> - virPCIDeviceAddress addr;
>> - virBufferAddLit(buf, ">\n");
>> - virBufferAdjustIndent(buf, 2);
>> - virBufferAsprintf(buf, "<parentaddr unique_id='%d'>\n",
>> - src->adapter.data.scsi_host.unique_id);
>> - virBufferAdjustIndent(buf, 2);
>> - addr = src->adapter.data.scsi_host.parentaddr;
>> - ignore_value(virPCIDeviceAddressFormat(buf, addr, false));
>> - virBufferAdjustIndent(buf, -2);
>> - virBufferAddLit(buf, "</parentaddr>\n");
>> - virBufferAdjustIndent(buf, -2);
>> - virBufferAddLit(buf, "</adapter>\n");
>> - }
>> - }
>> +
>> + if (src->adapter.type == VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_SCSI_HOST)
>> + virStorageAdapterSCSIHostFormat(buf, src);
>> }
>
>
> I think this patch is okay if the args are changed to virStoragePoolSourceAdapterPtr, "Parse" is removed from the Validate function's name, and the use of "Legacy" in the one function is explained in a comment.
>
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 03/14/2017 07:05 PM, John Ferlan wrote:
>
> 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@redhat.com>
>>> ---
>>> 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?
With a typecast in the switch. Don't forget the typecast in the switch!
>
>>> - }
>>> }
>>>
>>>
>>> @@ -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
Okay, some sort of comment about that would be appreciated. Looks like I hadn't actually said the magic word anywhere in my original response. Now that I know the argument types are heading somewhere in later patches, I can say "ACK with the *ParseValidate* name 'fixed', and a short comment added stating what is the "Legacy" being accounted for".
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2026 Red Hat, Inc.