[Libvirt-cim] [PATCH] [TEST] Enabling 02_reverse.py with XenFV, KVM, LXC and modified logicaldevices.py to work for all virt types

[Kaitlin Rupert]

Deepti B Kalakeri wrote:
> 
> 
> Kaitlin Rupert wrote:
>>
>>> +def verify_eafp_values(server, virt, in_pllist, test_disk):
>>> + # Looping through the in_pllist to get association for various pools.
>>> + eafp_values = eafp_list(virt, test_disk)
>>> + an = get_typed_class(virt, "ElementAllocatedFromPool")
>>> + for cn, instid in sorted(in_pllist.iteritems()):
>>> + try:
>>> + assoc_info = Associators(server, an, cn, virt, InstanceID = instid)
>>> + assoc_inst_list = get_inst_for_dom(assoc_info)
>>> + if len(assoc_inst_list) < 1 :
>>> + logger.error("'%s' with '%s' did not return any records for"
>>> + " domain: '%s'", an, cn, test_dom)
>>
>> This should be;
>>
>> logger.error("'%s' should have returned '%i' Processor"
>> " details, got '%i'" % (an, test_vcpus,
>> len(assoc_inst_list)))
>>
>> If you use commas instead, the formatting of the string is off.
>>
> The above log stmt is just not for Processor.

Oops, you are right.  Sorry, I had copied this from a different part of 
the code and pasted it here accidentally.  It should be:

logger.error("'%s' with '%s' did not return any records for"
              " domain: '%s'" % (an, cn, test_dom))

  I will try to make it better.
> I did not find any formatting difference by using Commas and I got the 
> same o/p for with/without commas.
> Did I misunderstand something ?

You're right, it is printing correctly.  In the past, I'd seen the 
string not be formatted correctly with the arguments passed this way. 
But maybe that was an old issue.  It appears to be working fine now.

You can ignore this comment. =)

>>>
>>> -def verify_proc_values(assoc_info, list_values):
>>
>> Instead of having a verify_<>_values function for each device type, 
>> could these be condensed into one function? All of them check the 
>> CreationClassName and the DeviceID, which isn't needed. The function 
>> could take the classname, which would enable you to determine with 
>> values need to be checked.
>>
>>
>>
> I think DeviceID is a very important field that needs to be checked. Did 
> you mean I should skip checking CreationClassName and DeviceID and check 
> only Device specific details ?

I agree - I think checking the DeviceID is important.  What I meant is 
that it's redundant to have each function checking the same value.

I think these could be consolidated into something like:

def verify_device_values(assoc_info, list_values, cn, virt='Xen'):
     values = list_values[cn]
     if assoc_info['CreationClassName'] != values['CreationClassName']:
         field_err(assoc_info, values, fieldname = 'CreationClassName')
         return FAIL
     if assoc_info['DeviceID'] != values['DeviceID']:
         field_err(assoc_info, values, fieldname = 'DeviceID')
         return FAIL

     if cn == get_typed_class(virt, 'Processor'):
         sysname = assoc_info['SystemName']
         if sysname != values['SystemName']:
             spec_err(sysname, values, fieldname = 'SystemName')
             return FAIL

     # Handle other device types here

     return PASS

If you think it's cleaner, you could have something like:

     if cn == get_typed_class(virt, 'Processor'):
         return verify_proc_values()

Either approach is fine.  Creating one function (or a wrapper function) 
will clean up the elif portion of verify_eafp_values() some.

Thoughts?

-- 
Kaitlin Rupert
IBM Linux Technology Center
kaitlin at linux.vnet.ibm.com