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 <[email protected]>
>> ---
>> 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
[email protected]
https://www.redhat.com/mailman/listinfo/libvir-list
