On 2/17/23 18:45, Richard W.M. Jones wrote:

This reverts commit 85235aec837716f1ddb2926b9a59a02543195500. --- python/handle.c | 1 - 1 file changed, 1 deletion(-)

diff --git a/python/handle.c b/python/handle.c index bf639b5789..8eeabe60a7 100644 --- a/python/handle.c +++ b/python/handle.c @@ -139,7 +139,6 @@ guestfs_int_py_event_callback_wrapper (guestfs_h *g, goto out; }

- Py_INCREF (args); py_r = PyObject_CallObject (py_callback, args); Py_DECREF (args); if (py_r != NULL)

The patch looks good, but I think the commit's subject line, and the commit message, should be a bit more clear / verbose: - we're only reverting a part of commit 85235aec837716f1ddb2926b9a59a02543195500 - the reason for doing this is that Py_BuildValue() always returns a new reference <https://docs.python.org/3/c-api/arg.html?highlight=py_buildvalue#c.Py_BuildValue> (when it succeeds). With those notes: Reviewed-by: Laszlo Ersek <lersek@redhat.com>

On 2/17/23 18:45, Richard W.M. Jones wrote:

See also:

https://listman.redhat.com/archives/libguestfs/2023-February/030730.html https://listman.redhat.com/archives/libguestfs/2023-February/030745.html https://listman.redhat.com/archives/libguestfs/2023-February/030746.html

Fixes: commit 6ef5837e2d8c5d4d83eff51c0201eb2e08f719de Thanks: Laszlo Ersek, Eric Blake --- python/handle.c | 1 + 1 file changed, 1 insertion(+)

diff --git a/python/handle.c b/python/handle.c index 8eeabe60a7..85089e6bce 100644 --- a/python/handle.c +++ b/python/handle.c @@ -134,6 +134,7 @@ guestfs_int_py_event_callback_wrapper (guestfs_h *g, args = Py_BuildValue ("(Kiy#O)", (unsigned PY_LONG_LONG) event, event_handle, buf, buf_len, py_array); + Py_DECREF (py_array); if (args == NULL) { PyErr_PrintEx (0); goto out;

I *think* doing it like this is safe in this instance. I got concerned for a minute because of: https://docs.python.org/3/c-api/refcounting.html?highlight=py_decref#c.Py_DE... Warning The deallocation function can cause arbitrary Python code to be invoked (e.g. when a class instance with a __del__() method is deallocated). While exceptions in such code are not propagated, the executed code has free access to all Python global variables. This means that any object that is reachable from a global variable should be in a consistent state before Py_DECREF() is invoked. For example, code to delete an object from a list should copy a reference to the deleted object in a temporary variable, update the list data structure, and then call Py_DECREF() for the temporary variable. and: https://docs.python.org/3/c-api/exceptions.html?highlight=pyerr_printex#c.Py... Call this function only when the error indicator is set. Otherwise it will cause a fatal error! So I had two concerns: - can Py_DECREF() clear the error indicator? - can Py_DECREF() lead to the exception (pending form Py_BuildValue()) being replaced (masked?) with a different exception? I think the code is OK: - My reading of the docs implies that the error indicator only gets cleared with explicit PyErr_Clear() or PyErr_PrintEx() calls. - We're only releasing a simple list of longs, so no particular exception should be expected while destructing that. And the testing described in the cover letter confirms this. Reviewed-by: Laszlo Ersek <lersek@redhat.com>