I noticed these while working on adding fua support into python, these are independent enough to push now (and I'll have to rebase my 'optional may_trim' patch on top of this). Eric Blake (2): python: Use Py_XDEFREF() python: Simplify calling into plugin plugins/python/python.c | 106 ++++++++---------------------------------------- 1 file changed, 18 insertions(+), 88 deletions(-) -- 2.14.3
No need to do a NULL check ourself, when we can just use the
write Python wrapper.
Signed-off-by: Eric Blake <eblake@redhat.com>
---
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
Eric Blake
2018-Apr-06 22:24 UTC
[Libguestfs] [nbdkit PATCH 2/2] python: Simplify calling into plugin
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@redhat.com>
---
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
Eric Blake
2018-Apr-07 11:30 UTC
Re: [Libguestfs] [nbdkit PATCH 1/2] python: Use Py_XDEFREF()
On 04/06/2018 05:24 PM, Eric Blake wrote:> No need to do a NULL check ourself, when we can just use the > write Python wrapper.s/write/right/ (I should quit writing commit messages late in the day...)> > Signed-off-by: Eric Blake <eblake@redhat.com> > --- > 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); >-- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Eric Blake
2018-Apr-07 11:50 UTC
Re: [Libguestfs] [nbdkit PATCH 2/2] python: Simplify calling into plugin
On 04/06/2018 05:24 PM, Eric Blake wrote:> 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). >> @@ -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);This leaks the PyByteArray object. Will fix. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Eric Blake
2018-Apr-09 13:15 UTC
Re: [Libguestfs] [nbdkit PATCH 2/2] python: Simplify calling into plugin
On 04/06/2018 05:24 PM, Eric Blake wrote:> 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@redhat.com> > --- > plugins/python/python.c | 97 ++++++++----------------------------------------- > 1 file changed, 15 insertions(+), 82 deletions(-) >> @@ -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);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)...> -#ifdef HAVE_PYSTRING_FROMSTRING > - PyTuple_SetItem (args, 0, PyString_FromString (key));...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).> - 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);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
Richard W.M. Jones
2018-Apr-10 11:33 UTC
Re: [Libguestfs] [nbdkit PATCH 0/2] Python cleanups
On Fri, Apr 06, 2018 at 05:24:17PM -0500, Eric Blake wrote:> I noticed these while working on adding fua support into python, > these are independent enough to push now (and I'll have to rebase > my 'optional may_trim' patch on top of this). > > Eric Blake (2): > python: Use Py_XDEFREF() > python: Simplify calling into plugin > > plugins/python/python.c | 106 ++++++++---------------------------------------- > 1 file changed, 18 insertions(+), 88 deletions(-)Yes these patches seem fine to me, ACK. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-df lists disk usage of guests without needing to install any software inside the virtual machine. Supports Linux and Windows. http://people.redhat.com/~rjones/virt-df/
Reasonably Related Threads
- [nbdkit PATCH v2 5/5] RFC: python: Track and cache per-connection state in C struct
- Re: [nbdkit PATCH v2 5/5] RFC: python: Track and cache per-connection state in C struct
- [PATCH nbdkit v2 05/10] python: Share common code in boolean callbacks.
- [nbdkit PATCH v2 1/5] python: Let zero's may_trim parameter be optional
- [PATCH nbdkit v2 01/10] python: Use PyObject_CallFunction instead of constructing the tuple.