Re: [libvirt-python PATCH 16/23] change the order of some statements

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

 




On 09/24/2015 10:01 AM, Pavel Hrdina wrote:
> This change makes it easier to free allocated object especially for
> python objects.  We can benefit from the fact, that if you call
> Py_DECREF on eny python object it will also remove reference for all

s/eny/any

> assigned object to the root object.  For example, calling Py_DECREF on
> dict will also remove reference recursively on all elements in that
> dictionary.  Our job is then just call Py_DECREF on the root element and
> don't care about anything else.
> 

Something else that could go in HACKING - usage of Py_[X]DECREF...

> Signed-off-by: Pavel Hrdina <phrdina@xxxxxxxxxx>
> ---
>  libvirt-override.c | 189 ++++++++++++++++++++++++-----------------------------
>  1 file changed, 85 insertions(+), 104 deletions(-)
> 
> diff --git a/libvirt-override.c b/libvirt-override.c
> index 92c31b8..63a469b 100644
> --- a/libvirt-override.c
> +++ b/libvirt-override.c
> @@ -1238,9 +1238,16 @@ libvirt_virDomainGetVcpus(PyObject *self ATTRIBUTE_UNUSED,
>          goto cleanup;
>      if ((pycpuinfo = PyList_New(dominfo.nrVirtCpu)) == NULL)
>          goto cleanup;
> +
> +    if (PyTuple_SetItem(pyretval, 0, pycpuinfo) < 0)
> +        goto cleanup;
> +
>      if ((pycpumap = PyList_New(dominfo.nrVirtCpu)) == NULL)
>          goto cleanup;
>  
> +    if (PyTuple_SetItem(pyretval, 1, pycpumap) < 0)
> +        goto cleanup;
> +
>      for (i = 0; i < dominfo.nrVirtCpu; i++) {
>          PyObject *info = PyTuple_New(4);
>          PyObject *item = NULL;
> @@ -1248,54 +1255,42 @@ libvirt_virDomainGetVcpus(PyObject *self ATTRIBUTE_UNUSED,
>          if (info == NULL)
>              goto cleanup;
>  
> +        if (PyList_SetItem(pycpuinfo, i, info) < 0)
> +            goto cleanup;
> +
>          if ((item = libvirt_intWrap((long)cpuinfo[i].number)) == NULL ||
>              PyTuple_SetItem(info, 0, item) < 0)
> -            goto itemError;
> +            goto cleanup;
>  
>          if ((item = libvirt_intWrap((long)cpuinfo[i].state)) == NULL ||
>              PyTuple_SetItem(info, 1, item) < 0)
> -            goto itemError;
> +            goto cleanup;
>  
>          if ((item = libvirt_ulonglongWrap(cpuinfo[i].cpuTime)) == NULL ||
>              PyTuple_SetItem(info, 2, item) < 0)
> -            goto itemError;
> +            goto cleanup;
>  
>          if ((item = libvirt_intWrap((long)cpuinfo[i].cpu)) == NULL ||
>              PyTuple_SetItem(info, 3, item) < 0)
> -            goto itemError;
> -
> -        if (PyList_SetItem(pycpuinfo, i, info) < 0)
> -            goto itemError;
> -
> -        continue;
> -     itemError:
> -        Py_DECREF(info);
> -        Py_XDECREF(item);
> -        goto cleanup;
> +            goto cleanup;
>      }
>      for (i = 0; i < dominfo.nrVirtCpu; i++) {
>          PyObject *info = PyTuple_New(cpunum);
>          size_t j;
>          if (info == NULL)
>              goto cleanup;
> +
> +        if (PyList_SetItem(pycpumap, i, info) < 0)
> +            goto cleanup;
> +
>          for (j = 0; j < cpunum; j++) {
>              PyObject *item = NULL;
>              if ((item = PyBool_FromLong(VIR_CPU_USABLE(cpumap, cpumaplen,
>                                                         i, j))) == NULL ||
> -                PyTuple_SetItem(info, j, item) < 0) {
> -                Py_DECREF(info);
> -                Py_XDECREF(item);
> +                PyTuple_SetItem(info, j, item) < 0)
>                  goto cleanup;
> -            }
> -        }
> -        if (PyList_SetItem(pycpumap, i, info) < 0) {
> -            Py_DECREF(info);
> -            goto cleanup;
>          }
>      }
> -    if (PyTuple_SetItem(pyretval, 0, pycpuinfo) < 0 ||
> -        PyTuple_SetItem(pyretval, 1, pycpumap) < 0)
> -        goto cleanup;
>  
>      VIR_FREE(cpuinfo);
>      VIR_FREE(cpumap);
> @@ -1306,8 +1301,6 @@ libvirt_virDomainGetVcpus(PyObject *self ATTRIBUTE_UNUSED,
>      VIR_FREE(cpuinfo);
>      VIR_FREE(cpumap);
>      Py_XDECREF(pyretval);
> -    Py_XDECREF(pycpuinfo);
> -    Py_XDECREF(pycpumap);
>      return error;
>  }

Splatter from bleeding eyeballs - perhaps would have been easier to read
with incremental change ;-)

>  
> @@ -1489,12 +1482,13 @@ libvirt_virDomainGetVcpuPinInfo(PyObject *self ATTRIBUTE_UNUSED,
>          if (mapinfo == NULL)
>              goto cleanup;
>  
> +        PyList_SetItem(pycpumaps, vcpu, mapinfo);
> +
>          for (pcpu = 0; pcpu < cpunum; pcpu++) {
>              PyTuple_SetItem(mapinfo, pcpu,
>                              PyBool_FromLong(VIR_CPU_USABLE(cpumaps, cpumaplen,
>                                                             vcpu, pcpu)));
>          }
> -        PyList_SetItem(pycpumaps, vcpu, mapinfo);
>      }
>  
>      VIR_FREE(cpumaps);
> @@ -1877,12 +1871,14 @@ libvirt_virErrorFuncHandler(ATTRIBUTE_UNUSED void *ctx,
>          if ((list = PyTuple_New(2)) == NULL)
>              goto cleanup;
>  
> +        Py_XINCREF(libvirt_virPythonErrorFuncCtxt);
> +        PyTuple_SetItem(list, 0, libvirt_virPythonErrorFuncCtxt);
> +
>          if ((info = PyTuple_New(9)) == NULL)
>              goto cleanup;
>  
> -        PyTuple_SetItem(list, 0, libvirt_virPythonErrorFuncCtxt);
>          PyTuple_SetItem(list, 1, info);
> -        Py_XINCREF(libvirt_virPythonErrorFuncCtxt);
> +
>          PyTuple_SetItem(info, 0, libvirt_intWrap((long) err->code));
>          PyTuple_SetItem(info, 1, libvirt_intWrap((long) err->domain));
>          PyTuple_SetItem(info, 2, libvirt_constcharPtrWrap(err->message));
> @@ -1894,14 +1890,11 @@ libvirt_virErrorFuncHandler(ATTRIBUTE_UNUSED void *ctx,
>          PyTuple_SetItem(info, 8, libvirt_intWrap((long) err->int2));
>          /* TODO pass conn and dom if available */
>          result = PyEval_CallObject(libvirt_virPythonErrorFuncHandler, list);
> -        Py_XDECREF(list);
>          Py_XDECREF(result);
>      }
>  
>   cleanup:
>      Py_XDECREF(list);
> -    Py_XDECREF(info);
> -

This one I'm confused by why 'info' is removed...

The rest seemed OK - a couple were tough to follow, but not impossible.

John

>      LIBVIRT_RELEASE_THREAD_STATE;
>  }
>  
> @@ -1966,6 +1959,8 @@ virConnectCredCallbackWrapper(virConnectCredentialPtr cred,
>      if ((pycred = PyTuple_New(ncred)) == NULL)
>          goto cleanup;
>  
> +    PyTuple_SetItem(list, 0, pycred);
> +
>      for (i = 0; i < ncred; i++) {
>          PyObject *pycreditem = PyList_New(5);
>          if (pycreditem == NULL)
> @@ -1989,7 +1984,6 @@ virConnectCredCallbackWrapper(virConnectCredentialPtr cred,
>          PyList_SetItem(pycreditem, 4, VIR_PY_NONE);
>      }
>  
> -    PyTuple_SetItem(list, 0, pycred);
>      Py_XINCREF(pycbdata);
>      PyTuple_SetItem(list, 1, pycbdata);
>  
> @@ -4720,19 +4714,18 @@ libvirt_virDomainGetBlockJobInfo(PyObject *self ATTRIBUTE_UNUSED,
>          return NULL;
>      domain = (virDomainPtr) PyvirDomain_Get(pyobj_domain);
>  
> -    if ((dict = PyDict_New()) == NULL)
> -        return NULL;
> -
>      LIBVIRT_BEGIN_ALLOW_THREADS;
>      c_ret = virDomainGetBlockJobInfo(domain, path, &info, flags);
>      LIBVIRT_END_ALLOW_THREADS;
>  
> -    if (c_ret == 0) {
> -        return dict;
> -    } else if (c_ret < 0) {
> -        Py_DECREF(dict);
> +    if (c_ret < 0)
>          return VIR_PY_NONE;
> -    }
> +
> +    if ((dict = PyDict_New()) == NULL)
> +        return NULL;
> +
> +    if (c_ret == 0)
> +        return dict;
>  
>      if ((type = libvirt_intWrap(info.type)) == NULL ||
>          PyDict_SetItemString(dict, "type", type) < 0)
> @@ -5270,16 +5263,22 @@ libvirt_virEventAddHandleFunc(int fd,
>                                virFreeCallback ff)
>  {
>      PyObject *result;
> -    PyObject *python_cb;
>      PyObject *cb_obj;
>      PyObject *ff_obj;
>      PyObject *opaque_obj;
> +    PyObject *python_cb = NULL;
>      PyObject *cb_args = NULL;
>      PyObject *pyobj_args = NULL;
>      int retval = -1;
>  
>      LIBVIRT_ENSURE_THREAD_STATE;
>  
> +    if ((pyobj_args = PyTuple_New(4)) == NULL)
> +        goto cleanup;
> +
> +    PyTuple_SetItem(pyobj_args, 0, libvirt_intWrap(fd));
> +    PyTuple_SetItem(pyobj_args, 1, libvirt_intWrap(event));
> +
>      /* Lookup the python callback */
>      python_cb = libvirt_lookupPythonFunc("_eventInvokeHandleCallback");
>      if (!python_cb) {
> @@ -5287,26 +5286,21 @@ libvirt_virEventAddHandleFunc(int fd,
>      }
>      Py_INCREF(python_cb);
>  
> -    /* create tuple for cb */
> -    cb_obj = libvirt_virEventHandleCallbackWrap(cb);
> -    ff_obj = libvirt_virFreeCallbackWrap(ff);
> -    opaque_obj = libvirt_virVoidPtrWrap(opaque);
> +    PyTuple_SetItem(pyobj_args, 2, python_cb);
>  
>      if ((cb_args = PyTuple_New(3)) == NULL)
>          goto cleanup;
>  
> +    PyTuple_SetItem(pyobj_args, 3, cb_args);
> +
> +    /* create tuple for cb */
> +    cb_obj = libvirt_virEventHandleCallbackWrap(cb);
> +    ff_obj = libvirt_virFreeCallbackWrap(ff);
> +    opaque_obj = libvirt_virVoidPtrWrap(opaque);
>      PyTuple_SetItem(cb_args, 0, cb_obj);
>      PyTuple_SetItem(cb_args, 1, opaque_obj);
>      PyTuple_SetItem(cb_args, 2, ff_obj);
>  
> -    if ((pyobj_args = PyTuple_New(4)) == NULL)
> -        goto cleanup;
> -
> -    PyTuple_SetItem(pyobj_args, 0, libvirt_intWrap(fd));
> -    PyTuple_SetItem(pyobj_args, 1, libvirt_intWrap(event));
> -    PyTuple_SetItem(pyobj_args, 2, python_cb);
> -    PyTuple_SetItem(pyobj_args, 3, cb_args);
> -
>      result = PyEval_CallObject(addHandleObj, pyobj_args);
>      if (!result) {
>          PyErr_Print();
> @@ -5415,33 +5409,32 @@ libvirt_virEventAddTimeoutFunc(int timeout,
>  
>      LIBVIRT_ENSURE_THREAD_STATE;
>  
> +    if ((pyobj_args = PyTuple_New(3)) == NULL)
> +        goto cleanup;
> +
> +    PyTuple_SetItem(pyobj_args, 0, libvirt_intWrap(timeout));
> +
>      /* Lookup the python callback */
>      python_cb = libvirt_lookupPythonFunc("_eventInvokeTimeoutCallback");
>      if (!python_cb) {
>          goto cleanup;
>      }
>      Py_INCREF(python_cb);
> +    PyTuple_SetItem(pyobj_args, 1, python_cb);
> +
> +    if ((cb_args = PyTuple_New(3)) == NULL)
> +        goto cleanup;
> +
> +    PyTuple_SetItem(pyobj_args, 2, cb_args);
>  
>      /* create tuple for cb */
>      cb_obj = libvirt_virEventTimeoutCallbackWrap(cb);
>      ff_obj = libvirt_virFreeCallbackWrap(ff);
>      opaque_obj = libvirt_virVoidPtrWrap(opaque);
> -
> -    if ((cb_args = PyTuple_New(3)) == NULL)
> -        goto cleanup;
> -
>      PyTuple_SetItem(cb_args, 0, cb_obj);
>      PyTuple_SetItem(cb_args, 1, opaque_obj);
>      PyTuple_SetItem(cb_args, 2, ff_obj);
>  
> -    pyobj_args = PyTuple_New(3);
> -    if ((pyobj_args = PyTuple_New(3)) == NULL)
> -        goto cleanup;
> -
> -    PyTuple_SetItem(pyobj_args, 0, libvirt_intWrap(timeout));
> -    PyTuple_SetItem(pyobj_args, 1, python_cb);
> -    PyTuple_SetItem(pyobj_args, 2, cb_args);
> -
>      result = PyEval_CallObject(addTimeoutObj, pyobj_args);
>      if (!result) {
>          PyErr_Print();
> @@ -6171,12 +6164,13 @@ libvirt_virConnectDomainEventGraphicsCallback(virConnectPtr conn ATTRIBUTE_UNUSE
>          if (pair == NULL)
>              goto cleanup;
>  
> +        PyList_SetItem(pyobj_subject, i, pair);
> +
>          PyTuple_SetItem(pair, 0,
>                          libvirt_constcharPtrWrap(subject->identities[i].type));
>          PyTuple_SetItem(pair, 1,
>                          libvirt_constcharPtrWrap(subject->identities[i].name));
>  
> -        PyList_SetItem(pyobj_subject, i, pair);
>      }
>  
>      /* Call the Callback Dispatcher */
> @@ -7751,6 +7745,9 @@ libvirt_virNodeGetCPUMap(PyObject *self ATTRIBUTE_UNUSED,
>      if ((pycpumap = PyList_New(i_retval)) == NULL)
>          goto error;
>  
> +    if (PyTuple_SetItem(ret, 1, pycpumap) < 0)
> +        goto error;
> +
>      for (i = 0; i < i_retval; i++) {
>          if ((pyused = PyBool_FromLong(VIR_CPU_USED(cpumap, i))) == NULL)
>              goto error;
> @@ -7758,9 +7755,6 @@ libvirt_virNodeGetCPUMap(PyObject *self ATTRIBUTE_UNUSED,
>              goto error;
>      }
>  
> -    if (PyTuple_SetItem(ret, 1, pycpumap) < 0)
> -        goto error;
> -
>      /* 2: number of online CPUs */
>      if ((pyonline = libvirt_uintWrap(online)) == NULL ||
>          PyTuple_SetItem(ret, 2, pyonline) < 0)
> @@ -7769,12 +7763,9 @@ libvirt_virNodeGetCPUMap(PyObject *self ATTRIBUTE_UNUSED,
>   cleanup:
>      VIR_FREE(cpumap);
>      return ret;
> +
>   error:
>      Py_CLEAR(ret);
> -    Py_XDECREF(pycpumap);
> -    Py_XDECREF(pyused);
> -    Py_XDECREF(pycpunum);
> -    Py_XDECREF(pyonline);
>      goto cleanup;
>  }
>  #endif /* LIBVIR_CHECK_VERSION(1, 0, 0) */
> @@ -8149,6 +8140,12 @@ libvirt_virNodeGetFreePages(PyObject *self ATTRIBUTE_UNUSED,
>              goto cleanup;
>          }
>  
> +        if (PyDict_SetItem(pyobj_counts, node, per_node) < 0) {
> +            Py_XDECREF(per_node);
> +            Py_XDECREF(node);
> +            goto cleanup;
> +        }
> +
>          for (j = 0; j < pyobj_pagesize_size; j ++) {
>              PyObject *page = NULL;
>              PyObject *count = NULL;
> @@ -8164,12 +8161,6 @@ libvirt_virNodeGetFreePages(PyObject *self ATTRIBUTE_UNUSED,
>              }
>          }
>          i += pyobj_pagesize_size;
> -
> -        if (PyDict_SetItem(pyobj_counts, node, per_node) < 0) {
> -            Py_XDECREF(per_node);
> -            Py_XDECREF(node);
> -            goto cleanup;
> -        }
>      }
>  
>      py_retval = pyobj_counts;
> @@ -8220,6 +8211,9 @@ libvirt_virNetworkGetDHCPLeases(PyObject *self ATTRIBUTE_UNUSED,
>          if ((py_lease = PyDict_New()) == NULL)
>              goto error;
>  
> +        if (PyList_SetItem(py_retval, i, py_lease) < 0)
> +            goto error;
> +
>  #define VIR_SET_LEASE_ITEM(NAME, VALUE_OBJ_FUNC)                            \
>          do {                                                                \
>              PyObject *tmp_val;                                              \
> @@ -8244,15 +8238,9 @@ libvirt_virNetworkGetDHCPLeases(PyObject *self ATTRIBUTE_UNUSED,
>          VIR_SET_LEASE_ITEM("iaid", libvirt_charPtrWrap(lease->iaid));
>  
>  #undef VIR_SET_LEASE_ITEM
> -
> -        if (PyList_SetItem(py_retval, i, py_lease) < 0)
> -            goto error;
> -
> -        py_lease = NULL;
>      }
>  
>   cleanup:
> -    Py_XDECREF(py_lease);
>      if (leases) {
>          for (i = 0; i < leases_count; i++)
>              virNetworkDHCPLeaseFree(leases[i]);
> @@ -8287,6 +8275,9 @@ convertDomainStatsRecord(virDomainStatsRecordPtr *records,
>          if (!(py_record = PyTuple_New(2)))
>              goto error;
>  
> +        if (PyList_SetItem(py_retval, i, py_record) < 0)
> +            goto error;
> +
>          /* libvirt_virDomainPtrWrap steals the object */
>          virDomainRef(records[i]->dom);
>          if (!(py_record_domain = libvirt_virDomainPtrWrap(records[i]->dom))) {
> @@ -8294,33 +8285,21 @@ convertDomainStatsRecord(virDomainStatsRecordPtr *records,
>              goto error;
>          }
>  
> -        if (!(py_record_stats = getPyVirTypedParameter(records[i]->params,
> -                                                       records[i]->nparams)))
> -            goto error;
> -
>          if (PyTuple_SetItem(py_record, 0, py_record_domain) < 0)
>              goto error;
>  
> -        py_record_domain = NULL;
> -
> -        if (PyTuple_SetItem(py_record, 1, py_record_stats) < 0)
> +        if (!(py_record_stats = getPyVirTypedParameter(records[i]->params,
> +                                                       records[i]->nparams)))
>              goto error;
>  
> -        py_record_stats = NULL;
> -
> -        if (PyList_SetItem(py_retval, i, py_record) < 0)
> +        if (PyTuple_SetItem(py_record, 1, py_record_stats) < 0)
>              goto error;
> -
> -        py_record = NULL;
>      }
>  
>      return py_retval;
>  
>   error:
>      Py_XDECREF(py_retval);
> -    Py_XDECREF(py_record);
> -    Py_XDECREF(py_record_domain);
> -    Py_XDECREF(py_record_stats);
>      return NULL;
>  }
>  
> @@ -8546,13 +8525,15 @@ libvirt_virDomainGetFSInfo(PyObject *self ATTRIBUTE_UNUSED,
>          if (info == NULL)
>              goto cleanup;
>          PyList_SetItem(py_retval, i, info);
> -        alias = PyList_New(0);
> -        if (alias == NULL)
> -            goto cleanup;
>  
>          PyTuple_SetItem(info, 0, libvirt_constcharPtrWrap(fs->mountpoint));
>          PyTuple_SetItem(info, 1, libvirt_constcharPtrWrap(fs->name));
>          PyTuple_SetItem(info, 2, libvirt_constcharPtrWrap(fs->fstype));
> +
> +        alias = PyList_New(0);
> +        if (alias == NULL)
> +            goto cleanup;
> +
>          PyTuple_SetItem(info, 3, alias);
>  
>          for (j = 0; j < fs->ndevAlias; j++)
> 

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list



[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]