Re: [libvirt-python PATCH 22/23] use VIR_PY_DICT_SET_GOTO

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

 



On Sat, Sep 26, 2015 at 10:46:13AM -0400, John Ferlan wrote:
> 
> 
> On 09/24/2015 10:01 AM, Pavel Hrdina wrote:
> > Signed-off-by: Pavel Hrdina <phrdina@xxxxxxxxxx>
> > ---
> >  libvirt-override.c | 275 +++++++++++++++++++----------------------------------
> >  libvirt-utils.c    |  13 +--
> >  2 files changed, 99 insertions(+), 189 deletions(-)
> > 
> > diff --git a/libvirt-override.c b/libvirt-override.c
> > index 1f795d9..3192e74 100644
> > --- a/libvirt-override.c
> > +++ b/libvirt-override.c
> > @@ -403,18 +403,14 @@ libvirt_virDomainMemoryStats(PyObject *self ATTRIBUTE_UNUSED,
> >          }
> >          val = libvirt_ulonglongWrap(stats[i].val);
> >  
> > -        if (!key || !val || PyDict_SetItem(info, key, val) < 0) {
> > -            Py_CLEAR(info);
> > -            goto cleanup;
> > -        }
> > -        Py_CLEAR(key);
> > -        Py_CLEAR(val);
> > +        VIR_PY_DICT_SET_GOTO(info, key, val, error);
> >      }
> >  
> > - cleanup:
> > -    Py_XDECREF(key);
> > -    Py_XDECREF(val);
> >      return info;
> > +
> > + error:
> > +    Py_DECREF(info);
> > +    return NULL;
> >  }
> >  
> >  static PyObject *
> > @@ -3358,23 +3354,16 @@ libvirt_virNodeGetCPUStats(PyObject *self ATTRIBUTE_UNUSED,
> >          key = libvirt_constcharPtrWrap(stats[i].field);
> >          val = libvirt_ulonglongWrap(stats[i].value);
> >  
> > -        if (!key || !val || PyDict_SetItem(ret, key, val) < 0) {
> > -            Py_CLEAR(ret);
> > -            goto error;
> > -        }
> > -
> > -        Py_DECREF(key);
> > -        Py_DECREF(val);
> > +        VIR_PY_DICT_SET_GOTO(ret, key, val, error);
> >      }
> >  
> > + cleanup:
> >      VIR_FREE(stats);
> >      return ret;
> >  
> >   error:
> > -    VIR_FREE(stats);
> > -    Py_XDECREF(key);
> > -    Py_XDECREF(val);
> > -    return ret;
> > +    Py_CLEAR(ret);
> > +    goto cleanup;
> >  }
> >  
> >  static PyObject *
> > @@ -3423,23 +3412,16 @@ libvirt_virNodeGetMemoryStats(PyObject *self ATTRIBUTE_UNUSED,
> >          key = libvirt_constcharPtrWrap(stats[i].field);
> >          val = libvirt_ulonglongWrap(stats[i].value);
> >  
> > -        if (!key || !val || PyDict_SetItem(ret, key, val) < 0) {
> > -            Py_CLEAR(ret);
> > -            goto error;
> > -        }
> > -
> > -        Py_DECREF(key);
> > -        Py_DECREF(val);
> > +        VIR_PY_DICT_SET_GOTO(ret, key, val, error);
> >      }
> >  
> > + cleanup:
> >      VIR_FREE(stats);
> >      return ret;
> >  
> >   error:
> > -    VIR_FREE(stats);
> > -    Py_XDECREF(key);
> > -    Py_XDECREF(val);
> > -    return ret;
> > +    Py_CLEAR(ret);
> > +    goto cleanup;
> >  }
> >  
> >  static PyObject *
> > @@ -4747,11 +4729,8 @@ libvirt_virDomainGetJobStats(PyObject *self ATTRIBUTE_UNUSED,
> >      if (!(dict = getPyVirTypedParameter(params, nparams)))
> >          goto cleanup;
> >  
> > -    if (PyDict_SetItem(dict, libvirt_constcharPtrWrap("type"),
> > -                       libvirt_intWrap(type)) < 0) {
> > -        Py_CLEAR(dict);
> > -        goto cleanup;
> > -    }
> > +    VIR_PY_DICT_SET_GOTO(dict, libvirt_constcharPtrWrap("type"),
> > +                         libvirt_intWrap(type), cleanup);
> 
> it's not clear in this case on error that there's a "Py_CLEAR(dict);" -
> think you may need a goto error which has the Py_CLEAR(dict) and goto
> cleanup

Right, I'll fix that.

> 
> >  
> >   cleanup:
> >      virTypedParamsFree(params, nparams);
> > @@ -4790,34 +4769,19 @@ libvirt_virDomainGetBlockJobInfo(PyObject *self ATTRIBUTE_UNUSED,
> >      if (c_ret == 0)
> >          return dict;
> >  
> > -    if ((type = libvirt_intWrap(info.type)) == NULL ||
> > -        PyDict_SetItemString(dict, "type", type) < 0)
> > -        goto error;
> > -    Py_DECREF(type);
> > -
> > -    if ((bandwidth = libvirt_ulongWrap(info.bandwidth)) == NULL ||
> > -        PyDict_SetItemString(dict, "bandwidth", bandwidth) < 0)
> > -        goto error;
> > -    Py_DECREF(bandwidth);
> > -
> > -    if ((cur = libvirt_ulonglongWrap(info.cur)) == NULL ||
> > -        PyDict_SetItemString(dict, "cur", cur) < 0)
> > -        goto error;
> > -    Py_DECREF(cur);
> > -
> > -    if ((end = libvirt_ulonglongWrap(info.end)) == NULL ||
> > -        PyDict_SetItemString(dict, "end", end) < 0)
> > -        goto error;
> > -    Py_DECREF(end);
> > +    VIR_PY_DICT_SET_GOTO(dict, libvirt_constcharPtrWrap("type"),
> > +                         libvirt_intWrap(info.type), error);
> > +    VIR_PY_DICT_SET_GOTO(dict, libvirt_constcharPtrWrap("bandwidth"),
> > +                         libvirt_ulongWrap(info.bandwidth), error);
> > +    VIR_PY_DICT_SET_GOTO(dict, libvirt_constcharPtrWrap("cur"),
> > +                         libvirt_ulonglongWrap(info.cur), error);
> > +    VIR_PY_DICT_SET_GOTO(dict, libvirt_constcharPtrWrap("end"),
> > +                         libvirt_ulonglongWrap(info.end), error);
> 
> build issue - 'end', 'cur', 'bandwidth', and 'type' no longer necessary
> >  
> >      return dict;
> >  
> >   error:
> >      Py_DECREF(dict);
> 
> Should this by Py_CLEAR or XDECREF? (I forget already)

If you are sure, that dict != NULL, you can use Py_DECREF, if dict can by null,
you must use Py_XDECREF.  Py_CLEAR is used only in case, that you need to
decrement the dict and also set it to NULL.  The correct one is Py_DECREF,
becasue we know, that dict != null and we don't need to set it to null.

Pavel

> 
> > -    Py_XDECREF(type);
> > -    Py_XDECREF(bandwidth);
> > -    Py_XDECREF(cur);
> > -    Py_XDECREF(end);
> >      return NULL;
> >  }
> >  
> > @@ -4983,9 +4947,10 @@ libvirt_virDomainGetDiskErrors(PyObject *self ATTRIBUTE_UNUSED,
> >          goto cleanup;
> >  
> >      for (i = 0; i < count; i++) {
> > -        PyDict_SetItem(py_retval,
> > -                       libvirt_constcharPtrWrap(disks[i].disk),
> > -                       libvirt_intWrap(disks[i].error));
> > +        VIR_PY_DICT_SET_GOTO(py_retval,
> > +                             libvirt_constcharPtrWrap(disks[i].disk),
> > +                             libvirt_intWrap(disks[i].error),
> > +                             error);
> >      }
> >  
> >   cleanup:
> > @@ -4995,6 +4960,10 @@ libvirt_virDomainGetDiskErrors(PyObject *self ATTRIBUTE_UNUSED,
> >          VIR_FREE(disks);
> >      }
> >      return py_retval;
> > +
> > + error:
> > +    Py_CLEAR(py_retval);
> > +    goto cleanup;
> >  }
> >  
> >  
> > @@ -5038,12 +5007,8 @@ libvirt_virDomainInterfaceAddresses(PyObject *self ATTRIBUTE_UNUSED,
> >          if (!(py_iface = PyDict_New()))
> >              goto error;
> >  
> > -        if ((py_iname = libvirt_charPtrWrap(iface->name)) == NULL ||
> > -            PyDict_SetItem(py_retval, py_iname, py_iface) < 0) {
> > -            Py_XDECREF(py_iname);
> > -            Py_DECREF(py_iface);
> > -            goto error;
> > -        }
> > +        VIR_PY_DICT_SET_GOTO(py_retval, libvirt_charPtrWrap(iface->name),
> > +                             py_iface, error);
> 
> build issue - 'py_iname' no longer necessary
> 
> >  
> >          if (iface->naddrs) {
> >              if (!(py_addrs = PyList_New(iface->naddrs))) {
> > @@ -5053,20 +5018,11 @@ libvirt_virDomainInterfaceAddresses(PyObject *self ATTRIBUTE_UNUSED,
> >              py_addrs = VIR_PY_NONE;
> >          }
> >  
> > -        if ((py_iname = libvirt_constcharPtrWrap("addrs")) == NULL ||
> > -            PyDict_SetItem(py_iface, py_iname, py_addrs) < 0) {
> > -            Py_XDECREF(py_iname);
> > -            Py_DECREF(py_addrs);
> > -            goto error;
> > -        }
> > +        VIR_PY_DICT_SET_GOTO(py_iface, libvirt_constcharPtrWrap("addrs"),
> > +                             py_addrs, error);
> >  
> > -        if ((py_iname = libvirt_constcharPtrWrap("hwaddr")) == NULL ||
> > -            (py_ivalue = libvirt_constcharPtrWrap(iface->hwaddr)) == NULL ||
> > -            PyDict_SetItem(py_iface, py_iname, py_ivalue) < 0) {
> > -            Py_XDECREF(py_iname);
> > -            Py_XDECREF(py_ivalue);
> > -            goto error;
> > -        }
> > +        VIR_PY_DICT_SET_GOTO(py_iface, libvirt_constcharPtrWrap("hwaddr"),
> > +                             libvirt_constcharPtrWrap(iface->hwaddr), error);
> 
> build issue - 'py_ivalue' no longer necessary
> 
> >  
> >          for (j = 0; j < iface->naddrs; j++) {
> >              virDomainIPAddressPtr addr = &(iface->addrs[j]);
> > @@ -5077,27 +5033,12 @@ libvirt_virDomainInterfaceAddresses(PyObject *self ATTRIBUTE_UNUSED,
> >  
> >              VIR_PY_LIST_SET_GOTO(py_addrs, j, py_addr, error);
> >  
> > -            if ((py_iname = libvirt_constcharPtrWrap("addr")) == NULL ||
> > -                (py_ivalue = libvirt_constcharPtrWrap(addr->addr)) == NULL ||
> > -                PyDict_SetItem(py_addr, py_iname, py_ivalue) < 0) {
> > -                Py_XDECREF(py_iname);
> > -                Py_XDECREF(py_ivalue);
> > -                goto error;
> > -            }
> > -            if ((py_iname = libvirt_constcharPtrWrap("prefix")) == NULL ||
> > -                (py_ivalue = libvirt_intWrap(addr->prefix)) == NULL ||
> > -                PyDict_SetItem(py_addr, py_iname, py_ivalue) < 0) {
> > -                Py_XDECREF(py_iname);
> > -                Py_XDECREF(py_ivalue);
> > -                goto error;
> > -            }
> > -            if ((py_iname = libvirt_constcharPtrWrap("type")) == NULL ||
> > -                (py_ivalue = libvirt_intWrap(addr->type)) == NULL ||
> > -                PyDict_SetItem(py_addr, py_iname, py_ivalue) < 0) {
> > -                Py_XDECREF(py_iname);
> > -                Py_XDECREF(py_ivalue);
> > -                goto error;
> > -            }
> > +            VIR_PY_DICT_SET_GOTO(py_addr, libvirt_constcharPtrWrap("addr"),
> > +                                 libvirt_constcharPtrWrap(addr->addr), error);
> > +            VIR_PY_DICT_SET_GOTO(py_addr, libvirt_constcharPtrWrap("prefix"),
> > +                                 libvirt_uintWrap(addr->prefix), error);
> > +            VIR_PY_DICT_SET_GOTO(py_addr, libvirt_constcharPtrWrap("type"),
> > +                                 libvirt_intWrap(addr->type), error);
> >          }
> >      }
> >  
> > @@ -6171,28 +6112,34 @@ libvirt_virConnectDomainEventGraphicsCallback(virConnectPtr conn ATTRIBUTE_UNUSE
> >      if ((pyobj_local = PyDict_New()) == NULL)
> >          goto cleanup;
> >  
> > -    PyDict_SetItem(pyobj_local,
> > -                   libvirt_constcharPtrWrap("family"),
> > -                   libvirt_intWrap(local->family));
> > -    PyDict_SetItem(pyobj_local,
> > -                   libvirt_constcharPtrWrap("node"),
> > -                   libvirt_constcharPtrWrap(local->node));
> > -    PyDict_SetItem(pyobj_local,
> > -                   libvirt_constcharPtrWrap("service"),
> > -                   libvirt_constcharPtrWrap(local->service));
> > +    VIR_PY_DICT_SET_GOTO(pyobj_local,
> > +                         libvirt_constcharPtrWrap("family"),
> > +                         libvirt_intWrap(local->family),
> > +                         cleanup);
> > +    VIR_PY_DICT_SET_GOTO(pyobj_local,
> > +                         libvirt_constcharPtrWrap("node"),
> > +                         libvirt_constcharPtrWrap(local->node),
> > +                         cleanup);
> > +    VIR_PY_DICT_SET_GOTO(pyobj_local,
> > +                         libvirt_constcharPtrWrap("service"),
> > +                         libvirt_constcharPtrWrap(local->service),
> > +                         cleanup);
> >  
> >      if ((pyobj_remote = PyDict_New()) == NULL)
> >          goto cleanup;
> >  
> > -    PyDict_SetItem(pyobj_remote,
> > -                   libvirt_constcharPtrWrap("family"),
> > -                   libvirt_intWrap(remote->family));
> > -    PyDict_SetItem(pyobj_remote,
> > -                   libvirt_constcharPtrWrap("node"),
> > -                   libvirt_constcharPtrWrap(remote->node));
> > -    PyDict_SetItem(pyobj_remote,
> > -                   libvirt_constcharPtrWrap("service"),
> > -                   libvirt_constcharPtrWrap(remote->service));
> > +    VIR_PY_DICT_SET_GOTO(pyobj_remote,
> > +                         libvirt_constcharPtrWrap("family"),
> > +                         libvirt_intWrap(remote->family),
> > +                         cleanup);
> > +    VIR_PY_DICT_SET_GOTO(pyobj_remote,
> > +                         libvirt_constcharPtrWrap("node"),
> > +                         libvirt_constcharPtrWrap(remote->node),
> > +                         cleanup);
> > +    VIR_PY_DICT_SET_GOTO(pyobj_remote,
> > +                         libvirt_constcharPtrWrap("service"),
> > +                         libvirt_constcharPtrWrap(remote->service),
> > +                         cleanup);
> >  
> >      if ((pyobj_subject = PyList_New(subject->nidentity)) == NULL)
> >          goto cleanup;
> > @@ -8011,15 +7958,10 @@ libvirt_virDomainGetTime(PyObject *self ATTRIBUTE_UNUSED,
> >          goto cleanup;
> >      }
> >  
> > -    if (!(pyobj_seconds = libvirt_longlongWrap(seconds)) ||
> > -        PyDict_SetItemString(dict, "seconds", pyobj_seconds) < 0)
> > -        goto cleanup;
> > -    Py_DECREF(pyobj_seconds);
> > -
> > -    if (!(pyobj_nseconds = libvirt_uintWrap(nseconds)) ||
> > -        PyDict_SetItemString(dict, "nseconds", pyobj_nseconds) < 0)
> > -        goto cleanup;
> > -    Py_DECREF(pyobj_nseconds);
> > +    VIR_PY_DICT_SET_GOTO(dict, libvirt_constcharPtrWrap("seconds"),
> > +                         libvirt_longlongWrap(seconds), cleanup);
> > +    VIR_PY_DICT_SET_GOTO(dict, libvirt_constcharPtrWrap("nseconds"),
> > +                         libvirt_longlongWrap(nseconds), cleanup);
> 
> build issue - 'pyobj_seconds' and 'pyobj_nseconds' no longer necessary
> 
> The rest seemed OK
> 
> John
> >  
> >      py_retval = dict;
> >      dict = NULL;
> > @@ -8148,35 +8090,18 @@ libvirt_virNodeGetFreePages(PyObject *self ATTRIBUTE_UNUSED,
> >  
> >      for (i = 0; i < c_retval;) {
> >          PyObject *per_node = NULL;
> > -        PyObject *node = NULL;
> >  
> > -        if (!(per_node = PyDict_New()) ||
> > -            !(node = libvirt_intWrap(startCell + i/pyobj_pagesize_size))) {
> > -            Py_XDECREF(per_node);
> > -            Py_XDECREF(node);
> > +        if (!(per_node = PyDict_New()))
> >              goto cleanup;
> > -        }
> >  
> > -        if (PyDict_SetItem(pyobj_counts, node, per_node) < 0) {
> > -            Py_XDECREF(per_node);
> > -            Py_XDECREF(node);
> > -            goto cleanup;
> > -        }
> > +        VIR_PY_DICT_SET_GOTO(pyobj_counts,
> > +                             libvirt_intWrap(startCell + i/pyobj_pagesize_size),
> > +                             per_node, cleanup);
> > +
> > +        for (j = 0; j < pyobj_pagesize_size; j ++)
> > +            VIR_PY_DICT_SET_GOTO(per_node, libvirt_intWrap(pages[j]),
> > +                                 libvirt_intWrap(counts[i + j]), cleanup);
> >  
> > -        for (j = 0; j < pyobj_pagesize_size; j ++) {
> > -            PyObject *page = NULL;
> > -            PyObject *count = NULL;
> > -
> > -            if (!(page = libvirt_intWrap(pages[j])) ||
> > -                !(count = libvirt_intWrap(counts[i + j])) ||
> > -                PyDict_SetItem(per_node, page, count) < 0) {
> > -                Py_XDECREF(page);
> > -                Py_XDECREF(count);
> > -                Py_XDECREF(per_node);
> > -                Py_XDECREF(node);
> > -                goto cleanup;
> > -            }
> > -        }
> >          i += pyobj_pagesize_size;
> >      }
> >  
> > @@ -8230,30 +8155,24 @@ libvirt_virNetworkGetDHCPLeases(PyObject *self ATTRIBUTE_UNUSED,
> >  
> >          VIR_PY_LIST_SET_GOTO(py_retval, i, py_lease, error);
> >  
> > -#define VIR_SET_LEASE_ITEM(NAME, VALUE_OBJ_FUNC)                            \
> > -        do {                                                                \
> > -            PyObject *tmp_val;                                              \
> > -                                                                            \
> > -            if (!(tmp_val = VALUE_OBJ_FUNC))                                \
> > -                goto error;                                                 \
> > -                                                                            \
> > -            if (PyDict_SetItemString(py_lease, NAME, tmp_val) < 0) {        \
> > -                Py_DECREF(tmp_val);                                         \
> > -                goto error;                                                 \
> > -            }                                                               \
> > -        } while (0)
> > -
> > -        VIR_SET_LEASE_ITEM("iface", libvirt_charPtrWrap(lease->iface));
> > -        VIR_SET_LEASE_ITEM("expirytime", libvirt_longlongWrap(lease->expirytime));
> > -        VIR_SET_LEASE_ITEM("type", libvirt_intWrap(lease->type));
> > -        VIR_SET_LEASE_ITEM("mac", libvirt_charPtrWrap(lease->mac));
> > -        VIR_SET_LEASE_ITEM("ipaddr", libvirt_charPtrWrap(lease->ipaddr));
> > -        VIR_SET_LEASE_ITEM("prefix", libvirt_uintWrap(lease->prefix));
> > -        VIR_SET_LEASE_ITEM("hostname", libvirt_charPtrWrap(lease->hostname));
> > -        VIR_SET_LEASE_ITEM("clientid", libvirt_charPtrWrap(lease->clientid));
> > -        VIR_SET_LEASE_ITEM("iaid", libvirt_charPtrWrap(lease->iaid));
> > -
> > -#undef VIR_SET_LEASE_ITEM
> > +        VIR_PY_DICT_SET_GOTO(py_lease, libvirt_constcharPtrWrap("iface"),
> > +                             libvirt_charPtrWrap(lease->iface), error);
> > +        VIR_PY_DICT_SET_GOTO(py_lease, libvirt_constcharPtrWrap("expirytime"),
> > +                             libvirt_longlongWrap(lease->expirytime), error);
> > +        VIR_PY_DICT_SET_GOTO(py_lease, libvirt_constcharPtrWrap("type"),
> > +                             libvirt_intWrap(lease->type), error);
> > +        VIR_PY_DICT_SET_GOTO(py_lease, libvirt_constcharPtrWrap("mac"),
> > +                             libvirt_charPtrWrap(lease->mac), error);
> > +        VIR_PY_DICT_SET_GOTO(py_lease, libvirt_constcharPtrWrap("ipaddr"),
> > +                             libvirt_charPtrWrap(lease->ipaddr), error);
> > +        VIR_PY_DICT_SET_GOTO(py_lease, libvirt_constcharPtrWrap("prefix"),
> > +                             libvirt_uintWrap(lease->prefix), error);
> > +        VIR_PY_DICT_SET_GOTO(py_lease, libvirt_constcharPtrWrap("hostname"),
> > +                             libvirt_charPtrWrap(lease->hostname), error);
> > +        VIR_PY_DICT_SET_GOTO(py_lease, libvirt_constcharPtrWrap("clientid"),
> > +                             libvirt_charPtrWrap(lease->clientid), error);
> > +        VIR_PY_DICT_SET_GOTO(py_lease, libvirt_constcharPtrWrap("iaid"),
> > +                             libvirt_charPtrWrap(lease->iaid), error);
> >      }
> >  
> >   cleanup:
> > diff --git a/libvirt-utils.c b/libvirt-utils.c
> > index 02a28ac..2bf7519 100644
> > --- a/libvirt-utils.c
> > +++ b/libvirt-utils.c
> > @@ -267,22 +267,13 @@ getPyVirTypedParameter(const virTypedParameter *params,
> >          }
> >  
> >          key = libvirt_constcharPtrWrap(params[i].field);
> > -        if (!key || !val)
> > -            goto cleanup;
> > -
> > -        if (PyDict_SetItem(info, key, val) < 0) {
> > -            Py_DECREF(info);
> > -            goto cleanup;
> > -        }
> >  
> > -        Py_DECREF(key);
> > -        Py_DECREF(val);
> > +        VIR_PY_DICT_SET_GOTO(info, key, val, cleanup);
> >      }
> >      return info;
> >  
> >   cleanup:
> > -    Py_XDECREF(key);
> > -    Py_XDECREF(val);
> > +    Py_DECREF(info);
> >      return NULL;
> >  }
> >  
> > 

--
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]