Libguestfs - Nov 2019 - [nbdkit PATCH 0/5] Counterproposal for python v2 interfaces

As mentioned in my reviews, I wonder if we should make our python
callbacks look a bit more Pythonic by having kwargs added for each
new flag that we want to expose.  The idea was first floated here:

https://www.redhat.com/archives/libguestfs/2018-April/msg00108.html

Note that with my proposal, there is no need for a python script to
expose a global API_VERSION variable; new flags are added using a
Python API that is backwards-compatible.  In fact, with introspection,
we can even get away with the user not having to write a
can_fast_zero; the mere presence of support for a fast=... kw
parameter is enough to let us pick the correct default.

However, there are aspects of Rich's proposal that I like better: for
example, by having an explicit API_VERSION variable, we can change the
interface of pread to read directly into a buffer, which my code can't
do.  And although mine produces interfaces that feel a bit more
Pythonic, I also note that mine requires more lines of code (and
that's without even implementing the cache/can_cache callback present
in Rich's proposal).  Thus, this is an RFC to see which pieces we like
best out of the various proposals, rather than something that should
be applied as-is.

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
  python: Add can_fast_zero support

 plugins/python/nbdkit-python-plugin.pod | 122 ++++++++--
 plugins/python/python.c                 | 286 ++++++++++++++++++++++--
 plugins/python/example.py               |   6 +-
 tests/test.py                           |   6 +-
 4 files changed, 375 insertions(+), 45 deletions(-)

-- 
2.21.0

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, even though 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                 | 78 +++++++++++++++++++++++--
 plugins/python/example.py               |  2 +-
 tests/test.py                           |  2 +-
 4 files changed, 102 insertions(+), 15 deletions(-)

diff --git a/plugins/python/nbdkit-python-plugin.pod
b/plugins/python/nbdkit-python-plugin.pod
index 3680fd65..f504cf7a 100644
--- a/plugins/python/nbdkit-python-plugin.pod
+++ b/plugins/python/nbdkit-python-plugin.pod
@@ -40,11 +40,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 Python versions

@@ -260,13 +273,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 7052aac0..d18bcb5e 100644
--- a/plugins/python/python.c
+++ b/plugins/python/python.c
@@ -1,5 +1,5 @@
 /* nbdkit
- * Copyright (C) 2013-2018 Red Hat Inc.
+ * Copyright (C) 2013-2019 Red Hat Inc.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions are
@@ -109,6 +109,43 @@ callback_defined (const char *name, PyObject **obj_rtn)
   return 1;
 }

+/* 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, *params;
+
+  assert (script != NULL);
+  assert (module != NULL);
+
+  pname = PyUnicode_FromString ("inspect");
+  if (!pname)
+    return -1;
+  inspect = PyImport_Import (pname);
+  Py_DECREF (pname);
+
+  if (!inspect)
+    return -1;
+
+  spec = PyObject_CallMethod (inspect, "signature", "O",
fn);
+  Py_DECREF (inspect);
+  if (!spec)
+    return -1;
+
+  /* We got the signature; now check for the requested keyword */
+  params = PyObject_GetAttrString (spec, "parameters");
+  if (!params) {
+    Py_DECREF (spec);
+    return -1;
+  }
+  r = PyMapping_HasKeyString (params, name);
+
+  Py_DECREF (spec);
+  Py_DECREF (params);
+  return r;
+}
+
 /* Convert bytes/str/unicode into a string.  Caller must free. */
 static char *
 python_to_string (PyObject *str)
@@ -564,16 +601,49 @@ 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;
-    r = PyObject_CallFunction (fn, "OiLO",
-                               obj, count, offset,
-                               may_trim ? Py_True : Py_False);
+    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 || last_error == ENOTSUP) {
       /* When user requests this particular error, we want to
        * gracefully fall back, and to accommodate both a normal return
diff --git a/plugins/python/example.py b/plugins/python/example.py
index 60f9d7f0..9205f107 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 9a2e947d..a83f1181 100644
--- a/tests/test.py
+++ b/tests/test.py
@@ -43,7 +43,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.21.0

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

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

diff --git a/plugins/python/nbdkit-python-plugin.pod
b/plugins/python/nbdkit-python-plugin.pod
index f504cf7a..ec126f5a 100644
--- a/plugins/python/nbdkit-python-plugin.pod
+++ b/plugins/python/nbdkit-python-plugin.pod
@@ -208,6 +208,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)
@@ -317,7 +324,6 @@ C<config_help>,
 C<magic_config_key>,
 C<can_fua>,
 C<can_cache>,
-C<can_zero>,
 C<can_fast_zero>,
 C<can_extents>,
 C<can_multi_conn>,
diff --git a/plugins/python/python.c b/plugins/python/python.c
index d18bcb5e..aa09dce7 100644
--- a/plugins/python/python.c
+++ b/plugins/python/python.c
@@ -718,6 +718,12 @@ py_can_trim (void *handle)
   return boolean_callback (handle, "can_trim", "trim");
 }

+static int
+py_can_zero (void *handle)
+{
+  return boolean_callback (handle, "can_zero", "zero");
+}
+
 #define py_config_help \
   "script=<FILENAME>     (required) The Python plugin to
run.\n" \
   "[other arguments may be used by the plugin that you load]"
@@ -744,6 +750,7 @@ static struct nbdkit_plugin plugin = {
   .can_write         = py_can_write,
   .can_flush         = py_can_flush,
   .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 a83f1181..1b7d3e0e 100644
--- a/tests/test.py
+++ b/tests/test.py
@@ -32,6 +32,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.21.0

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 | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/plugins/python/python.c b/plugins/python/python.c
index aa09dce7..1954881c 100644
--- a/plugins/python/python.c
+++ b/plugins/python/python.c
@@ -46,6 +46,8 @@
 #define PY_SSIZE_T_CLEAN 1
 #include <Python.h>

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

 #include "cleanup.h"
@@ -477,7 +479,7 @@ 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;
@@ -485,6 +487,7 @@ py_pread (void *handle, void *buf,
   Py_buffer view = {0};
   int ret = -1;

+  assert (!flags);
   if (!callback_defined ("pread", &fn)) {
     nbdkit_error ("%s: missing callback: %s", script,
"pread");
     return ret;
@@ -523,12 +526,13 @@ out:

 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 ();

@@ -549,12 +553,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 ();

@@ -573,12 +578,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 ();

@@ -597,14 +603,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.21.0

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 |  46 +++++++-
 plugins/python/python.c                 | 148 +++++++++++++++++++++---
 plugins/python/example.py               |   6 +-
 3 files changed, 179 insertions(+), 21 deletions(-)

diff --git a/plugins/python/nbdkit-python-plugin.pod
b/plugins/python/nbdkit-python-plugin.pod
index ec126f5a..6940f4d9 100644
--- a/plugins/python/nbdkit-python-plugin.pod
+++ b/plugins/python/nbdkit-python-plugin.pod
@@ -215,6 +215,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)
@@ -239,7 +255,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

@@ -247,6 +263,12 @@ The body of your C<pwrite> function should write the
buffer C<buf> 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,
@@ -262,6 +284,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.

@@ -269,18 +296,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, fua=False):
    # no return value

 The body of your C<zero> function should ensure that C<count> bytes
@@ -292,6 +325,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,
@@ -322,7 +361,6 @@ C<longname>,
 C<description>,
 C<config_help>,
 C<magic_config_key>,
-C<can_fua>,
 C<can_cache>,
 C<can_fast_zero>,
 C<can_extents>,
diff --git a/plugins/python/python.c b/plugins/python/python.c
index 1954881c..677420bd 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)
@@ -368,6 +371,32 @@ py_config (const char *key, const char *value)
                     "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. */
@@ -524,22 +553,52 @@ out:
   return ret;
 }

+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,
+    args = Py_BuildValue ("ONL", obj,
             PyMemoryView_FromMemory ((char *)buf, count, PyBUF_READ),
             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);
@@ -549,7 +608,7 @@ py_pwrite (void *handle, const void *buf,
     return -1;
   }

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

 static int
@@ -582,14 +641,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);
+    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);
@@ -599,7 +686,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
@@ -611,8 +698,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;

@@ -648,6 +737,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);
@@ -665,7 +763,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");
@@ -674,7 +772,8 @@ py_zero (void *handle, uint32_t count, uint64_t offset,
uint32_t flags)
 }

 static int
-boolean_callback (void *handle, const char *can_fn, const char *plain_fn)
+boolean_callback (void *handle, const char *can_fn, const char *plain_fn,
+                  int missing)
 {
   PyObject *obj = handle;
   PyObject *fn;
@@ -699,37 +798,55 @@ boolean_callback (void *handle, const char *can_fn, const
char *plain_fn)
   else if (plain_fn && callback_defined (plain_fn, NULL))
     return 1;
   else
-    return 0;
+    return missing;
 }

 static int
 py_is_rotational (void *handle)
 {
-  return boolean_callback (handle, "is_rotational", NULL);
+  return boolean_callback (handle, "is_rotational", NULL, 0);
 }

 static int
 py_can_write (void *handle)
 {
-  return boolean_callback (handle, "can_write", "pwrite");
+  return boolean_callback (handle, "can_write", "pwrite",
0);
 }

 static int
 py_can_flush (void *handle)
 {
-  return boolean_callback (handle, "can_flush", "flush");
+  return boolean_callback (handle, "can_flush", "flush",
0);
 }

 static int
 py_can_trim (void *handle)
 {
-  return boolean_callback (handle, "can_trim", "trim");
+  return boolean_callback (handle, "can_trim", "trim", 0);
 }

 static int
 py_can_zero (void *handle)
 {
-  return boolean_callback (handle, "can_zero", "zero");
+  return boolean_callback (handle, "can_zero", "zero", 0);
+}
+
+static int
+py_can_fua (void *handle)
+{
+  /* We have to convert a python bool into the nbdkit tristate. */
+  switch (boolean_callback (handle, "can_fua", NULL, 1)) {
+  case -1:
+    return -1;
+  case 0:
+    return NBDKIT_FUA_NONE;
+  default:
+    if (pwrite_has_fua || zero_has_fua || trim_has_fua)
+      return NBDKIT_FUA_NATIVE;
+    if (callback_defined ("flush", NULL))
+      return NBDKIT_FUA_EMULATE;
+    return NBDKIT_FUA_NONE;
+  }
 }

 #define py_config_help \
@@ -759,6 +876,7 @@ static struct nbdkit_plugin plugin = {
   .can_flush         = py_can_flush,
   .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 9205f107..ca7c2661 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.21.0

Add the can_fast_zero callback, and yet another bool keyword parameter
to the zero callback.

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

diff --git a/plugins/python/nbdkit-python-plugin.pod
b/plugins/python/nbdkit-python-plugin.pod
index 6940f4d9..7b0051af 100644
--- a/plugins/python/nbdkit-python-plugin.pod
+++ b/plugins/python/nbdkit-python-plugin.pod
@@ -215,6 +215,18 @@ contents will be garbage collected.
  def can_zero(h):
    # return a boolean

+
+=item C<can_fast_zero>
+
+(Optional)
+
+ def can_fast_zero(h):
+   # return a boolean
+
+Unlike the C counterpart, this can often be omitted: the Python
+callback uses Python introspection to default to true if the C<zero>
+callback supports an optional parameter C<fast>.
+
 =item C<can_fua>

 (Optional)
@@ -313,7 +325,7 @@ request for FUA by calling C<flush>.

 (Optional)

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

 The body of your C<zero> function should ensure that C<count> bytes
@@ -331,13 +343,19 @@ 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,
-optionally using C<nbdkit.set_error> first.  In particular, if
-you would like to automatically fall back to C<pwrite> (perhaps
-because there is nothing to optimize if C<may_trim> is false),
-use C<nbdkit.set_error(errno.EOPNOTSUPP)>.
+Your function may support an optional C<fast> parameter; if it is
+present and the caller sets it to True, then your callback must return
+a failure if it is not any faster than a corresponding C<pwrite>. If
+it is absent but C<can_fast_zero> returned True, then nbdkit treats
+all fast zero requests as failures.
+
+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, optionally using
+C<nbdkit.set_error> first.  In particular, if you would like to
+automatically fall back to C<pwrite> (perhaps because there is nothing
+to optimize if C<may_trim> is false) or declare that fast zero is not
+possible, use C<nbdkit.set_error(errno.EOPNOTSUPP)>.

 =back

@@ -362,7 +380,6 @@ C<description>,
 C<config_help>,
 C<magic_config_key>,
 C<can_cache>,
-C<can_fast_zero>,
 C<can_extents>,
 C<can_multi_conn>,
 C<cache>,
diff --git a/plugins/python/python.c b/plugins/python/python.c
index 677420bd..fee339f4 100644
--- a/plugins/python/python.c
+++ b/plugins/python/python.c
@@ -699,11 +699,14 @@ py_zero (void *handle, uint32_t count, uint64_t offset,
uint32_t flags)
   PyObject *r;
   int may_trim = (flags & NBDKIT_FLAG_MAY_TRIM) != 0;
   int fua = (flags & NBDKIT_FLAG_FUA) != 0;
+  int fast = (flags & NBDKIT_FLAG_FAST_ZERO) != 0;
   int need_flush = fua && !zero_has_fua;

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

     if (zero_may_trim < 0)
       zero_may_trim = callback_has_parameter (fn, "may_trim");
@@ -711,6 +714,18 @@ py_zero (void *handle, uint32_t count, uint64_t offset,
uint32_t flags)
       check_python_failure ("zero");
       return -1;
     }
+    if (zero_fast < 0)
+      zero_fast = callback_has_parameter (fn, "fast");
+    if (zero_fast < 0) {
+      check_python_failure ("zero");
+      return -1;
+    }
+
+    if (fast && zero_fast != 1) {
+      nbdkit_debug ("zero lacks fast support, failing fast request");
+      nbdkit_set_error (EOPNOTSUPP);
+      return -1;
+    }

     PyErr_Clear ();

@@ -746,6 +761,15 @@ py_zero (void *handle, uint32_t count, uint64_t offset,
uint32_t flags)
       Py_DECREF (fn);
       return -1;
     }
+    if (zero_fast &&
+        PyDict_SetItemString (kwargs, "fast",
+                              fast ? 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);
@@ -831,6 +855,24 @@ py_can_zero (void *handle)
   return boolean_callback (handle, "can_zero", "zero", 0);
 }

+static int
+py_can_fast_zero (void *handle)
+{
+  int r = boolean_callback (handle, "can_fast_zero", NULL, 2);
+  PyObject *fn;
+
+  if (r == 2) {
+    /* can_fast_zero was missing, but we still want to default to true
+     * if the zero callback is missing or supports optional fast argument.
+     */
+    if (callback_defined ("zero", &fn))
+      r = callback_has_parameter (fn, "fast");
+    else
+      r = 1;
+  }
+  return r;
+}
+
 static int
 py_can_fua (void *handle)
 {
@@ -876,6 +918,7 @@ static struct nbdkit_plugin plugin = {
   .can_flush         = py_can_flush,
   .can_trim          = py_can_trim,
   .can_zero          = py_can_zero,
+  .can_fast_zero     = py_can_fast_zero,
   .can_fua           = py_can_fua,

   .pread             = py_pread,
diff --git a/plugins/python/example.py b/plugins/python/example.py
index ca7c2661..b1a9fed1 100644
--- a/plugins/python/example.py
+++ b/plugins/python/example.py
@@ -67,7 +67,7 @@ def pwrite(h, buf, offset, fua=False):
     disk[offset:end] = buf


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

Thanks for posting this.  Some inconclusive comments in no particular
order:

I don't have enough experience to judge whether or not kwargs are more
Pythonic.  My impression is that bitmasks are used in the library and
so are kwargs, and kwargs are used more often, and bitmasks seem to be
used mainly in lowlevel functions where the Python code is talking to C.

About API_VERSION, I think we will need that in order to implement
pread into a buf, unless we implement both pread() (returning buf) and
a second call preadinto(..., buf, ...).  In my current patch series I
only implemented pread(..., buf, ...) so API_VERSION is definitely
required by mine.
>From the virt-v2v point of view, performance is the most important
factor for us.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-p2v converts physical machines to virtual machines.  Boot with a
live CD or over the network (PXE) and turn machines into KVM guests.
http://libguestfs.org/virt-v2v