Re: [Libguestfs] [PATCH 1/2] python: Avoid crash if callback parameters cannot be built

From: Eric Blake <eblake@redhat.com> To: Laszlo Ersek <lersek@redhat.com>, docs@python.org Cc: Nir Soffer <nsoffer@redhat.com>, aryeyur@google.com, libguestfs@redhat.com, shtarkman@google.com Bcc: Subject: Re: [Libguestfs] [PATCH 1/2] python: Avoid crash if callback parameters cannot be built Reply-To: In-Reply-To: <c8259881-9fac-5987-8ac1-3286ff35adc1@redhat.com> [adding docs@python.org, in the hopes that this conversation can spur an improvement in Python's documentation] I'm trimming to just context on the doc issue at hand (this question spawned deep within a thread [1] to libguestfs reference counting bug in its Python bindings) [1] https://listman.redhat.com/archives/libguestfs/2023-February/030663.html On Tue, Feb 21, 2023 at 03:06:47PM +0100, Laszlo Ersek wrote:
In short, the issue is that the Python documentation (even in the latest version at https://docs.python.org/3/c-api/arg.html) has TWO separate paradigms on the SAME page, and so many format characters that you can easily get lost in a wall of X (foo) [bar] designators that you can easily lose track of whether you are reading the docs for PyArg_ParseTuple() and friends with paradigm "X (Python source) [C destination]", or the docs for Py_BuildValue() with paradigm "X (Python destination) [C source]". When it comes to reference counting, there is a HUGE and IMPORTANT difference between the former: O (object) [PyObject *] Store a Python object (without any conversion) in a C object pointer. The C program thus receives the actual object that was passed. The object’s reference count is not increased. The pointer stored is not NULL. and the latter: O (object) [PyObject *] Pass a Python object untouched (except for its reference count, which is incremented by one). If the object passed in is a NULL pointer, it is assumed that this was caused because the call producing the argument found an error and set an exception. Therefore, Py_BuildValue() will return NULL but won’t raise an exception. If no exception has been raised yet, SystemError is set. But as neither instance of "O (object) [PyObject *]" is on the screen at the same time as the functions to which that format specifier is documenting, it is EASY for a novice coder to read the wrong one, and then get the wrong ideas about reference counting when using the "O" format specifier. My suggestion: a simple swap of the order of the Py_BuildValue() notations to: PyArg_ParseValue()... X (Python source) [C destination] Py_BuildValue()... X [C source] (Python destination] so that it is always source before destination, and always [] for the C counterpart and () for the Python counterpart. Another idea might be splitting the page into two, so that the two walls of format specifier text cannot be mistaken for one another when rapidly scrolling through the page looking for a given format letter; but I personally like both on the same page as the two sets of format specifiers are (intentionally) related, even if subtly different depending on direction. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
participants (1)
-
Eric Blake