thr3ads.net - Libguestfs - [Libguestfs] [nbdkit PATCH 0/2] Python cleanups [Apr 2018]

If this information is useful, please help other people find it:
Share via:

Eric Blake

2018-Apr-06 22:24 UTC

[Libguestfs] [nbdkit PATCH 0/2] Python cleanups

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

Eric Blake

2018-Apr-06 22:24 UTC

head link

[Libguestfs] [nbdkit PATCH 1/2] python: Use Py_XDEFREF()

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

head link

[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

head link

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

head link

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

head link

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

head link

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

Search for more possibly parallel threads

Libguestfs - Apr 2018 - [nbdkit PATCH 0/2] Python cleanups

[Libguestfs] [nbdkit PATCH 0/2] Python cleanups

[Libguestfs] [nbdkit PATCH 1/2] python: Use Py_XDEFREF()

[Libguestfs] [nbdkit PATCH 2/2] python: Simplify calling into plugin

Re: [Libguestfs] [nbdkit PATCH 1/2] python: Use Py_XDEFREF()

Re: [Libguestfs] [nbdkit PATCH 2/2] python: Simplify calling into plugin

Re: [Libguestfs] [nbdkit PATCH 2/2] python: Simplify calling into plugin

Re: [Libguestfs] [nbdkit PATCH 0/2] Python cleanups

Reasonably Related Threads