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

[Kaitlin Rupert]

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

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