[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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)
else if (cond2)

can be joined like this:
if (cond1 || cond2)

      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;

          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. :)


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]