Re: [PATCH v3 08/18] conf: Extract SCSI adapter type processing into their own helpers

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxxx>
>>> ---
>>>  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@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list



[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]
  Powered by Linux