[libvirt] [libvirt-python PATCH 22/23] use VIR_PY_DICT_SET_GOTO
Pavel Hrdina
phrdina at redhat.com
Tue Sep 29 16:46:21 UTC 2015
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 at redhat.com>
> > ---
> > 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;
> > }
> >
> >
More information about the libvir-list
mailing list