[libvirt] [PATCH 1/2] python: cleanup vcpu related binding APIs

[Guannan Ren]

On 09/27/2012 09:51 PM, Michal Privoznik wrote:
> On 26.09.2012 19:33, Guannan Ren wrote:
>> libvirt_virDomainGetVcpus: add error handling, return -1 instead of None
>> libvirt_virDomainPinVcpu and libvirt_virDomainPinVcpuFlags:
>>    make use of libvirt_boolUnwrap
>>
>>    Set bitmap according to these values which are contained in given
>>    argument of vcpu tuple and turn off these bit corresponding to
>>    missing vcpus in argument tuple
>>
>>    The original way ignored the error info from PyTuple_GetItem
>>    if index is out of range.
>>    "IndexError: tuple index out of range"
>>    The error message will only be raised on next command in interactive mode.
>> ---
>>   python/libvirt-override.c | 171 +++++++++++++++++++++++++++++++++-------------
>>   1 file changed, 123 insertions(+), 48 deletions(-)
>>
>> diff --git a/python/libvirt-override.c b/python/libvirt-override.c
>> index 25f9d3f..b69f5cf 100644
>> --- a/python/libvirt-override.c
>> +++ b/python/libvirt-override.c
>> @@ -1333,9 +1333,11 @@ cleanup:
>>   
>>   static PyObject *
>>   libvirt_virDomainGetVcpus(PyObject *self ATTRIBUTE_UNUSED,
>> -                          PyObject *args) {
>> +                          PyObject *args)
>> +{
>>       virDomainPtr domain;
>>       PyObject *pyobj_domain, *pyretval = NULL, *pycpuinfo = NULL, *pycpumap = NULL;
>> +    PyObject *error = NULL;
> You are not setting this variable before every 'goto cleanup;' and I
> think it should be done. Or is it okay to return NULL?

       we set error variable in two places.
      error = PyErr_NoMemory();
      error = VIR_PY_INT_FAIL;

      For the other places before goto cleanup, we don't need to set 
specific error because
      the cpython function did it already. what left to me is to return 
NULL to show these
      error messages to user.

      In common,  if cpython function(e.g PyTuple_New()) reports errors, 
we just return NULL.


>
> No. I know python bindings code are mess but this won't make it any
> better. First of all:
> if (cond1)
>    code_block;
> else if (cond2)
>    the_very_same_codeblock;
>
> can be joined like this:
> if (cond1 || cond2)
>    code_block;

       Thanks I will change to this.


>
> Second - drop the do { } while(0) and use a label instead:
> if (cond1 || cond2)
>    goto label;

        without do () while, we  have to have two labels
        I think it will be a little messy.
        I tried the following before, it is ok to walk back.

        if (cond1 || cond2)
           goto label;

        continue;
        label:
           Py_DECREF(info);
           ..
           goto cleanup

>
> This label can in this special case be at the end of the parent for
> loop; However, there needs to be 'continue' just before the label so the
> for loop doesn't execute the label itself.
>
>
>

      For the following, I will change according to your comments.
      Thanks  for this review. :)

      Guannan