No need to do a NULL check ourself, when we can just use the write Python wrapper. Signed-off-by: Eric Blake <eblake(a)redhat.com&gt; --- plugins/python/python.c | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/plugins/python/python.c b/plugins/python/python.c index 35e8df2..c5cc4bd 100644 --- a/plugins/python/python.c +++ b/plugins/python/python.c @@ -183,8 +183,7 @@ py_load (void) static void py_unload (void) { - if (module) - Py_DECREF (module); + Py_XDECREF (module); Py_Finalize (); } @@ -346,8 +345,7 @@ py_close (void *handle) Py_DECREF (fn); Py_DECREF (args); check_python_failure ("close"); - if (r) - Py_DECREF (r); + Py_XDECREF (r); } Py_DECREF (obj); @@ -550,8 +548,7 @@ py_zero (void *handle, uint32_t count, uint64_t offset, int may_trim) gracefully fall back, and to accomodate both a normal return and an exception. */ nbdkit_debug ("zero requested falling back to pwrite"); - if (r) - Py_DECREF (r); + Py_XDECREF (r); PyErr_Clear (); return -1; } -- 2.14.3

PyObject_CallObject is powerful, but awkward - we have to wrap all arguments into a temporary tuple before using it. Let python do more of the work by using PyObject_CallFunction anywhere that all arguments can be described by a Py_BuildValue() format string, instead of creating one-shot arguments ourselves. In fact, for our py_config(), this makes it easier to not worry about python 2 String vs. python 3 Unicode. Similarly, PyObject_CallFunctionObjArgs is nicer when we already have PyObjects for all parameters (including in py_open(), where we can't use a Py_BuildValue() string for converting a C int into Py_True or Py_False, but can easily avoid the tuple wrapper). py_zero() is not converted, as it will be changed differently to make 'may_trim' an optional callback parameter. Signed-off-by: Eric Blake <eblake(a)redhat.com&gt; --- plugins/python/python.c | 97 ++++++++----------------------------------------- 1 file changed, 15 insertions(+), 82 deletions(-) diff --git a/plugins/python/python.c b/plugins/python/python.c index c5cc4bd..5ece97a 100644 --- a/plugins/python/python.c +++ b/plugins/python/python.c @@ -209,7 +209,6 @@ py_config (const char *key, const char *value) FILE *fp; PyObject *modname; PyObject *fn; - PyObject *args; PyObject *r; if (!script) { @@ -258,17 +257,8 @@ py_config (const char *key, const char *value) /* Other parameters are passed to the Python .config callback. */ PyErr_Clear (); - args = PyTuple_New (2); -#ifdef HAVE_PYSTRING_FROMSTRING - PyTuple_SetItem (args, 0, PyString_FromString (key)); - PyTuple_SetItem (args, 1, PyString_FromString (value)); -#else - PyTuple_SetItem (args, 0, PyUnicode_FromString (key)); - PyTuple_SetItem (args, 1, PyUnicode_FromString (value)); -#endif - r = PyObject_CallObject (fn, args); + r = PyObject_CallFunction (fn, "ss", key, value); Py_DECREF (fn); - Py_DECREF (args); if (check_python_failure ("config") == -1) return -1; Py_DECREF (r); @@ -306,7 +296,6 @@ static void * py_open (int readonly) { PyObject *fn; - PyObject *args; PyObject *handle; if (!callback_defined ("open", &fn)) { @@ -316,11 +305,9 @@ py_open (int readonly) PyErr_Clear (); - args = PyTuple_New (1); - PyTuple_SetItem (args, 0, PyBool_FromLong (readonly)); - handle = PyObject_CallObject (fn, args); + handle = PyObject_CallFunctionObjArgs (fn, readonly ? Py_True : Py_False, + NULL); Py_DECREF (fn); - Py_DECREF (args); if (check_python_failure ("open") == -1) return NULL; @@ -332,18 +319,13 @@ py_close (void *handle) { PyObject *obj = handle; PyObject *fn; - PyObject *args; PyObject *r; if (callback_defined ("close", &fn)) { PyErr_Clear (); - args = PyTuple_New (1); - Py_INCREF (obj); /* decremented by Py_DECREF (args) */ - PyTuple_SetItem (args, 0, obj); - r = PyObject_CallObject (fn, args); + r = PyObject_CallFunctionObjArgs (fn, obj, NULL); Py_DECREF (fn); - Py_DECREF (args); check_python_failure ("close"); Py_XDECREF (r); } @@ -356,7 +338,6 @@ py_get_size (void *handle) { PyObject *obj = handle; PyObject *fn; - PyObject *args; PyObject *r; int64_t ret; @@ -367,12 +348,8 @@ py_get_size (void *handle) PyErr_Clear (); - args = PyTuple_New (1); - Py_INCREF (obj); /* decremented by Py_DECREF (args) */ - PyTuple_SetItem (args, 0, obj); - r = PyObject_CallObject (fn, args); + r = PyObject_CallFunctionObjArgs (fn, obj, NULL); Py_DECREF (fn); - Py_DECREF (args); if (check_python_failure ("get_size") == -1) return -1; @@ -390,7 +367,6 @@ py_pread (void *handle, void *buf, { PyObject *obj = handle; PyObject *fn; - PyObject *args; PyObject *r; if (!callback_defined ("pread", &fn)) { @@ -400,14 +376,8 @@ py_pread (void *handle, void *buf, PyErr_Clear (); - args = PyTuple_New (3); - Py_INCREF (obj); /* decremented by Py_DECREF (args) */ - PyTuple_SetItem (args, 0, obj); - PyTuple_SetItem (args, 1, PyLong_FromLong (count)); - PyTuple_SetItem (args, 2, PyLong_FromUnsignedLongLong (offset)); - r = PyObject_CallObject (fn, args); + r = PyObject_CallFunction (fn, "OiL", obj, count, offset, NULL); Py_DECREF (fn); - Py_DECREF (args); if (check_python_failure ("pread") == -1) return -1; @@ -436,20 +406,15 @@ py_pwrite (void *handle, const void *buf, { PyObject *obj = handle; PyObject *fn; - PyObject *args; PyObject *r; if (callback_defined ("pwrite", &fn)) { PyErr_Clear (); - args = PyTuple_New (3); - Py_INCREF (obj); /* decremented by Py_DECREF (args) */ - PyTuple_SetItem (args, 0, obj); - PyTuple_SetItem (args, 1, PyByteArray_FromStringAndSize (buf, count)); - PyTuple_SetItem (args, 2, PyLong_FromUnsignedLongLong (offset)); - r = PyObject_CallObject (fn, args); + r = PyObject_CallFunction (fn, "OOL", obj, + PyByteArray_FromStringAndSize (buf, count), + offset, NULL); Py_DECREF (fn); - Py_DECREF (args); if (check_python_failure ("pwrite") == -1) return -1; Py_DECREF (r); @@ -467,18 +432,13 @@ py_flush (void *handle) { PyObject *obj = handle; PyObject *fn; - PyObject *args; PyObject *r; if (callback_defined ("flush", &fn)) { PyErr_Clear (); - args = PyTuple_New (1); - Py_INCREF (obj); /* decremented by Py_DECREF (args) */ - PyTuple_SetItem (args, 0, obj); - r = PyObject_CallObject (fn, args); + r = PyObject_CallFunctionObjArgs (fn, obj, NULL); Py_DECREF (fn); - Py_DECREF (args); if (check_python_failure ("flush") == -1) return -1; Py_DECREF (r); @@ -496,20 +456,13 @@ py_trim (void *handle, uint32_t count, uint64_t offset) { PyObject *obj = handle; PyObject *fn; - PyObject *args; PyObject *r; if (callback_defined ("trim", &fn)) { PyErr_Clear (); - args = PyTuple_New (3); - Py_INCREF (obj); /* decremented by Py_DECREF (args) */ - PyTuple_SetItem (args, 0, obj); - PyTuple_SetItem (args, 1, PyLong_FromLong (count)); - PyTuple_SetItem (args, 2, PyLong_FromUnsignedLongLong (offset)); - r = PyObject_CallObject (fn, args); + r = PyObject_CallFunction (fn, "OiL", obj, count, offset, NULL); Py_DECREF (fn); - Py_DECREF (args); if (check_python_failure ("trim") == -1) return -1; Py_DECREF (r); @@ -568,19 +521,14 @@ py_can_write (void *handle) { PyObject *obj = handle; PyObject *fn; - PyObject *args; PyObject *r; int ret; if (callback_defined ("can_write", &fn)) { PyErr_Clear (); - args = PyTuple_New (1); - Py_INCREF (obj); /* decremented by Py_DECREF (args) */ - PyTuple_SetItem (args, 0, obj); - r = PyObject_CallObject (fn, args); + r = PyObject_CallFunctionObjArgs (fn, obj, NULL); Py_DECREF (fn); - Py_DECREF (args); if (check_python_failure ("can_write") == -1) return -1; ret = r == Py_True; @@ -601,19 +549,14 @@ py_can_flush (void *handle) { PyObject *obj = handle; PyObject *fn; - PyObject *args; PyObject *r; int ret; if (callback_defined ("can_flush", &fn)) { PyErr_Clear (); - args = PyTuple_New (1); - Py_INCREF (obj); /* decremented by Py_DECREF (args) */ - PyTuple_SetItem (args, 0, obj); - r = PyObject_CallObject (fn, args); + r = PyObject_CallFunctionObjArgs (fn, obj, NULL); Py_DECREF (fn); - Py_DECREF (args); if (check_python_failure ("can_flush") == -1) return -1; ret = r == Py_True; @@ -634,19 +577,14 @@ py_is_rotational (void *handle) { PyObject *obj = handle; PyObject *fn; - PyObject *args; PyObject *r; int ret; if (callback_defined ("is_rotational", &fn)) { PyErr_Clear (); - args = PyTuple_New (1); - Py_INCREF (obj); /* decremented by Py_DECREF (args) */ - PyTuple_SetItem (args, 0, obj); - r = PyObject_CallObject (fn, args); + r = PyObject_CallFunctionObjArgs (fn, obj, NULL); Py_DECREF (fn); - Py_DECREF (args); if (check_python_failure ("is_rotational") == -1) return -1; ret = r == Py_True; @@ -662,19 +600,14 @@ py_can_trim (void *handle) { PyObject *obj = handle; PyObject *fn; - PyObject *args; PyObject *r; int ret; if (callback_defined ("can_trim", &fn)) { PyErr_Clear (); - args = PyTuple_New (1); - Py_INCREF (obj); /* decremented by Py_DECREF (args) */ - PyTuple_SetItem (args, 0, obj); - r = PyObject_CallObject (fn, args); + r = PyObject_CallFunctionObjArgs (fn, obj, NULL); Py_DECREF (fn); - Py_DECREF (args); if (check_python_failure ("can_trim") == -1) return -1; ret = r == Py_True; -- 2.14.3

On 04/06/2018 05:24 PM, Eric Blake wrote: In fact, this patch is a corner-case bug fix. Here, and in all sorts of similar places, we are failing to check for args == NULL (unlikely except on OOM conditions, but still a valid error condition, where we would presumably have a nice Python error object to report failure with)... ...and instead passing NULL on to a function that would crash. We are likewise not checking for errors here (although such errors should never be possible). The replacement code is not only shorter, it has fewer places to worry about corner-case boilerplate error checking. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org