On 11/12/2013 07:00 AM, Eric Blake wrote: > On 11/12/2013 06:53 AM, Martin Kletzander wrote: >>> @@ -2621,7 +2630,8 @@ libvirt_virDomainSnapshotListChildrenNames(PyObject *self ATTRIBUTE_UNUSED, >>> return VIR_PY_NONE; >>> } >>> } >>> - py_retval = PyList_New(c_retval); >>> + if (!(py_retval = PyList_New(c_retval))) >>> + goto cleanup; >>> >> >> This function should follow others and return VIR_PY_NONE, but cleanup >> path returns py_retval. > > Actually, you WANT to return NULL, not VIR_PY_NONE, when PyList_New() > fails. Returning NULL is the hint to python to report the OOM > exception; Re-reading what I wrote, I kind of implied that OOM error will always be reported for a NULL return; but that's not true - OOM error is the most likely case of failure for PyList_New(), but other calls definitely can have non-OOM reasons for returning NULL. The more general rule is that returning NULL says that python should raise the current python thread-local error information as an exception. Most Py* API are documented as setting thread-local error information (whether OOM or otherwise) before returning NULL. If you have a situation where you are returning NULL to raise a python exception, but did not obtain that NULL from a Py* call, then you have to explicitly set the thread-local error (see for example getPyVirTypedParameter, which gets to reuse PyInt_* conversions in most cases, but has to explicitly call PyErr_Format() for a non-OOM error in the default case; see also the use of PyErr_NoMemory() in setPyVirTypedParameter as a way to explicitly request an OOM error when the allocation failure was not due to any other Py* call). But the general observation remains - we have a lot of crufty code that does error handling incorrectly in our python bindings :) And there's still the question of whether we are going to split python bindings into their own repository; it might be nice to have the code cleaned up before that point. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list