On 09/27/2012 09:51 PM, Michal Privoznik 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
On 26.09.2012 19:33, Guannan Ren wrote: 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