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
