[Libvirt-cim] [PATCH 3 of 3] [TEST] Add new tc to verify the err values for RPCS DeleteResourceInPool()

[Deepti B Kalakeri]

Kaitlin Rupert wrote:
> Deepti B Kalakeri wrote:
>>
>>
>> Kaitlin Rupert wrote:
>>>
>>>> +def verify_rpcs_err_val(virt, server, rpcs_conn, dp_cn, pool_name, 
>>>> + exp_vol_path, dp_inst):
>>>> + for err_scen in invalid_scen.keys(): + logger.info("Verifying 
>>>> errors for '%s'....", err_scen)
>>>> + status = FAIL
>>>> + del_res = [FAIL] + try:
>>>
>>> I would put the try / execpt outside of the for loop. This will save 
>>> you some indentation.
>> I would need the try: except block .. so that I can catch the errors 
>> for each of the invalid delete() scenarios.
>
> Agreed. Your code does something like:
>
> + for err_scen in invalid_scen.keys():
>
> <snip>
>
> + try:
>
> <snip>
>
> +
> + except CIMError, (err_no, err_desc):
>
> Why not do:
>
> try:
>
> for
>
> except CIMError, (err_no, err_desc):
>
> except Exception, details:
If I change the existing code to
try:

for
except CIMError, (err_no, err_desc):

except Exception, details:

Then, I will be able to execute the for loop only one time and the 
execution will come out with suitable message from verify_error*().

> This would save you some indentation, and allow you to catch any 
> unexpected errors in addition to the errors thrown by the delete call.
>
Good Point!! to include the exception for the other cases apart from the 
DeleteResourceInPool().
>
>>>> + resource = inst_to_mof(res_settings) + del_res = 
>>>> rpcs_conn.DeleteResourceInPool(Resource=resource,
>>>> + Pool=dp_inst)
>>>> + else:
>>>> + exp_err_no = CIM_ERR_INVALID_PARAMETER
>>>> + if err_scen == "MISSING_RESOURCE":
>>>> + del_res = rpcs_conn.DeleteResourceInPool(Pool=dp_inst)
>>>> + elif err_scen == "MISSING_POOL":
>>>> + del_res = rpcs_conn.DeleteResourceInPool(Resource=resource)
>>>
>>> Will invalid_scen.keys() already return the keys in the same order? 
>>> I'm wondering if it is possible for resource to be undefined here 
>>> since it only gets defined if "if not "MISSING" in err_scen:" has 
>>> passed in a prior iteration of the loop.
>>>
>>> If "if not "MISSING" in err_scen:" fails the first time through the 
>>> loop, resource will be undefined.
>>>
>> I am not sure I understand the comment here.
>
> If you look at the Python documentation, the keys are returned an 
> arbitrary order (http://docs.python.org/library/stdtypes.html#dict.items).
Yeah thats correct the keys() would not come in a particular oder unless 
sorted.
>
> So taking a look at your code, let's say keys() returns err_scen == 
> MISSING_POOL the first time through the loop...
>
> if not "MISSING" in err_scen:
> This check fails
>
> else:
> exp_err_no = CIM_ERR_INVALID_PARAMETER
> if err_scen == "MISSING_RESOURCE":
> del_res = rpcs_conn.DeleteResourceInPool(Pool=dp_inst)
> elif err_scen == "MISSING_POOL":
> del_res = rpcs_conn.DeleteResourceInPool(Resource=resource)
>
> This code is executed, but resource hasn't been set yet.
>
>
yeah!! the old patch did not set the resource for the MISSING_POOL case 
and that was a mistake. This has been included in the new patch that was 
sent yesterday.

-- 
Thanks and Regards,
Deepti B. Kalakeri
IBM Linux Technology Center
deeptik at linux.vnet.ibm.com