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

On 09/24/2015 10:01 AM, Pavel Hrdina wrote: > Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com&gt; > --- > 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

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

> - 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; > } > >