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
