[libvirt] [PATCH 2/2] python: add error checking for calls to PyList_New

[Eric Blake]

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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 621 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20131112/d013c9f1/attachment-0001.sig>