[Libvirt-cim] [PATCH] [TEST] #2 Updating 01_forward.py of EAFP

[Deepti B Kalakeri]

Kaitlin Rupert wrote:
>> +def get_inst(server, virt, cn, key, key_list):
>
> The key param is never used in this function.
>
>> + inst = None + try:
>> + inst = getInstance(server, cn, key_list, virt)
>>
>
>> +
>> +def init_list(server, virt, vsxml, test_disk):
>> + lelist = {}
>> + if virt != 'LXC':
>> + cn_keys_list = { + "LogicalDisk" : "%s/%s" % (test_dom, test_disk),
>> + "Memory" : "%s/%s" % (test_dom, "mem"),
>> + "NetworkPort" : "%s/%s" % (test_dom, test_mac),
>> + "Processor" : "%s/%s" % (test_dom, "0")
>> + }
>> + else:
>> + cn_keys_list = { + "Memory" : "%s/%s" % (test_dom, "mem"),
>> + }
>> +
>> + for cname, id in cn_keys_list.items():
>> + key_list = get_keys(virt, cname, id)
>
> Since the SSN and SN don't change, you could do the following instead:
>
> 1) Declare ssn somewhere before the for loop.
> 2) Declare the following above the for loop:
>
> key_list = { 'DeviceID' : None,
> 'CreationClassName' : None,
> 'SystemName' : test_dom,
> 'SystemCreationClassName' : sccn
> }
>
> 3) In the for loop:
> cn = get_typed_class(virt, cname)
> key_list['DeviceID'] = id
> key_list['CreationClassName'] = cn
>
> Or you can leave it as is. Either way is fine.
>
>
>> + inst = get_inst(server, virt, cname, id, key_list)
>
> You don't really need to call get_inst() here, unless you just want to 
> verify the instances exist.
>
> The list you build below (lelist) should be identical to cn_keys_list 
> (except for the key value of the list, lelist uses full CIM class 
> names and the other does not).
>
>> +
>> +def verify_eafp_values(server, virt, in_pllist, gi_inst_list):
>> + # Looping through the in_pllist to get association for devices.
>> + status = PASS
>> + an = get_typed_class(virt, "ElementAllocatedFromPool")
>> + sccn = get_typed_class(virt, "ComputerSystem")
>> + for cn, devid in sorted(in_pllist.iteritems()):
>> + try:
>> + assoc_info = Associators(server, an, cn, + DeviceID = devid, + 
>> CreationClassName = cn, + SystemName = test_dom,
>> + SystemCreationClassName = sccn, + virt=virt)
>> + if len(assoc_info) != 1:
>> + logger.error("%s returned %i ResourcePool objects for "
>> + "domain '%s'", an, len(assoc_info), + test_dom)
>> + status = FAIL
>> + break
>
> You can just return FAIL here.
>
>> + assoc_eafp_info = assoc_info[0] + CCName = assoc_eafp_info.classname
>> + gi_inst = gi_inst_list[CCName]
>> + if assoc_eafp_info['InstanceID'] != gi_inst['InstanceID']:
>> + field_err(assoc_eafp_info, gi_inst, 'InstanceID')
>> + return FAIL
>> + if assoc_eafp_info['PoolID'] != gi_inst['PoolID']:
>> + field_err(assoc_eafp_info, gi_inst, 'PoolID')
>> + return FAIL
>
> If you wanted, instead of building the gi_inst_list, you could pass in 
> the instances returned from the get_inst() call in get_pool_details(). 
> You could then call compare_all_prop().
>
> Either way is fine though.
I prefer using compare_all_prop() in the ResourcePool related tc and 
would want to check only the ID's here.
>
>> + except Exception, detail:
>> + logger.error(CIM_ERROR_ASSOCIATORS, an)
>> + logger.error("Exception: %s", detail)
>> + cleanup_restore(server, virt)
>> + status = FAIL
>> + return status
>> +
>>
>> @do_main(sup_types)
>> def main():
>> options = main.options
>> + server = options.ip
>> + virt = options.virt status = PASS
>
> I know this was already part of the test, but can you remove this 
> line? Or can you set status = FAIL instead? If all of the failure 
> paths aren't carefully checked, then having status = PASS can lead to 
> returning a false positive.
>
>>
>> - try: - cn = "Xen_LogicalDisk"
>> - key_list = get_keys(cn, test_disk)
>> - disk = devices.Xen_LogicalDisk(options.ip, key_list)
>> - except Exception,detail:
>> - print_error(cn, detail)
>> - return FAIL
>> + status, lelist = init_list(server, virt, vsxml, test_disk)
>> + if status != PASS:
>> + return status
>
> Need to call cleanup_restore() and destroy() here.
>
The call to cleanup_restore() and destroy() is done before the above 
function returns failure.
>> -
>> - ret = test_domain_function(test_dom, options.ip, \
>> - cmd = "destroy")
>> + status, gi_inst_list = get_pool_details(server, virt, vsxml, diskid)
>> + if status != PASS:
>> + return status
>
> Need to call cleanup_restore() and destroy() here.
>
The call to cleanup_restore() and destroy() is done before the above 
function returns failure.

Thanks and Regards,
Deepti.