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

> +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.

> + 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.

> - > - 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.