Re: [libvirt] [PATCH 1/2] python: cleanup vcpu related binding APIs
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