Libguestfs - Apr 2018 - [nbdkit PATCH] python: Let zero's may_trim parameter be optional

In preparation for adding other optional flag arguments to the
python bindings, start by making the existing 'may_trim' flag
to 'zero' be optional.  That is, the plugin need not define
the parameter if it does not make any semantic difference (ie.
if the plugin ignores the hint and never trims); while if the
parameter exists, we now pass it as a keyword argument rather
than as a positional argument.  This requires the plugin to
give the parameter a specific name, and works whether or not
the plugin provides a default for the parameter (although we do
now recommend that plugins provide a default value, as it makes
plugins built against newer nbdkit more likely to run even on
older nbdkit).  We can't see through a plugin that used
'**kwargs', but that is less likely.

If we are super-worried about back-compatibility to older
plugins which hard-coded 4 required parameters, we could also
tweak the introspection to assume that a 'zero' with 4
parameters and no defaults also supports 'may_trim', but pass
it via positional rather than a keyword argument. However, I
suspect most plugins just copied from our examples, which means
they used the right parameter naming to just keep working.

The introspection framework here will also make it easy to
probe for future flag additions.

Signed-off-by: Eric Blake <eblake@redhat.com>
---

At least the plugin for rhv-upload [1] used the right naming:

[1]
https://github.com/libguestfs/libguestfs/blob/53b31df7c/v2v/rhv-upload-plugin.py#L276

 plugins/python/nbdkit-python-plugin.pod | 35 +++++++++++----
 plugins/python/python.c                 | 80 +++++++++++++++++++++++++++++++--
 plugins/python/example.py               |  2 +-
 tests/test.py                           |  2 +-
 4 files changed, 105 insertions(+), 14 deletions(-)

diff --git a/plugins/python/nbdkit-python-plugin.pod
b/plugins/python/nbdkit-python-plugin.pod
index c3a564e..19f9eff 100644
--- a/plugins/python/nbdkit-python-plugin.pod
+++ b/plugins/python/nbdkit-python-plugin.pod
@@ -51,11 +51,24 @@ C<__main__> module):
  def pread(h, count, offset):
    # see below

-Note that the subroutines must have those literal names (like C<open>),
-because the C part looks up and calls those functions directly.  You
-may want to include documentation and globals (eg. for storing global
-state).  Any other top level statements are run when the script is
-loaded, just like ordinary Python.
+Note that the functions must have those literal names (like C<open>),
+because the C part looks up and calls those functions directly.  Where
+this documentation lists a parameter as mandatory, you can name the
+parameter what you would like (the C part sets that parameter
+positionally); however, where this documentation lists a parameter
+with a default value, you must either omit that parameter or use that
+exact parameter name (the C part inspects the function signature to
+see whether your callback implemented that parameter, and if so, sets
+the parameter via keyword).  In this way, newer versions of nbdkit can
+add additional flags with default values that can be useful to newer
+plugins, but still run older plugins without requiring them to be
+rewritten to add support for the new flags.  Note that using
+C<**kwargs> in your function instead of named parameters would defeat
+the C code that inspects the signature.
+
+You may want to include documentation and globals (eg. for storing
+global state).  Any other top level statements are run when the script
+is loaded, just like ordinary Python.

 =head2 EXECUTABLE SCRIPT

@@ -231,13 +244,17 @@ exception, optionally using C<nbdkit.set_error>
first.

 (Optional)

- def zero(h, count, offset, may_trim):
+ def zero(h, count, offset, may_trim=False):
    # no return value

 The body of your C<zero> function should ensure that C<count> bytes
-of the disk, starting at C<offset>, will read back as zero.  If
-C<may_trim> is true, the operation may be optimized as a trim as long
-as subsequent reads see zeroes.
+of the disk, starting at C<offset>, will read back as zero.
+
+Your function may support an optional C<may_trim> parameter; if it is
+present and the caller sets it to True, then your callback may
+optimize the operation by using a trim, as long as subsequent reads
+see zeroes.  If you omit the optional parameter, or if the caller sets
+it to False, writing zeroes should not punch holes.

 NBD only supports whole writes, so your function should try to
 write the whole region (perhaps requiring a loop).  If the write
diff --git a/plugins/python/python.c b/plugins/python/python.c
index 35e8df2..d944905 100644
--- a/plugins/python/python.c
+++ b/plugins/python/python.c
@@ -64,6 +64,8 @@ static PyObject *module;

 static int last_error;

+static int zero_may_trim = -1;
+
 static PyObject *
 set_error (PyObject *self, PyObject *args)
 {
@@ -108,6 +110,68 @@ callback_defined (const char *name, PyObject **obj_rtn)
   return 1;
 }

+/* Checks whether a list of strings contains the given name */
+static int
+check_list (PyObject *list, const char *name)
+{
+  ssize_t i = 0;
+  PyObject *elt;
+
+  if (!list)
+    return 0;
+  while ((elt = PyList_GetItem(list, i++))) {
+    char *str = PyString_AsString(elt);
+    if (str && !strcmp(str, name))
+      return 1;
+  }
+  return 0;
+}
+
+/* Does a callback support the given keyword parameter? */
+static int
+callback_has_parameter (PyObject *fn, const char *name)
+{
+  int r = 0;
+  PyObject *inspect, *pname, *spec, *args;
+
+  assert (script != NULL);
+  assert (module != NULL);
+
+  pname = PyString_FromString ("inspect");
+  if (!pname)
+    return -1;
+  inspect = PyImport_Import (pname);
+  Py_DECREF (pname);
+
+  if (!inspect)
+    return -1;
+
+#if PY_MAJOR_VERSION >= 3
+  pname = PyString_FromString ("getfullargspec");
+#else
+  pname = PyString_FromString ("getargspec");
+#endif
+  spec = PyObject_CallMethodObjArgs (inspect, pname, fn, NULL);
+  Py_DECREF (pname);
+  Py_DECREF (inspect);
+  if (!spec)
+    return -1;
+
+  /* Yay, we got the signature; now inspect if it contains keyword
'name' */
+  args = PyTuple_GetItem(spec, 0);
+  if (check_list(args, name))
+    r = 1;
+  else {
+    /* inspecting kwonly args is only available in python 3 */
+    args = PyTuple_GetItem(spec, 5);
+    if (check_list(args, name))
+      r = 1;
+  }
+
+  Py_DECREF (spec);
+  return r;
+}
+
 /* Convert bytes/str/unicode into a string.  Caller must free. */
 static char *
 python_to_string (PyObject *str)
@@ -530,21 +594,31 @@ py_zero (void *handle, uint32_t count, uint64_t offset,
int may_trim)
   PyObject *obj = handle;
   PyObject *fn;
   PyObject *args;
+  PyObject *kwargs = NULL;
   PyObject *r;

   if (callback_defined ("zero", &fn)) {
+    if (zero_may_trim < 0)
+      zero_may_trim = callback_has_parameter (fn, "may_trim");
+    if (zero_may_trim < 0) {
+      check_python_failure ("zero");
+      return -1;
+    }
+
     PyErr_Clear ();

     last_error = 0;
-    args = PyTuple_New (4);
+    args = PyTuple_New (3);
     Py_INCREF (obj); /* decremented by Py_DECREF (args) */
     PyTuple_SetItem (args, 0, obj);
     PyTuple_SetItem (args, 1, PyLong_FromUnsignedLongLong (count));
     PyTuple_SetItem (args, 2, PyLong_FromUnsignedLongLong (offset));
-    PyTuple_SetItem (args, 3, PyBool_FromLong (may_trim));
-    r = PyObject_CallObject (fn, args);
+    if (zero_may_trim)
+      kwargs = Py_BuildValue("{s:i}", "may_trim",
may_trim);
+    r = PyObject_Call (fn, args, kwargs);
     Py_DECREF (fn);
     Py_DECREF (args);
+    Py_XDECREF (kwargs);
     if (last_error == EOPNOTSUPP) {
       /* When user requests this particular error, we want to
          gracefully fall back, and to accomodate both a normal return
diff --git a/plugins/python/example.py b/plugins/python/example.py
index 60f9d7f..9205f10 100644
--- a/plugins/python/example.py
+++ b/plugins/python/example.py
@@ -65,7 +65,7 @@ def pwrite(h, buf, offset):
     disk[offset:end] = buf


-def zero(h, count, offset, may_trim):
+def zero(h, count, offset, may_trim=False):
     global disk
     if may_trim:
         disk[offset:offset+count] = bytearray(count)
diff --git a/tests/test.py b/tests/test.py
index 630ac2f..518cdd4 100644
--- a/tests/test.py
+++ b/tests/test.py
@@ -41,7 +41,7 @@ def pwrite(h, buf, offset):
     disk[offset:end] = buf


-def zero(h, count, offset, may_trim=False):
+def zero(h, count, offset):
     global disk
     disk[offset:offset+count] = bytearray(count)

-- 
2.14.3

On 04/06/2018 09:52 AM, Eric Blake wrote:
> In preparation for adding other optional flag arguments to the
> python bindings, start by making the existing 'may_trim' flag
> to 'zero' be optional.  That is, the plugin need not define
> the parameter if it does not make any semantic difference (ie.
> if the plugin ignores the hint and never trims); while if the
> parameter exists, we now pass it as a keyword argument rather
> than as a positional argument.  This requires the plugin to
> give the parameter a specific name, and works whether or not
> the plugin provides a default for the parameter (although we do
> now recommend that plugins provide a default value, as it makes
> plugins built against newer nbdkit more likely to run even on
> older nbdkit).  We can't see through a plugin that used
> '**kwargs', but that is less likely.
> 
So far, I've only tested under Python 2, on Fedora 27.  Before
committing, I'll have to re-test with python 3.  I already noted that:
> +/* Does a callback support the given keyword parameter? */
> +static int
> +callback_has_parameter (PyObject *fn, const char *name)
> +{
> +  int r = 0;
> +  PyObject *inspect, *pname, *spec, *args;
> +
> +  assert (script != NULL);
> +  assert (module != NULL);
> +
> +  pname = PyString_FromString ("inspect");
Needs python 2/3 protection using HAVE_PYSTRING_FROMSTRING.
> +  if (!pname)
> +    return -1;
> +  inspect = PyImport_Import (pname);
> +  Py_DECREF (pname);
> +
> +  if (!inspect)
> +    return -1;
> +
> +#if PY_MAJOR_VERSION >= 3
> +  pname = PyString_FromString ("getfullargspec");
and this should use PyUnicode_FromString.
> +#else
> +  pname = PyString_FromString ("getargspec");
> +#endif
> +  spec = PyObject_CallMethodObjArgs (inspect, pname, fn, NULL);
> +  Py_DECREF (pname);
> +  Py_DECREF (inspect);
> +  if (!spec)
> +    return -1;
> +
> @@ -530,21 +594,31 @@ py_zero (void *handle, uint32_t count, uint64_t
offset, int may_trim)
>      PyTuple_SetItem (args, 2, PyLong_FromUnsignedLongLong (offset));
> -    PyTuple_SetItem (args, 3, PyBool_FromLong (may_trim));
> -    r = PyObject_CallObject (fn, args);
> +    if (zero_may_trim)
> +      kwargs = Py_BuildValue("{s:i}", "may_trim",
may_trim);
I also wonder if Py_BuildValue() is any easier to use instead of
conditionals on HAVE_PYSTRING_FROMSTRING.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org