Libguestfs - Apr 2018 - Re: [nbdkit PATCH v2 3/5] python: Update internals to plugin API level 2

First out of our four language bindings to add FUA support (for
reference, I added 'zero' support for python, perl, and ruby
back in 1.1.13, then Rich had to add it for ocaml in 1.1.20).
I tested this heavily under python 2, but for now only compile
tested under python 3; I plan to do further testing there and
make any tweaks if necessary.

I wrote patch 5 early on, but then realized I probably don't have
a use for it if we fix the C code to cache for all plugins, so
it's more for information than for applying.  (It would matter
if we ever had any per-connection state beyond the script's
handle, and which would not also be cached on our behalf by
the main nbdkit code).

Something I thought of while writing patch 4, but haven't explored
yet, would be using attribute((cleanup)) to automatically call
Py_XDECREF() in a lot more places, for more compact code (it felt
like a lot of boilerplate to avoid leaks of python objects on all
exit paths).

Eric Blake (5):
  python: Let zero's may_trim parameter be optional
  python: Expose can_zero callback
  python: Update internals to plugin API level 2
  python: Expose FUA support
  RFC: python: Track and cache per-connection state in C struct

 plugins/python/nbdkit-python-plugin.pod |  85 ++++++-
 plugins/python/python.c                 | 388 ++++++++++++++++++++++++++------
 plugins/python/example.py               |   6 +-
 tests/test.py                           |   6 +-
 4 files changed, 408 insertions(+), 77 deletions(-)

-- 
2.14.3

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, although that's not the direction we typically
guarantee).  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, where we do not recognize the
fourth parameter name, is an old-style plugin, and call it
with may_trim passed via a 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; furthermore, while we have been clear
that backwards API compatibility is essential for C, we have
not made the same guarantee for language plugins.

The introspection framework here will also make it easy to
probe for future flag additions (support for 'fua' being the
obvious candidate in the short term).

This patch also fixes failure to check for (unlikely) errors
during creation of 'args' within py_zero().

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 plugins/python/nbdkit-python-plugin.pod |  35 ++++++++---
 plugins/python/python.c                 | 108 +++++++++++++++++++++++++++++---
 plugins/python/example.py               |   2 +-
 tests/test.py                           |   2 +-
 4 files changed, 129 insertions(+), 18 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 7eb91d7..07559a5 100644
--- a/plugins/python/python.c
+++ b/plugins/python/python.c
@@ -108,6 +108,73 @@ 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);
+
+#ifdef HAVE_PYSTRING_FROMSTRING
+  pname = PyString_FromString ("inspect");
+#else
+  pname = PyUnicode_FromString ("inspect");
+#endif
+  if (!pname)
+    return -1;
+  inspect = PyImport_Import (pname);
+  Py_DECREF (pname);
+
+  if (!inspect)
+    return -1;
+
+#if PY_MAJOR_VERSION >= 3
+  pname = PyUnicode_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;
+
+  /* We got the signature; now check for the requested keyword */
+  args = PyTuple_GetItem (spec, 0);
+  if (check_list (args, name))
+    r = 1;
+#if PY_MAJOR_VERSION >= 3
+  else {
+    args = PyTuple_GetItem (spec, 5);
+    if (check_list (args, name))
+      r = 1;
+  }
+#endif
+
+  Py_DECREF (spec);
+  return r;
+}
+
 /* Convert bytes/str/unicode into a string.  Caller must free. */
 static char *
 python_to_string (PyObject *str)
@@ -481,21 +548,48 @@ py_zero (void *handle, uint32_t count, uint64_t offset,
int may_trim)
   PyObject *obj = handle;
   PyObject *fn;
   PyObject *args;
+  PyObject *kwargs;
   PyObject *r;

   if (callback_defined ("zero", &fn)) {
+    static int zero_may_trim = -1;
+
+    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);
-    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);
+    args = Py_BuildValue ("OiL", obj, count, offset);
+    if (!args) {
+      check_python_failure ("zero");
+      Py_DECREF (fn);
+      return -1;
+    }
+    kwargs = PyDict_New ();
+    if (!kwargs) {
+      check_python_failure ("zero");
+      Py_DECREF (args);
+      Py_DECREF (fn);
+      return -1;
+    }
+    if (zero_may_trim &&
+        PyDict_SetItemString (kwargs, "may_trim",
+                              may_trim ? Py_True : Py_False) == -1) {
+      check_python_failure ("zero");
+      Py_DECREF (kwargs);
+      Py_DECREF (args);
+      Py_DECREF (fn);
+      return -1;
+    }
+    r = PyObject_Call (fn, args, kwargs);
     Py_DECREF (fn);
     Py_DECREF (args);
+    Py_DECREF (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

Fairly straightforward, as the code in plugins.c already takes
care of necessary .pwrite fallbacks.

Simplify similar can_FOO callbacks that were copy-and-pasted while
implementing py_can_zero.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 plugins/python/nbdkit-python-plugin.pod |  7 +++++
 plugins/python/python.c                 | 51 +++++++++++++++++++--------------
 tests/test.py                           |  4 +++
 3 files changed, 41 insertions(+), 21 deletions(-)

diff --git a/plugins/python/nbdkit-python-plugin.pod
b/plugins/python/nbdkit-python-plugin.pod
index 19f9eff..29250ce 100644
--- a/plugins/python/nbdkit-python-plugin.pod
+++ b/plugins/python/nbdkit-python-plugin.pod
@@ -183,6 +183,13 @@ contents will be garbage collected.
  def can_trim(h):
    # return a boolean

+=item C<can_zero>
+
+(Optional)
+
+ def can_zero(h):
+   # return a boolean
+
 =item C<pread>

 (Required)
diff --git a/plugins/python/python.c b/plugins/python/python.c
index 07559a5..d75b36a 100644
--- a/plugins/python/python.c
+++ b/plugins/python/python.c
@@ -629,13 +629,8 @@ py_can_write (void *handle)
     Py_DECREF (r);
     return ret;
   }
-  /* No Python can_write callback, but there's a Python pwrite callback
-   * defined, so return 1.  (In C modules, nbdkit would do this).
-   */
-  else if (callback_defined ("pwrite", NULL))
-    return 1;
-  else
-    return 0;
+  /* No Python can_write callback, so check for Python pwrite callback. */
+  return callback_defined ("pwrite", NULL);
 }

 static int
@@ -657,13 +652,8 @@ py_can_flush (void *handle)
     Py_DECREF (r);
     return ret;
   }
-  /* No Python can_flush callback, but there's a Python flush callback
-   * defined, so return 1.  (In C modules, nbdkit would do this).
-   */
-  else if (callback_defined ("flush", NULL))
-    return 1;
-  else
-    return 0;
+  /* No Python can_flush callback, so check for Python flush callback. */
+  return callback_defined ("flush", NULL);
 }

 static int
@@ -708,13 +698,31 @@ py_can_trim (void *handle)
     Py_DECREF (r);
     return ret;
   }
-  /* No Python can_trim callback, but there's a Python trim callback
-   * defined, so return 1.  (In C modules, nbdkit would do this).
-   */
-  else if (callback_defined ("trim", NULL))
-    return 1;
-  else
-    return 0;
+  /* No Python can_trim callback, so check for Python trim callback. */
+  return callback_defined ("trim", NULL);
+}
+
+static int
+py_can_zero (void *handle)
+{
+  PyObject *obj = handle;
+  PyObject *fn;
+  PyObject *r;
+  int ret;
+
+  if (callback_defined ("can_zero", &fn)) {
+    PyErr_Clear ();
+
+    r = PyObject_CallFunctionObjArgs (fn, obj, NULL);
+    Py_DECREF (fn);
+    if (check_python_failure ("can_zero") == -1)
+      return -1;
+    ret = r == Py_True;
+    Py_DECREF (r);
+    return ret;
+  }
+  /* No Python can_zero callback, so check for Python zero callback. */
+  return callback_defined ("zero", NULL);
 }

 #define py_config_help \
@@ -743,6 +751,7 @@ static struct nbdkit_plugin plugin = {
   .can_flush         = py_can_flush,
   .is_rotational     = py_is_rotational,
   .can_trim          = py_can_trim,
+  .can_zero          = py_can_zero,

   .pread             = py_pread,
   .pwrite            = py_pwrite,
diff --git a/tests/test.py b/tests/test.py
index 518cdd4..852af55 100644
--- a/tests/test.py
+++ b/tests/test.py
@@ -30,6 +30,10 @@ def can_trim(h):
     return True


+def can_zero(h):
+    return True
+
+
 def pread(h, count, offset):
     global disk
     return disk[offset:offset+count]
-- 
2.14.3

Adjust internal functions in preparation for FUA support; although
at this point, the default plugins.c can_fua implementation
correctly reports python as needing emulation, and we can assert
that we aren't seeing a FUA flag.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 plugins/python/python.c | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/plugins/python/python.c b/plugins/python/python.c
index d75b36a..a50bf85 100644
--- a/plugins/python/python.c
+++ b/plugins/python/python.c
@@ -49,6 +49,7 @@
 #include <assert.h>
 #include <errno.h>

+#define NBDKIT_API_VERSION 2
 #include <nbdkit-plugin.h>

 /* XXX Apparently global state is technically wrong in Python 3, see:
@@ -430,12 +431,13 @@ py_get_size (void *handle)

 static int
 py_pread (void *handle, void *buf,
-          uint32_t count, uint64_t offset)
+          uint32_t count, uint64_t offset, uint32_t flags)
 {
   PyObject *obj = handle;
   PyObject *fn;
   PyObject *r;

+  assert (!flags);
   if (!callback_defined ("pread", &fn)) {
     nbdkit_error ("%s: missing callback: %s", script,
"pread");
     return -1;
@@ -469,12 +471,13 @@ py_pread (void *handle, void *buf,

 static int
 py_pwrite (void *handle, const void *buf,
-           uint32_t count, uint64_t offset)
+           uint32_t count, uint64_t offset, uint32_t flags)
 {
   PyObject *obj = handle;
   PyObject *fn;
   PyObject *r;

+  assert (!flags);
   if (callback_defined ("pwrite", &fn)) {
     PyErr_Clear ();

@@ -495,12 +498,13 @@ py_pwrite (void *handle, const void *buf,
 }

 static int
-py_flush (void *handle)
+py_flush (void *handle, uint32_t flags)
 {
   PyObject *obj = handle;
   PyObject *fn;
   PyObject *r;

+  assert (!flags);
   if (callback_defined ("flush", &fn)) {
     PyErr_Clear ();

@@ -519,12 +523,13 @@ py_flush (void *handle)
 }

 static int
-py_trim (void *handle, uint32_t count, uint64_t offset)
+py_trim (void *handle, uint32_t count, uint64_t offset, uint32_t flags)
 {
   PyObject *obj = handle;
   PyObject *fn;
   PyObject *r;

+  assert (!flags);
   if (callback_defined ("trim", &fn)) {
     PyErr_Clear ();

@@ -543,14 +548,16 @@ py_trim (void *handle, uint32_t count, uint64_t offset)
 }

 static int
-py_zero (void *handle, uint32_t count, uint64_t offset, int may_trim)
+py_zero (void *handle, uint32_t count, uint64_t offset, uint32_t flags)
 {
   PyObject *obj = handle;
   PyObject *fn;
   PyObject *args;
   PyObject *kwargs;
   PyObject *r;
+  int may_trim = (flags & NBDKIT_FLAG_MAY_TRIM) != 0;

+  assert (!(flags & ~NBDKIT_FLAG_MAY_TRIM));
   if (callback_defined ("zero", &fn)) {
     static int zero_may_trim = -1;

-- 
2.14.3

Expose support for the FUA flag to pwrite, zero, and trim, as
well as a can_fua callback, for use in python plugins.  There
are some slight semantic differences: the C plugin had to
switch to a new API with a single uint32_t flags argument (so
we don't have to keep adding new ABI when new flags are added),
but in so doing, there is no way to probe whether a C plugin
supports FUA flags, so the C .can_fua has to return a tri-state.
But for python plugins, thanks to the fact that python functions
have named and optional parameters, it's a lot cleaner to
provide new flags as a bool option apiece, instead of requiring
the python program to parse out bits from an int, and easy to
introspect if FUA support is present, so can_fua only has to
return a bool on whether to advertise FUA support to the
client.

One other thing that makes for some interesting C glue code:
if at least one of the three callbacks supports FUA, then the
C can_fua function claims native support, but in so doing,
the python plugin must gracefully handle the fallback to flush
for the remaining callbacks that did not support FUA, since
nbdkit plugins.c won't do it.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 plugins/python/nbdkit-python-plugin.pod |  45 +++++++++-
 plugins/python/python.c                 | 153 +++++++++++++++++++++++++++++---
 plugins/python/example.py               |   6 +-
 3 files changed, 189 insertions(+), 15 deletions(-)

diff --git a/plugins/python/nbdkit-python-plugin.pod
b/plugins/python/nbdkit-python-plugin.pod
index 29250ce..a480a25 100644
--- a/plugins/python/nbdkit-python-plugin.pod
+++ b/plugins/python/nbdkit-python-plugin.pod
@@ -190,6 +190,22 @@ contents will be garbage collected.
  def can_zero(h):
    # return a boolean

+=item C<can_fua>
+
+(Optional)
+
+ def can_fua(h):
+   # return a boolean
+
+Unlike the C counterpart, the Python callback does not need a
+tri-state return value, because Python introspection is sufficient to
+learn whether callbacks support FUA.  Thus, this function only returns
+a bool, which merely controls whether Forced Unit Access (FUA) support
+is advertised to the client on a per-connection basis.  If omitted or
+this returns true, FUA support is advertised as native if any of the
+C<pwrite>, C<zero>, or C<trim> callbacks support an optional
parameter
+C<fua>; or as emulated if there is a C<flush> callback.
+
 =item C<pread>

 (Required)
@@ -210,7 +226,7 @@ C<nbdkit.set_error> first.

 (Optional)

- def pwrite(h, buf, offset):
+ def pwrite(h, buf, offset, fua=False):
    length = len (buf)
    # no return value

@@ -218,6 +234,12 @@ The body of your C<pwrite> function should write the
C<buf> string to
 the disk.  You should write C<count> bytes to the disk starting at
 C<offset>.

+Your function may support an optional C<fua> parameter; if it is
+present and the caller sets it to True, then your callback must not
+return success until the write has landed in persistent storage.  If
+it is absent but C<can_fua> returned True, then nbdkit emulates a
+client request for FUA by calling C<flush>.
+
 NBD only supports whole writes, so your function should try to
 write the whole region (perhaps requiring a loop).  If the write
 fails or is partial, your function should throw an exception,
@@ -233,6 +255,11 @@ fails or is partial, your function should throw an
exception,
 The body of your C<flush> function should do a L<sync(2)> or
 L<fdatasync(2)> or equivalent on the backing store.

+This function will not be called directly by the client if
+C<can_flush> returned false; however, it may still be called by nbdkit
+if C<can_fua> returned True but C<pwrite>, C<zero>, or
C<trim> lacked
+a C<fua> parameter.
+
 If the flush fails, your function should throw an exception, optionally
 using C<nbdkit.set_error> first.

@@ -240,18 +267,24 @@ using C<nbdkit.set_error> first.

 (Optional)

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

 The body of your C<trim> function should "punch a hole" in the
 backing store.  If the trim fails, your function should throw an
 exception, optionally using C<nbdkit.set_error> first.

+Your function may support an optional C<fua> parameter; if it is
+present and the caller sets it to True, then your callback must not
+return success until the trim has landed in persistent storage.  If it
+is absent but C<can_fua> returned True, then nbdkit emulates a client
+request for FUA by calling C<flush>.
+
 =item C<zero>

 (Optional)

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

 The body of your C<zero> function should ensure that C<count> bytes
@@ -263,6 +296,12 @@ 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.

+Your function may support an optional C<fua> parameter; if it is
+present and the caller sets it to True, then your callback must not
+return success until the write has landed in persistent storage.  If
+it is absent but C<can_fua> returned True, then nbdkit emulates a
+client request for FUA by calling C<flush>.
+
 NBD only supports whole writes, so your function should try to
 write the whole region (perhaps requiring a loop).  If the write
 fails or is partial, your function should throw an exception,
diff --git a/plugins/python/python.c b/plugins/python/python.c
index a50bf85..ad79c80 100644
--- a/plugins/python/python.c
+++ b/plugins/python/python.c
@@ -64,6 +64,9 @@ static const char *script;
 static PyObject *module;

 static int last_error;
+static int pwrite_has_fua;
+static int zero_has_fua;
+static int trim_has_fua;

 static PyObject *
 set_error (PyObject *self, PyObject *args)
@@ -320,6 +323,32 @@ py_config (const char *key, const char *value)
       nbdkit_error ("%s: one of the required callbacks 'open',
'get_size' or 'pread' is not defined by this Python script. 
nbdkit requires these callbacks.", script);
       return -1;
     }
+
+    /* One-time setup to learn which callbacks have a fua parameter */
+    if (callback_defined ("pwrite", &fn)) {
+      pwrite_has_fua = callback_has_parameter (fn, "fua");
+      Py_DECREF (fn);
+      if (pwrite_has_fua < 0) {
+        check_python_failure ("config");
+        return -1;
+      }
+    }
+    if (callback_defined ("zero", &fn)) {
+      zero_has_fua = callback_has_parameter (fn, "fua");
+      Py_DECREF (fn);
+      if (zero_has_fua < 0) {
+        check_python_failure ("config");
+        return -1;
+      }
+    }
+    if (callback_defined ("trim", &fn)) {
+      trim_has_fua = callback_has_parameter (fn, "fua");
+      Py_DECREF (fn);
+      if (trim_has_fua < 0) {
+        check_python_failure ("config");
+        return -1;
+      }
+    }
   }
   else if (callback_defined ("config", &fn)) {
     /* Other parameters are passed to the Python .config callback. */
@@ -469,22 +498,52 @@ py_pread (void *handle, void *buf,
   return 0;
 }

+static int py_flush (void *handle, uint32_t flags);
+
 static int
 py_pwrite (void *handle, const void *buf,
            uint32_t count, uint64_t offset, uint32_t flags)
 {
   PyObject *obj = handle;
   PyObject *fn;
+  PyObject *args;
+  PyObject *kwargs;
   PyObject *r;
+  int fua = (flags & NBDKIT_FLAG_FUA) != 0;
+  int need_flush = fua && !pwrite_has_fua;

-  assert (!flags);
+  assert (!(flags & ~NBDKIT_FLAG_FUA));
   if (callback_defined ("pwrite", &fn)) {
     PyErr_Clear ();

-    r = PyObject_CallFunction (fn, "ONL", obj,
-                               PyByteArray_FromStringAndSize (buf, count),
-                               offset, NULL);
+    args = Py_BuildValue ("ONL", obj,
+                          PyByteArray_FromStringAndSize (buf, count),
+                          offset);
+    if (!args) {
+      check_python_failure ("pwrite");
+      Py_DECREF (fn);
+      return -1;
+    }
+    kwargs = PyDict_New ();
+    if (!kwargs) {
+      check_python_failure ("pwrite");
+      Py_DECREF (args);
+      Py_DECREF (fn);
+      return -1;
+    }
+    if (pwrite_has_fua &&
+        PyDict_SetItemString (kwargs, "fua",
+                              fua ? Py_True : Py_False) == -1) {
+      check_python_failure ("pwrite");
+      Py_DECREF (kwargs);
+      Py_DECREF (args);
+      Py_DECREF (fn);
+      return -1;
+    }
+    r = PyObject_Call (fn, args, kwargs);
     Py_DECREF (fn);
+    Py_DECREF (args);
+    Py_DECREF (kwargs);
     if (check_python_failure ("pwrite") == -1)
       return -1;
     Py_DECREF (r);
@@ -494,7 +553,7 @@ py_pwrite (void *handle, const void *buf,
     return -1;
   }

-  return 0;
+  return need_flush ? py_flush (handle, 0) : 0;
 }

 static int
@@ -527,14 +586,42 @@ py_trim (void *handle, uint32_t count, uint64_t offset,
uint32_t flags)
 {
   PyObject *obj = handle;
   PyObject *fn;
+  PyObject *args;
+  PyObject *kwargs;
   PyObject *r;
+  int fua = (flags & NBDKIT_FLAG_FUA) != 0;
+  int need_flush = fua && !trim_has_fua;

-  assert (!flags);
+  assert (!(flags & ~NBDKIT_FLAG_FUA));
   if (callback_defined ("trim", &fn)) {
     PyErr_Clear ();

-    r = PyObject_CallFunction (fn, "OiL", obj, count, offset, NULL);
+    args = Py_BuildValue ("OiL", obj, count, offset);
+    if (!args) {
+      check_python_failure ("trim");
+      Py_DECREF (fn);
+      return -1;
+    }
+    kwargs = PyDict_New ();
+    if (!kwargs) {
+      check_python_failure ("trim");
+      Py_DECREF (args);
+      Py_DECREF (fn);
+      return -1;
+    }
+    if (trim_has_fua &&
+        PyDict_SetItemString (kwargs, "fua",
+                              fua ? Py_True : Py_False) == -1) {
+      check_python_failure ("trim");
+      Py_DECREF (kwargs);
+      Py_DECREF (args);
+      Py_DECREF (fn);
+      return -1;
+    }
+    r = PyObject_Call (fn, args, kwargs);
     Py_DECREF (fn);
+    Py_DECREF (args);
+    Py_DECREF (kwargs);
     if (check_python_failure ("trim") == -1)
       return -1;
     Py_DECREF (r);
@@ -544,7 +631,7 @@ py_trim (void *handle, uint32_t count, uint64_t offset,
uint32_t flags)
     return -1;
   }

-  return 0;
+  return need_flush ? py_flush (handle, 0) : 0;
 }

 static int
@@ -556,8 +643,10 @@ py_zero (void *handle, uint32_t count, uint64_t offset,
uint32_t flags)
   PyObject *kwargs;
   PyObject *r;
   int may_trim = (flags & NBDKIT_FLAG_MAY_TRIM) != 0;
+  int fua = (flags & NBDKIT_FLAG_FUA) != 0;
+  int need_flush = fua && !zero_has_fua;

-  assert (!(flags & ~NBDKIT_FLAG_MAY_TRIM));
+  assert (!(flags & ~(NBDKIT_FLAG_MAY_TRIM | NBDKIT_FLAG_FUA)));
   if (callback_defined ("zero", &fn)) {
     static int zero_may_trim = -1;

@@ -593,6 +682,15 @@ py_zero (void *handle, uint32_t count, uint64_t offset,
uint32_t flags)
       Py_DECREF (fn);
       return -1;
     }
+    if (zero_has_fua &&
+        PyDict_SetItemString (kwargs, "fua",
+                              fua ? Py_True : Py_False) == -1) {
+      check_python_failure ("zero");
+      Py_DECREF (kwargs);
+      Py_DECREF (args);
+      Py_DECREF (fn);
+      return -1;
+    }
     r = PyObject_Call (fn, args, kwargs);
     Py_DECREF (fn);
     Py_DECREF (args);
@@ -609,7 +707,7 @@ py_zero (void *handle, uint32_t count, uint64_t offset,
uint32_t flags)
     if (check_python_failure ("zero") == -1)
       return -1;
     Py_DECREF (r);
-    return 0;
+    return need_flush ? py_flush (handle, 0) : 0;
   }

   nbdkit_debug ("zero missing, falling back to pwrite");
@@ -732,6 +830,40 @@ py_can_zero (void *handle)
   return callback_defined ("zero", NULL);
 }

+static int
+py_can_fua (void *handle)
+{
+  PyObject *obj = handle;
+  PyObject *fn;
+  PyObject *r;
+  int fua;
+
+  /* We have to convert a python bool into the nbdkit tristate. */
+  if (callback_defined ("can_fua", &fn)) {
+    PyErr_Clear ();
+
+    r = PyObject_CallFunctionObjArgs (fn, obj, NULL);
+    Py_DECREF (fn);
+    if (check_python_failure ("can_fua") == -1)
+      return -1;
+    if (r == Py_False)
+      fua = NBDKIT_FUA_NONE;
+    else if (pwrite_has_fua || zero_has_fua || trim_has_fua)
+      fua = NBDKIT_FUA_NATIVE;
+    else
+      fua = NBDKIT_FUA_EMULATE;
+    Py_DECREF (r);
+  }
+  else if (pwrite_has_fua || zero_has_fua || trim_has_fua)
+    fua = NBDKIT_FUA_NATIVE;
+  else if (callback_defined ("flush", NULL))
+    fua = NBDKIT_FUA_EMULATE;
+  else
+    fua = NBDKIT_FUA_NONE;
+
+  return fua;
+}
+
 #define py_config_help \
   "script=<FILENAME>     (required) The Python plugin to
run.\n" \
   "[other arguments may be used by the plugin that you load]"
@@ -759,6 +891,7 @@ static struct nbdkit_plugin plugin = {
   .is_rotational     = py_is_rotational,
   .can_trim          = py_can_trim,
   .can_zero          = py_can_zero,
+  .can_fua           = py_can_fua,

   .pread             = py_pread,
   .pwrite            = py_pwrite,
diff --git a/plugins/python/example.py b/plugins/python/example.py
index 9205f10..ca7c266 100644
--- a/plugins/python/example.py
+++ b/plugins/python/example.py
@@ -59,13 +59,15 @@ def pread(h, count, offset):
     return disk[offset:offset+count]


-def pwrite(h, buf, offset):
+# Since everything is stored in memory, flush is a no-op; this example
+# does not provide a flush(), but honors the fua flag by doing nothing.
+def pwrite(h, buf, offset, fua=False):
     global disk
     end = offset + len(buf)
     disk[offset:end] = buf


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

Now that we have FUA support, the C code can call can_fua as
frequently as on every write.  If the python script has a
can_fua, we can avoid doubling the calls into python by
caching the per-connection results, done by wrapping the
python handle in a C struct.

This commit is marked RFC because it might be nicer if the C
code implemented the caching for ALL plugins (TODO already
mentions that).

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 plugins/python/python.c | 83 ++++++++++++++++++++++++++++++-------------------
 1 file changed, 51 insertions(+), 32 deletions(-)

diff --git a/plugins/python/python.c b/plugins/python/python.c
index ad79c80..757a0e9 100644
--- a/plugins/python/python.c
+++ b/plugins/python/python.c
@@ -389,51 +389,66 @@ py_config_complete (void)
   return 0;
 }

+/* All per-connection state */
+typedef struct ConnHandle {
+  PyObject *obj;
+  int fua;
+} ConnHandle;
+
 static void *
 py_open (int readonly)
 {
   PyObject *fn;
-  PyObject *handle;
+  ConnHandle *h = malloc (sizeof *h);

+  if (!h) {
+    nbdkit_error ("%s: %m", script);
+    return NULL;
+  }
   if (!callback_defined ("open", &fn)) {
     nbdkit_error ("%s: missing callback: %s", script,
"open");
+    free (h);
     return NULL;
   }

   PyErr_Clear ();

-  handle = PyObject_CallFunctionObjArgs (fn, readonly ? Py_True : Py_False,
+  h->obj = PyObject_CallFunctionObjArgs (fn, readonly ? Py_True : Py_False,
                                          NULL);
   Py_DECREF (fn);
-  if (check_python_failure ("open") == -1)
+  if (check_python_failure ("open") == -1) {
+    free (h);
     return NULL;
+  }

-  return handle;
+  h->fua = -1;
+  return h;
 }

 static void
 py_close (void *handle)
 {
-  PyObject *obj = handle;
+  ConnHandle *h = handle;
   PyObject *fn;
   PyObject *r;

   if (callback_defined ("close", &fn)) {
     PyErr_Clear ();

-    r = PyObject_CallFunctionObjArgs (fn, obj, NULL);
+    r = PyObject_CallFunctionObjArgs (fn, h->obj, NULL);
     Py_DECREF (fn);
     check_python_failure ("close");
     Py_XDECREF (r);
   }

-  Py_DECREF (obj);
+  Py_DECREF (h->obj);
+  free (h);
 }

 static int64_t
 py_get_size (void *handle)
 {
-  PyObject *obj = handle;
+  ConnHandle *h = handle;
   PyObject *fn;
   PyObject *r;
   int64_t ret;
@@ -445,7 +460,7 @@ py_get_size (void *handle)

   PyErr_Clear ();

-  r = PyObject_CallFunctionObjArgs (fn, obj, NULL);
+  r = PyObject_CallFunctionObjArgs (fn, h->obj, NULL);
   Py_DECREF (fn);
   if (check_python_failure ("get_size") == -1)
     return -1;
@@ -462,7 +477,7 @@ static int
 py_pread (void *handle, void *buf,
           uint32_t count, uint64_t offset, uint32_t flags)
 {
-  PyObject *obj = handle;
+  ConnHandle *h = handle;
   PyObject *fn;
   PyObject *r;

@@ -474,7 +489,7 @@ py_pread (void *handle, void *buf,

   PyErr_Clear ();

-  r = PyObject_CallFunction (fn, "OiL", obj, count, offset, NULL);
+  r = PyObject_CallFunction (fn, "OiL", h->obj, count, offset,
NULL);
   Py_DECREF (fn);
   if (check_python_failure ("pread") == -1)
     return -1;
@@ -504,7 +519,7 @@ static int
 py_pwrite (void *handle, const void *buf,
            uint32_t count, uint64_t offset, uint32_t flags)
 {
-  PyObject *obj = handle;
+  ConnHandle *h = handle;
   PyObject *fn;
   PyObject *args;
   PyObject *kwargs;
@@ -516,7 +531,7 @@ py_pwrite (void *handle, const void *buf,
   if (callback_defined ("pwrite", &fn)) {
     PyErr_Clear ();

-    args = Py_BuildValue ("ONL", obj,
+    args = Py_BuildValue ("ONL", h->obj,
                           PyByteArray_FromStringAndSize (buf, count),
                           offset);
     if (!args) {
@@ -559,7 +574,7 @@ py_pwrite (void *handle, const void *buf,
 static int
 py_flush (void *handle, uint32_t flags)
 {
-  PyObject *obj = handle;
+  ConnHandle *h = handle;
   PyObject *fn;
   PyObject *r;

@@ -567,7 +582,7 @@ py_flush (void *handle, uint32_t flags)
   if (callback_defined ("flush", &fn)) {
     PyErr_Clear ();

-    r = PyObject_CallFunctionObjArgs (fn, obj, NULL);
+    r = PyObject_CallFunctionObjArgs (fn, h->obj, NULL);
     Py_DECREF (fn);
     if (check_python_failure ("flush") == -1)
       return -1;
@@ -584,7 +599,7 @@ py_flush (void *handle, uint32_t flags)
 static int
 py_trim (void *handle, uint32_t count, uint64_t offset, uint32_t flags)
 {
-  PyObject *obj = handle;
+  ConnHandle *h = handle;
   PyObject *fn;
   PyObject *args;
   PyObject *kwargs;
@@ -596,7 +611,7 @@ py_trim (void *handle, uint32_t count, uint64_t offset,
uint32_t flags)
   if (callback_defined ("trim", &fn)) {
     PyErr_Clear ();

-    args = Py_BuildValue ("OiL", obj, count, offset);
+    args = Py_BuildValue ("OiL", h->obj, count, offset);
     if (!args) {
       check_python_failure ("trim");
       Py_DECREF (fn);
@@ -637,7 +652,7 @@ py_trim (void *handle, uint32_t count, uint64_t offset,
uint32_t flags)
 static int
 py_zero (void *handle, uint32_t count, uint64_t offset, uint32_t flags)
 {
-  PyObject *obj = handle;
+  ConnHandle *h = handle;
   PyObject *fn;
   PyObject *args;
   PyObject *kwargs;
@@ -660,7 +675,7 @@ py_zero (void *handle, uint32_t count, uint64_t offset,
uint32_t flags)
     PyErr_Clear ();

     last_error = 0;
-    args = Py_BuildValue ("OiL", obj, count, offset);
+    args = Py_BuildValue ("OiL", h->obj, count, offset);
     if (!args) {
       check_python_failure ("zero");
       Py_DECREF (fn);
@@ -718,7 +733,7 @@ py_zero (void *handle, uint32_t count, uint64_t offset,
uint32_t flags)
 static int
 py_can_write (void *handle)
 {
-  PyObject *obj = handle;
+  ConnHandle *h = handle;
   PyObject *fn;
   PyObject *r;
   int ret;
@@ -726,7 +741,7 @@ py_can_write (void *handle)
   if (callback_defined ("can_write", &fn)) {
     PyErr_Clear ();

-    r = PyObject_CallFunctionObjArgs (fn, obj, NULL);
+    r = PyObject_CallFunctionObjArgs (fn, h->obj, NULL);
     Py_DECREF (fn);
     if (check_python_failure ("can_write") == -1)
       return -1;
@@ -741,7 +756,7 @@ py_can_write (void *handle)
 static int
 py_can_flush (void *handle)
 {
-  PyObject *obj = handle;
+  ConnHandle *h = handle;
   PyObject *fn;
   PyObject *r;
   int ret;
@@ -749,7 +764,7 @@ py_can_flush (void *handle)
   if (callback_defined ("can_flush", &fn)) {
     PyErr_Clear ();

-    r = PyObject_CallFunctionObjArgs (fn, obj, NULL);
+    r = PyObject_CallFunctionObjArgs (fn, h->obj, NULL);
     Py_DECREF (fn);
     if (check_python_failure ("can_flush") == -1)
       return -1;
@@ -764,7 +779,7 @@ py_can_flush (void *handle)
 static int
 py_is_rotational (void *handle)
 {
-  PyObject *obj = handle;
+  ConnHandle *h = handle;
   PyObject *fn;
   PyObject *r;
   int ret;
@@ -772,7 +787,7 @@ py_is_rotational (void *handle)
   if (callback_defined ("is_rotational", &fn)) {
     PyErr_Clear ();

-    r = PyObject_CallFunctionObjArgs (fn, obj, NULL);
+    r = PyObject_CallFunctionObjArgs (fn, h->obj, NULL);
     Py_DECREF (fn);
     if (check_python_failure ("is_rotational") == -1)
       return -1;
@@ -787,7 +802,7 @@ py_is_rotational (void *handle)
 static int
 py_can_trim (void *handle)
 {
-  PyObject *obj = handle;
+  ConnHandle *h = handle;
   PyObject *fn;
   PyObject *r;
   int ret;
@@ -795,7 +810,7 @@ py_can_trim (void *handle)
   if (callback_defined ("can_trim", &fn)) {
     PyErr_Clear ();

-    r = PyObject_CallFunctionObjArgs (fn, obj, NULL);
+    r = PyObject_CallFunctionObjArgs (fn, h->obj, NULL);
     Py_DECREF (fn);
     if (check_python_failure ("can_trim") == -1)
       return -1;
@@ -810,7 +825,7 @@ py_can_trim (void *handle)
 static int
 py_can_zero (void *handle)
 {
-  PyObject *obj = handle;
+  ConnHandle *h = handle;
   PyObject *fn;
   PyObject *r;
   int ret;
@@ -818,7 +833,7 @@ py_can_zero (void *handle)
   if (callback_defined ("can_zero", &fn)) {
     PyErr_Clear ();

-    r = PyObject_CallFunctionObjArgs (fn, obj, NULL);
+    r = PyObject_CallFunctionObjArgs (fn, h->obj, NULL);
     Py_DECREF (fn);
     if (check_python_failure ("can_zero") == -1)
       return -1;
@@ -833,16 +848,20 @@ py_can_zero (void *handle)
 static int
 py_can_fua (void *handle)
 {
-  PyObject *obj = handle;
+  ConnHandle *h = handle;
   PyObject *fn;
   PyObject *r;
   int fua;

+  /* Check the cache first */
+  if (h->fua != -1)
+    return h->fua;
+
   /* We have to convert a python bool into the nbdkit tristate. */
   if (callback_defined ("can_fua", &fn)) {
     PyErr_Clear ();

-    r = PyObject_CallFunctionObjArgs (fn, obj, NULL);
+    r = PyObject_CallFunctionObjArgs (fn, h->obj, NULL);
     Py_DECREF (fn);
     if (check_python_failure ("can_fua") == -1)
       return -1;
@@ -861,7 +880,7 @@ py_can_fua (void *handle)
   else
     fua = NBDKIT_FUA_NONE;

-  return fua;
+  return h->fua = fua;
 }

 #define py_config_help \
-- 
2.14.3

On Wed, Apr 11, 2018 at 12:03:38AM -0500, 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, although that's not the direction we typically
> guarantee).  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, where we do not recognize the
> fourth parameter name, is an old-style plugin, and call it
> with may_trim passed via a 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; furthermore, while we have been clear
> that backwards API compatibility is essential for C, we have
> not made the same guarantee for language plugins.
> 
> The introspection framework here will also make it easy to
> probe for future flag additions (support for 'fua' being the
> obvious candidate in the short term).
> 
> This patch also fixes failure to check for (unlikely) errors
> during creation of 'args' within py_zero().
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>  plugins/python/nbdkit-python-plugin.pod |  35 ++++++++---
>  plugins/python/python.c                 | 108
+++++++++++++++++++++++++++++---
>  plugins/python/example.py               |   2 +-
>  tests/test.py                           |   2 +-
>  4 files changed, 129 insertions(+), 18 deletions(-)
> 
This patch looked fine to me so ACK.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-builder quickly builds VMs from scratch
http://libguestfs.org/virt-builder.1.html

On Wed, Apr 11, 2018 at 12:03:39AM -0500, Eric Blake
wrote:
> Fairly straightforward, as the code in plugins.c already takes
> care of necessary .pwrite fallbacks.
> 
> Simplify similar can_FOO callbacks that were copy-and-pasted while
> implementing py_can_zero.
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>  plugins/python/nbdkit-python-plugin.pod |  7 +++++
>  plugins/python/python.c                 | 51
+++++++++++++++++++--------------
>  tests/test.py                           |  4 +++
>  3 files changed, 41 insertions(+), 21 deletions(-)
Straightforward, so ACK.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
Fedora Windows cross-compiler. Compile Windows programs, test, and
build Windows installers. Over 100 libraries supported.
http://fedoraproject.org/wiki/MinGW

On Wed, Apr 11, 2018 at 12:03:40AM -0500, Eric Blake
wrote:
> Adjust internal functions in preparation for FUA support; although
> at this point, the default plugins.c can_fua implementation
> correctly reports python as needing emulation, and we can assert
> that we aren't seeing a FUA flag.
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>  plugins/python/python.c | 17 ++++++++++++-----
>  1 file changed, 12 insertions(+), 5 deletions(-)
> 
> diff --git a/plugins/python/python.c b/plugins/python/python.c
> index d75b36a..a50bf85 100644
> --- a/plugins/python/python.c
> +++ b/plugins/python/python.c
> @@ -49,6 +49,7 @@
>  #include <assert.h>
>  #include <errno.h>
> 
> +#define NBDKIT_API_VERSION 2
>  #include <nbdkit-plugin.h>
> 
>  /* XXX Apparently global state is technically wrong in Python 3, see:
> @@ -430,12 +431,13 @@ py_get_size (void *handle)
> 
>  static int
>  py_pread (void *handle, void *buf,
> -          uint32_t count, uint64_t offset)
> +          uint32_t count, uint64_t offset, uint32_t flags)
>  {
>    PyObject *obj = handle;
>    PyObject *fn;
>    PyObject *r;
> 
> +  assert (!flags);
I'm confused by the assertions here (I understand that these are
relaxed in the next patch, but still present).

Shouldn't this return an error instead of segfaulting the daemon?

Do we assume that the nbdkit python plugin is special because you're
basically always running the version that ships with the daemon?

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
libguestfs lets you edit virtual machines.  Supports shell scripting,
bindings from many languages.  http://libguestfs.org

On Wed, Apr 11, 2018 at 12:03:41AM -0500, Eric Blake
wrote:
> +=item C<can_fua>
> +
> +(Optional)
> +
> + def can_fua(h):
> +   # return a boolean
> +
> +Unlike the C counterpart, the Python callback does not need a
> +tri-state return value, because Python introspection is sufficient to
> +learn whether callbacks support FUA.  Thus, this function only returns
> +a bool, which merely controls whether Forced Unit Access (FUA) support
> +is advertised to the client on a per-connection basis.  If omitted or
> +this returns true, FUA support is advertised as native if any of the
> +C<pwrite>, C<zero>, or C<trim> callbacks support an
optional parameter
> +C<fua>; or as emulated if there is a C<flush> callback.
This is a true description of what the code does ... but it's pretty
complicated.

First of all, if it returns true why don't we just "believe" the
plugin?  The callbacks don't have fua optargs, but so what?

And would it be easier both for us and for plugin implementors
if we stuck to the C API here instead of being clever?

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
libguestfs lets you edit virtual machines.  Supports shell scripting,
bindings from many languages.  http://libguestfs.org

On Wed, Apr 11, 2018 at 12:03:42AM -0500, Eric Blake
wrote:
> Now that we have FUA support, the C code can call can_fua as
> frequently as on every write.  If the python script has a
> can_fua, we can avoid doubling the calls into python by
> caching the per-connection results, done by wrapping the
> python handle in a C struct.
> 
> This commit is marked RFC because it might be nicer if the C
> code implemented the caching for ALL plugins (TODO already
> mentions that).
It's not very clear to me (and this applies to all can_* functions) if
it's OK to call them only once per connection.

I can't think of any argument against that.  Is there a potential
meaning for a plugin to "change its mind" during a connection?

Rich.
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>  plugins/python/python.c | 83
++++++++++++++++++++++++++++++-------------------
>  1 file changed, 51 insertions(+), 32 deletions(-)
> 
> diff --git a/plugins/python/python.c b/plugins/python/python.c
> index ad79c80..757a0e9 100644
> --- a/plugins/python/python.c
> +++ b/plugins/python/python.c
> @@ -389,51 +389,66 @@ py_config_complete (void)
>    return 0;
>  }
> 
> +/* All per-connection state */
> +typedef struct ConnHandle {
> +  PyObject *obj;
> +  int fua;
> +} ConnHandle;
> +
>  static void *
>  py_open (int readonly)
>  {
>    PyObject *fn;
> -  PyObject *handle;
> +  ConnHandle *h = malloc (sizeof *h);
> 
> +  if (!h) {
> +    nbdkit_error ("%s: %m", script);
> +    return NULL;
> +  }
>    if (!callback_defined ("open", &fn)) {
>      nbdkit_error ("%s: missing callback: %s", script,
"open");
> +    free (h);
>      return NULL;
>    }
> 
>    PyErr_Clear ();
> 
> -  handle = PyObject_CallFunctionObjArgs (fn, readonly ? Py_True :
Py_False,
> +  h->obj = PyObject_CallFunctionObjArgs (fn, readonly ? Py_True :
Py_False,
>                                           NULL);
>    Py_DECREF (fn);
> -  if (check_python_failure ("open") == -1)
> +  if (check_python_failure ("open") == -1) {
> +    free (h);
>      return NULL;
> +  }
> 
> -  return handle;
> +  h->fua = -1;
> +  return h;
>  }
> 
>  static void
>  py_close (void *handle)
>  {
> -  PyObject *obj = handle;
> +  ConnHandle *h = handle;
>    PyObject *fn;
>    PyObject *r;
> 
>    if (callback_defined ("close", &fn)) {
>      PyErr_Clear ();
> 
> -    r = PyObject_CallFunctionObjArgs (fn, obj, NULL);
> +    r = PyObject_CallFunctionObjArgs (fn, h->obj, NULL);
>      Py_DECREF (fn);
>      check_python_failure ("close");
>      Py_XDECREF (r);
>    }
> 
> -  Py_DECREF (obj);
> +  Py_DECREF (h->obj);
> +  free (h);
>  }
> 
>  static int64_t
>  py_get_size (void *handle)
>  {
> -  PyObject *obj = handle;
> +  ConnHandle *h = handle;
>    PyObject *fn;
>    PyObject *r;
>    int64_t ret;
> @@ -445,7 +460,7 @@ py_get_size (void *handle)
> 
>    PyErr_Clear ();
> 
> -  r = PyObject_CallFunctionObjArgs (fn, obj, NULL);
> +  r = PyObject_CallFunctionObjArgs (fn, h->obj, NULL);
>    Py_DECREF (fn);
>    if (check_python_failure ("get_size") == -1)
>      return -1;
> @@ -462,7 +477,7 @@ static int
>  py_pread (void *handle, void *buf,
>            uint32_t count, uint64_t offset, uint32_t flags)
>  {
> -  PyObject *obj = handle;
> +  ConnHandle *h = handle;
>    PyObject *fn;
>    PyObject *r;
> 
> @@ -474,7 +489,7 @@ py_pread (void *handle, void *buf,
> 
>    PyErr_Clear ();
> 
> -  r = PyObject_CallFunction (fn, "OiL", obj, count, offset,
NULL);
> +  r = PyObject_CallFunction (fn, "OiL", h->obj, count,
offset, NULL);
>    Py_DECREF (fn);
>    if (check_python_failure ("pread") == -1)
>      return -1;
> @@ -504,7 +519,7 @@ static int
>  py_pwrite (void *handle, const void *buf,
>             uint32_t count, uint64_t offset, uint32_t flags)
>  {
> -  PyObject *obj = handle;
> +  ConnHandle *h = handle;
>    PyObject *fn;
>    PyObject *args;
>    PyObject *kwargs;
> @@ -516,7 +531,7 @@ py_pwrite (void *handle, const void *buf,
>    if (callback_defined ("pwrite", &fn)) {
>      PyErr_Clear ();
> 
> -    args = Py_BuildValue ("ONL", obj,
> +    args = Py_BuildValue ("ONL", h->obj,
>                            PyByteArray_FromStringAndSize (buf, count),
>                            offset);
>      if (!args) {
> @@ -559,7 +574,7 @@ py_pwrite (void *handle, const void *buf,
>  static int
>  py_flush (void *handle, uint32_t flags)
>  {
> -  PyObject *obj = handle;
> +  ConnHandle *h = handle;
>    PyObject *fn;
>    PyObject *r;
> 
> @@ -567,7 +582,7 @@ py_flush (void *handle, uint32_t flags)
>    if (callback_defined ("flush", &fn)) {
>      PyErr_Clear ();
> 
> -    r = PyObject_CallFunctionObjArgs (fn, obj, NULL);
> +    r = PyObject_CallFunctionObjArgs (fn, h->obj, NULL);
>      Py_DECREF (fn);
>      if (check_python_failure ("flush") == -1)
>        return -1;
> @@ -584,7 +599,7 @@ py_flush (void *handle, uint32_t flags)
>  static int
>  py_trim (void *handle, uint32_t count, uint64_t offset, uint32_t flags)
>  {
> -  PyObject *obj = handle;
> +  ConnHandle *h = handle;
>    PyObject *fn;
>    PyObject *args;
>    PyObject *kwargs;
> @@ -596,7 +611,7 @@ py_trim (void *handle, uint32_t count, uint64_t offset,
uint32_t flags)
>    if (callback_defined ("trim", &fn)) {
>      PyErr_Clear ();
> 
> -    args = Py_BuildValue ("OiL", obj, count, offset);
> +    args = Py_BuildValue ("OiL", h->obj, count, offset);
>      if (!args) {
>        check_python_failure ("trim");
>        Py_DECREF (fn);
> @@ -637,7 +652,7 @@ py_trim (void *handle, uint32_t count, uint64_t offset,
uint32_t flags)
>  static int
>  py_zero (void *handle, uint32_t count, uint64_t offset, uint32_t flags)
>  {
> -  PyObject *obj = handle;
> +  ConnHandle *h = handle;
>    PyObject *fn;
>    PyObject *args;
>    PyObject *kwargs;
> @@ -660,7 +675,7 @@ py_zero (void *handle, uint32_t count, uint64_t offset,
uint32_t flags)
>      PyErr_Clear ();
> 
>      last_error = 0;
> -    args = Py_BuildValue ("OiL", obj, count, offset);
> +    args = Py_BuildValue ("OiL", h->obj, count, offset);
>      if (!args) {
>        check_python_failure ("zero");
>        Py_DECREF (fn);
> @@ -718,7 +733,7 @@ py_zero (void *handle, uint32_t count, uint64_t offset,
uint32_t flags)
>  static int
>  py_can_write (void *handle)
>  {
> -  PyObject *obj = handle;
> +  ConnHandle *h = handle;
>    PyObject *fn;
>    PyObject *r;
>    int ret;
> @@ -726,7 +741,7 @@ py_can_write (void *handle)
>    if (callback_defined ("can_write", &fn)) {
>      PyErr_Clear ();
> 
> -    r = PyObject_CallFunctionObjArgs (fn, obj, NULL);
> +    r = PyObject_CallFunctionObjArgs (fn, h->obj, NULL);
>      Py_DECREF (fn);
>      if (check_python_failure ("can_write") == -1)
>        return -1;
> @@ -741,7 +756,7 @@ py_can_write (void *handle)
>  static int
>  py_can_flush (void *handle)
>  {
> -  PyObject *obj = handle;
> +  ConnHandle *h = handle;
>    PyObject *fn;
>    PyObject *r;
>    int ret;
> @@ -749,7 +764,7 @@ py_can_flush (void *handle)
>    if (callback_defined ("can_flush", &fn)) {
>      PyErr_Clear ();
> 
> -    r = PyObject_CallFunctionObjArgs (fn, obj, NULL);
> +    r = PyObject_CallFunctionObjArgs (fn, h->obj, NULL);
>      Py_DECREF (fn);
>      if (check_python_failure ("can_flush") == -1)
>        return -1;
> @@ -764,7 +779,7 @@ py_can_flush (void *handle)
>  static int
>  py_is_rotational (void *handle)
>  {
> -  PyObject *obj = handle;
> +  ConnHandle *h = handle;
>    PyObject *fn;
>    PyObject *r;
>    int ret;
> @@ -772,7 +787,7 @@ py_is_rotational (void *handle)
>    if (callback_defined ("is_rotational", &fn)) {
>      PyErr_Clear ();
> 
> -    r = PyObject_CallFunctionObjArgs (fn, obj, NULL);
> +    r = PyObject_CallFunctionObjArgs (fn, h->obj, NULL);
>      Py_DECREF (fn);
>      if (check_python_failure ("is_rotational") == -1)
>        return -1;
> @@ -787,7 +802,7 @@ py_is_rotational (void *handle)
>  static int
>  py_can_trim (void *handle)
>  {
> -  PyObject *obj = handle;
> +  ConnHandle *h = handle;
>    PyObject *fn;
>    PyObject *r;
>    int ret;
> @@ -795,7 +810,7 @@ py_can_trim (void *handle)
>    if (callback_defined ("can_trim", &fn)) {
>      PyErr_Clear ();
> 
> -    r = PyObject_CallFunctionObjArgs (fn, obj, NULL);
> +    r = PyObject_CallFunctionObjArgs (fn, h->obj, NULL);
>      Py_DECREF (fn);
>      if (check_python_failure ("can_trim") == -1)
>        return -1;
> @@ -810,7 +825,7 @@ py_can_trim (void *handle)
>  static int
>  py_can_zero (void *handle)
>  {
> -  PyObject *obj = handle;
> +  ConnHandle *h = handle;
>    PyObject *fn;
>    PyObject *r;
>    int ret;
> @@ -818,7 +833,7 @@ py_can_zero (void *handle)
>    if (callback_defined ("can_zero", &fn)) {
>      PyErr_Clear ();
> 
> -    r = PyObject_CallFunctionObjArgs (fn, obj, NULL);
> +    r = PyObject_CallFunctionObjArgs (fn, h->obj, NULL);
>      Py_DECREF (fn);
>      if (check_python_failure ("can_zero") == -1)
>        return -1;
> @@ -833,16 +848,20 @@ py_can_zero (void *handle)
>  static int
>  py_can_fua (void *handle)
>  {
> -  PyObject *obj = handle;
> +  ConnHandle *h = handle;
>    PyObject *fn;
>    PyObject *r;
>    int fua;
> 
> +  /* Check the cache first */
> +  if (h->fua != -1)
> +    return h->fua;
> +
>    /* We have to convert a python bool into the nbdkit tristate. */
>    if (callback_defined ("can_fua", &fn)) {
>      PyErr_Clear ();
> 
> -    r = PyObject_CallFunctionObjArgs (fn, obj, NULL);
> +    r = PyObject_CallFunctionObjArgs (fn, h->obj, NULL);
>      Py_DECREF (fn);
>      if (check_python_failure ("can_fua") == -1)
>        return -1;
> @@ -861,7 +880,7 @@ py_can_fua (void *handle)
>    else
>      fua = NBDKIT_FUA_NONE;
> 
> -  return fua;
> +  return h->fua = fua;
>  }
> 
>  #define py_config_help \
> -- 
> 2.14.3
> 
> _______________________________________________
> Libguestfs mailing list
> Libguestfs@redhat.com
> https://www.redhat.com/mailman/listinfo/libguestfs
-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
Fedora Windows cross-compiler. Compile Windows programs, test, and
build Windows installers. Over 100 libraries supported.
http://fedoraproject.org/wiki/MinGW