On 2/17/23 18:45, Richard W.M. Jones wrote: 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_Bui... (when it succeeds). With those notes: Reviewed-by: Laszlo Ersek <lersek(a)redhat.com&gt;

On 2/17/23 18:45, Richard W.M. Jones wrote: 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... 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... 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(a)redhat.com&gt;