[Libvirt-cim] [PATCH 2 of 4] [TEST] Adding functions to common_util.py to support the verifications of EAFP fields

[Kaitlin Rupert]

> 
> +def get_value(server, cmd, log_msg, fieldname):

I wouldn't make a separate function out of this.  it probably won't be 
used except by the eafp_dpool_cap_reserve_val() function.

> +    msg = log_msg % fieldname
> +    ret, value = run_remote(server, cmd)
> +    if ret != 0:
> +        logger.error("%s", log_msg, fieldname)
> +        return FAIL, value
> +    return PASS, value
> +
> +def eafp_dpool_cap_reserve_val(server, virt, poolname):

The capabilities and reserved values are properties of the ResourcePools 
- I'd drop eafp from the name since these values aren't related to the 
EAFP association.

Also, I'd move these functions to the logicaldevices.py file.  Or, you 
could create a resourcepool.py file and add the 
verify_common_pool_values() function to it.

The common_util.py is becoming quite large - so, it might be a good idea 
to try to place more specific functions like these (that deal with a 
specific provider's values) into a file specific for that provider.

> +    libvirt_version = virsh_version(server, virt)
> +    capacity = reserved = None
> +    if libvirt_version >= '0.4.1':
> +        # get the value from pool-info
> +        log_msg= "Failed to get the '%s' info from pool-info"
> +        dp_name, pname = poolname.split("/")
> +
> +        cmd = "virsh pool-info %s | awk '/Capacity/ { print \$2}'" \
> +              % pname
> +        status, cap_val = get_value(server, cmd, log_msg, 'Capacity')
> +        if status != PASS:
> +           return FAIL, capacity, reserved 
> +        cap_val  = float(cap_val)
> +        capacity = int(cap_val * 1024 * 1024 * 1024) >> 20

This calculation is odd.  You're converting GB into bytes and then from 
bytes into megabytes.  You can instead convert straight to megabytes:

capacity = int(cap_val * 1024)

> + 
> +        cmd = "virsh pool-info %s | awk '/Allocation/ { print \$2}'" \
> +              % pname
> +        status, alloc_val = get_value(server, cmd, log_msg, 'Allocation')
> +        if status != PASS:
> +           return FAIL, capacity, reserved 
> +        alloc_val = float(alloc_val)
> +        reserved  = int(alloc_val * 1024 * 1024 *1024) >> 20

Same here - convert straight to megabytes

> +
> +    else:
> +        # get info from stat -filesystem
> +        log_msg = "Stat on the '%s' file failed"
> +
> +        cmd = "stat -f %s | awk '/size/  {print   \$7}'" % disk_file  
> +        status, f_bsize = get_value(server, cmd, log_msg, disk_file)
> +        if status != PASS:
> +           return FAIL, capacity, reserved 
> +         
> +        cmd = " stat -f %s | awk '/Blocks/  {print   \$3}'" % disk_file
> +        status, b_total = get_value(server, cmd, log_msg, disk_file)
> +        if status != PASS:
> +           return FAIL, capacity, reserved 
> +        cap_val = (int(f_bsize) * int(b_total)) 
> +        capacity = (int(f_bsize) * int(b_total)) >> 20
> +
> +        cmd = "stat -f %s | awk '/Blocks/  {print \$5}'" % disk_file
> +        status, b_free = get_value(server, cmd, log_msg, disk_file)
> +        if status != PASS:
> +           return FAIL, capacity, reserved 
> +        reserved = (cap_val - (int(f_bsize) * int(b_free))) >> 20
> +
> +    return PASS, capacity, reserved

If you want to break up the size of this function some, you could do 
something like:

dpool_cap_reserve_val():

     libvirt_version = virsh_version(server, virt)
     capacity = reserved = None
     if libvirt_version >= '0.4.1':
         libvirt_dpool_cap_res()
     else:
         fs_dpool_cap_res()

Having sub functions for each of these breaks up the work nicely, and 
it's possible (although unlikely) that a test would need to call one of 
these functions explicitly.

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