Libguestfs - Nov 2019 - Re: [PATCH nbdkit v2 03/10] python: Implement nbdkit API version 2.

v1:
https://www.redhat.com/archives/libguestfs/2019-November/msg00153.html

v2:

 - Fix implementation of can_cache.

 - Add implementation of can_fua.

 - Add a very thorough test suite which tests every command + flag
   combination.

It is unclear why we were constructing this by hand, but using the
following tip we can use PyObject_CallFunction:
https://stackoverflow.com/a/21221335
---
 plugins/python/python.c | 17 ++++++-----------
 1 file changed, 6 insertions(+), 11 deletions(-)

diff --git a/plugins/python/python.c b/plugins/python/python.c
index 148097f..d65ac45 100644
--- a/plugins/python/python.c
+++ b/plugins/python/python.c
@@ -557,26 +557,21 @@ py_zero (void *handle, uint32_t count, uint64_t offset,
int may_trim)
 {
   PyObject *obj = handle;
   PyObject *fn;
-  PyObject *args;
   PyObject *r;
 
   if (callback_defined ("zero", &fn)) {
     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);
+    r = PyObject_CallFunction (fn, "OiLO",
+                               obj, count, offset,
+                               may_trim ? Py_True : Py_False, NULL);
     Py_DECREF (fn);
-    Py_DECREF (args);
     if (last_error == EOPNOTSUPP || last_error == ENOTSUP) {
       /* When user requests this particular error, we want to
-         gracefully fall back, and to accomodate both a normal return
-         and an exception. */
+       * gracefully fall back, and to accomodate both a normal return
+       * and an exception.
+       */
       nbdkit_debug ("zero requested falling back to pwrite");
       Py_XDECREF (r);
       PyErr_Clear ();
-- 
2.23.0

These are accessible from the plugin by:

  import nbdkit

  if flags & nbdkit.FLAG_MAY_TRIM:
  &c.

Many (all?) of these are not yet useful for plugins, some will never
be useful, but they only consume a tiny bit of memory and it's nice to
have the complete set available for future use.
---
 plugins/python/python.c | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/plugins/python/python.c b/plugins/python/python.c
index d65ac45..69cf4e9 100644
--- a/plugins/python/python.c
+++ b/plugins/python/python.c
@@ -231,6 +231,33 @@ create_nbdkit_module (void)
     nbdkit_error ("could not create the nbdkit API module");
     exit (EXIT_FAILURE);
   }
+
+  /* Constants corresponding to various flags. */
+  PyModule_AddIntConstant (m, "THREAD_MODEL_SERIALIZE_CONNECTIONS",
+                           NBDKIT_THREAD_MODEL_SERIALIZE_CONNECTIONS);
+  PyModule_AddIntConstant (m, "THREAD_MODEL_SERIALIZE_ALL_REQUESTS",
+                           NBDKIT_THREAD_MODEL_SERIALIZE_ALL_REQUESTS);
+  PyModule_AddIntConstant (m, "THREAD_MODEL_SERIALIZE_REQUESTS",
+                           NBDKIT_THREAD_MODEL_SERIALIZE_REQUESTS);
+  PyModule_AddIntConstant (m, "THREAD_MODEL_PARALLEL",
+                           NBDKIT_THREAD_MODEL_PARALLEL);
+
+  PyModule_AddIntConstant (m, "FLAG_MAY_TRIM", 
NBDKIT_FLAG_MAY_TRIM);
+  PyModule_AddIntConstant (m, "FLAG_FUA",       NBDKIT_FLAG_FUA);
+  PyModule_AddIntConstant (m, "FLAG_REQ_ONE",   NBDKIT_FLAG_REQ_ONE);
+  PyModule_AddIntConstant (m, "FLAG_FAST_ZERO",
NBDKIT_FLAG_FAST_ZERO);
+
+  PyModule_AddIntConstant (m, "FUA_NONE",       NBDKIT_FUA_NONE);
+  PyModule_AddIntConstant (m, "FUA_EMULATE",    NBDKIT_FUA_EMULATE);
+  PyModule_AddIntConstant (m, "FUA_NATIVE",     NBDKIT_FUA_NATIVE);
+
+  PyModule_AddIntConstant (m, "CACHE_NONE",     NBDKIT_CACHE_NONE);
+  PyModule_AddIntConstant (m, "CACHE_EMULATE", 
NBDKIT_CACHE_EMULATE);
+  PyModule_AddIntConstant (m, "CACHE_NATIVE",   NBDKIT_CACHE_NATIVE);
+
+  PyModule_AddIntConstant (m, "EXTENT_HOLE",    NBDKIT_EXTENT_HOLE);
+  PyModule_AddIntConstant (m, "EXTENT_ZERO",    NBDKIT_EXTENT_ZERO);
+
   return m;
 }
 
-- 
2.23.0

To avoid breaking existing plugins, Python plugins wishing to use
version 2 of the API must opt in by declaring:

  def api_version():
    return 2

(Plugins which do not do this are assumed to want API version 1).
---
 plugins/python/example.py               |  15 +++-
 plugins/python/nbdkit-python-plugin.pod |  61 ++++++++++-----
 plugins/python/python.c                 | 100 ++++++++++++++++++++----
 tests/test.py                           |  20 +++--
 4 files changed, 148 insertions(+), 48 deletions(-)

diff --git a/plugins/python/example.py b/plugins/python/example.py
index 60f9d7f..25a0049 100644
--- a/plugins/python/example.py
+++ b/plugins/python/example.py
@@ -34,6 +34,13 @@ import errno
 disk = bytearray(1024 * 1024)
 
 
+# There are several variants of the API.  nbdkit will call this
+# function first to determine which one you want to use.  This is the
+# latest version at the time this example was written.
+def api_version():
+    return 2
+
+
 # This just prints the extra command line parameters, but real plugins
 # should parse them and reject any unknown parameters.
 def config(key, value):
@@ -54,20 +61,20 @@ def get_size(h):
     return len(disk)
 
 
-def pread(h, count, offset):
+def pread(h, count, offset, flags):
     global disk
     return disk[offset:offset+count]
 
 
-def pwrite(h, buf, offset):
+def pwrite(h, buf, offset, flags):
     global disk
     end = offset + len(buf)
     disk[offset:end] = buf
 
 
-def zero(h, count, offset, may_trim):
+def zero(h, count, offset, flags):
     global disk
-    if may_trim:
+    if flags & nbdkit.FLAG_MAY_TRIM:
         disk[offset:offset+count] = bytearray(count)
     else:
         nbdkit.set_error(errno.EOPNOTSUPP)
diff --git a/plugins/python/nbdkit-python-plugin.pod
b/plugins/python/nbdkit-python-plugin.pod
index 6453474..882c0d8 100644
--- a/plugins/python/nbdkit-python-plugin.pod
+++ b/plugins/python/nbdkit-python-plugin.pod
@@ -82,6 +82,19 @@ I<--dump-plugin> option, eg:
  python_version=3.7.0
  python_pep_384_abi_version=3
 
+=head2 API versions
+
+The nbdkit API has evolved and new versions are released periodically.
+To ensure backwards compatibility plugins have to opt in to the new
+version.  From Python you do this by declaring a function in your
+module:
+
+ def api_version():
+   return 2
+
+(where 2 is the latest version at the time this documentation was
+written).  All newly written Python modules must have this function.
+
 =head2 Executable script
 
 If you want you can make the script executable and include a
"shebang"
@@ -120,6 +133,10 @@ nbdkit-plugin(3).
 
 =over 4
 
+=item C<api_version>
+
+There are no arguments.  It must return C<2>.
+
 =item C<dump_plugin>
 
 (Optional)
@@ -199,12 +216,12 @@ contents will be garbage collected.
 
 (Required)
 
- def pread(h, count, offset):
+ def pread(h, count, offset, flags):
    # construct a bytearray of length count bytes and return it
 
 The body of your C<pread> function should construct a buffer of length
 (at least) C<count> bytes.  You should read C<count> bytes from the
-disk starting at C<offset>.
+disk starting at C<offset>.  C<flags> is always 0.
 
 NBD only supports whole reads, so your function should try to read
 the whole region (perhaps requiring a loop).  If the read fails or
@@ -215,13 +232,13 @@ C<nbdkit.set_error> first.
 
 (Optional)
 
- def pwrite(h, buf, offset):
+ def pwrite(h, buf, offset, flags):
    length = len (buf)
    # no return value
 
 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>.
+C<offset>.  C<flags> may contain C<nbdkit.FLAG_FUA>.
 
 NBD only supports whole writes, so your function should try to
 write the whole region (perhaps requiring a loop).  If the write
@@ -232,11 +249,12 @@ fails or is partial, your function should throw an
exception,
 
 (Optional)
 
- def flush(h):
+ def flush(h, flags):
    # no return value
 
 The body of your C<flush> function should do a L<sync(2)> or
 L<fdatasync(2)> or equivalent on the backing store.
+C<flags> is always 0.
 
 If the flush fails, your function should throw an exception, optionally
 using C<nbdkit.set_error> first.
@@ -245,32 +263,35 @@ using C<nbdkit.set_error> first.
 
 (Optional)
 
- def trim(h, count, offset):
+ def trim(h, count, offset, flags):
    # 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.
+The body of your C<trim> function should "punch a hole" in the
backing
+store.  C<flags> may contain C<nbdkit.FLAG_FUA>.  If the trim
fails,
+your function should throw an exception, optionally using
+C<nbdkit.set_error> first.
 
 =item C<zero>
 
 (Optional)
 
- def zero(h, count, offset, may_trim):
+ def zero(h, count, offset, flags):
    # 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.
+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.  C<flags>
is
+a bitmask which may include C<nbdkit.FLAG_MAY_TRIM>,
+C<nbdkit.FLAG_FUA>, C<nbdkit.FLAG_FAST_ZERO>.
 
 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)>.
+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
+S<C<flags & nbdkit.FLAG_MAY_TRIM>> is false), use
+S<C<nbdkit.set_error (errno.EOPNOTSUPP)>>.
 
 =back
 
diff --git a/plugins/python/python.c b/plugins/python/python.c
index 69cf4e9..20e05e0 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"
@@ -60,6 +62,7 @@
  */
 static const char *script;
 static PyObject *module;
+static int py_api_version = 1;
 
 static int last_error;
 
@@ -356,6 +359,26 @@ py_config (const char *key, const char *value)
                     "nbdkit requires these callbacks.", script);
       return -1;
     }
+
+    /* Get the API version. */
+    if (callback_defined ("api_version", &fn)) {
+      PyErr_Clear ();
+
+      r = PyObject_CallObject (fn, NULL);
+      Py_DECREF (fn);
+      if (check_python_failure ("api_version") == -1)
+        return -1;
+      py_api_version = (int) PyLong_AsLong (r);
+      Py_DECREF (r);
+      if (check_python_failure ("PyLong_AsLong") == -1)
+        return -1;
+      if (py_api_version < 1 || py_api_version > NBDKIT_API_VERSION) {
+        nbdkit_error ("%s: api_version() requested unknown version: %d. 
"
+                      "This plugin supports API versions between 1 and
%d.",
+                      script, py_api_version, NBDKIT_API_VERSION);
+        return -1;
+      }
+    }
   }
   else if (callback_defined ("config", &fn)) {
     /* Other parameters are passed to the Python .config callback. */
@@ -466,8 +489,8 @@ py_get_size (void *handle)
 }
 
 static int
-py_pread (void *handle, void *buf,
-          uint32_t count, uint64_t offset)
+py_pread (void *handle, void *buf, uint32_t count, uint64_t offset,
+          uint32_t flags)
 {
   PyObject *obj = handle;
   PyObject *fn;
@@ -480,7 +503,15 @@ py_pread (void *handle, void *buf,
 
   PyErr_Clear ();
 
-  r = PyObject_CallFunction (fn, "OiL", obj, count, offset, NULL);
+  switch (py_api_version) {
+  case 1:
+    r = PyObject_CallFunction (fn, "OiL", obj, count, offset, NULL);
+    break;
+  case 2:
+    r = PyObject_CallFunction (fn, "OiLI", obj, count, offset, flags,
NULL);
+    break;
+  default: abort ();
+  }
   Py_DECREF (fn);
   if (check_python_failure ("pread") == -1)
     return -1;
@@ -505,8 +536,8 @@ py_pread (void *handle, void *buf,
 }
 
 static int
-py_pwrite (void *handle, const void *buf,
-           uint32_t count, uint64_t offset)
+py_pwrite (void *handle, const void *buf, uint32_t count, uint64_t offset,
+           uint32_t flags)
 {
   PyObject *obj = handle;
   PyObject *fn;
@@ -515,9 +546,19 @@ py_pwrite (void *handle, const void *buf,
   if (callback_defined ("pwrite", &fn)) {
     PyErr_Clear ();
 
-    r = PyObject_CallFunction (fn, "ONL", obj,
-                               PyByteArray_FromStringAndSize (buf, count),
-                               offset, NULL);
+    switch (py_api_version) {
+    case 1:
+      r = PyObject_CallFunction (fn, "ONL", obj,
+                                 PyByteArray_FromStringAndSize (buf, count),
+                                 offset, NULL);
+      break;
+    case 2:
+      r = PyObject_CallFunction (fn, "ONLI", obj,
+                                 PyByteArray_FromStringAndSize (buf, count),
+                                 offset, flags, NULL);
+      break;
+    default: abort ();
+    }
     Py_DECREF (fn);
     if (check_python_failure ("pwrite") == -1)
       return -1;
@@ -532,7 +573,7 @@ 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;
@@ -541,7 +582,15 @@ py_flush (void *handle)
   if (callback_defined ("flush", &fn)) {
     PyErr_Clear ();
 
-    r = PyObject_CallFunctionObjArgs (fn, obj, NULL);
+    switch (py_api_version) {
+    case 1:
+      r = PyObject_CallFunctionObjArgs (fn, obj, NULL);
+      break;
+    case 2:
+      r = PyObject_CallFunction (fn, "OI", obj, flags, NULL);
+      break;
+    default: abort ();
+    }
     Py_DECREF (fn);
     if (check_python_failure ("flush") == -1)
       return -1;
@@ -556,7 +605,7 @@ 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;
@@ -565,7 +614,15 @@ py_trim (void *handle, uint32_t count, uint64_t offset)
   if (callback_defined ("trim", &fn)) {
     PyErr_Clear ();
 
-    r = PyObject_CallFunction (fn, "OiL", obj, count, offset, NULL);
+    switch (py_api_version) {
+    case 1:
+      r = PyObject_CallFunction (fn, "OiL", obj, count, offset,
NULL);
+      break;
+    case 2:
+      r = PyObject_CallFunction (fn, "OiLI", obj, count, offset,
flags, NULL);
+      break;
+    default: abort ();
+    }
     Py_DECREF (fn);
     if (check_python_failure ("trim") == -1)
       return -1;
@@ -580,7 +637,7 @@ 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;
@@ -590,9 +647,20 @@ py_zero (void *handle, uint32_t count, uint64_t offset, int
may_trim)
     PyErr_Clear ();
 
     last_error = 0;
-    r = PyObject_CallFunction (fn, "OiLO",
-                               obj, count, offset,
-                               may_trim ? Py_True : Py_False, NULL);
+    switch (py_api_version) {
+    case 1: {
+      int may_trim = flags & NBDKIT_FLAG_MAY_TRIM;
+      r = PyObject_CallFunction (fn, "OiLO",
+                                 obj, count, offset,
+                                 may_trim ? Py_True : Py_False, NULL);
+      break;
+    }
+    case 2:
+      r = PyObject_CallFunction (fn, "OiLI",
+                                 obj, count, offset, flags, NULL);
+      break;
+    default: abort ();
+    }
     Py_DECREF (fn);
     if (last_error == EOPNOTSUPP || last_error == ENOTSUP) {
       /* When user requests this particular error, we want to
diff --git a/tests/test.py b/tests/test.py
index 9a2e947..ac80d96 100644
--- a/tests/test.py
+++ b/tests/test.py
@@ -3,6 +3,10 @@ import nbdkit
 disk = bytearray(1024*1024)
 
 
+def api_version():
+    return 2
+
+
 def config_complete():
     print ("set_error = %r" % nbdkit.set_error)
 
@@ -32,25 +36,25 @@ def can_trim(h):
     return True
 
 
-def pread(h, count, offset):
+def pread(h, count, offset, flags):
     global disk
     return disk[offset:offset+count]
 
 
-def pwrite(h, buf, offset):
+def pwrite(h, buf, offset, flags):
     global disk
     end = offset + len(buf)
     disk[offset:end] = buf
 
 
-def zero(h, count, offset, may_trim=False):
-    global disk
-    disk[offset:offset+count] = bytearray(count)
+def flush(h, flags):
+    pass
 
 
-def flush(h):
+def trim(h, count, offset, flags):
     pass
 
 
-def trim(h, count, offset):
-    pass
+def zero(h, count, offset, flags):
+    global disk
+    disk[offset:offset+count] = bytearray(count)
-- 
2.23.0

---
 plugins/python/nbdkit-python-plugin.pod | 21 +++++++++++++++++++--
 1 file changed, 19 insertions(+), 2 deletions(-)

diff --git a/plugins/python/nbdkit-python-plugin.pod
b/plugins/python/nbdkit-python-plugin.pod
index 882c0d8..51e0f57 100644
--- a/plugins/python/nbdkit-python-plugin.pod
+++ b/plugins/python/nbdkit-python-plugin.pod
@@ -304,8 +304,25 @@ S<C<nbdkit.set_error (errno.EOPNOTSUPP)>>.
 These are not needed because you can just use ordinary Python
 constructs.
 
-=item Missing: C<name>, C<version>, C<longname>,
C<description>,
-C<config_help>, C<can_fua>, C<can_cache>, C<cache>
+=item Missing: C<thread_model>
+
+Probably not implementable while Python has a global interpreter lock.
+
+=item Missing:
+C<name>,
+C<version>,
+C<longname>,
+C<description>,
+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>,
+C<cache>,
+C<extents>.
 
 These are not yet supported.
 
-- 
2.23.0

This change is pure refactoring and should have no effect.
---
 plugins/python/nbdkit-python-plugin.pod | 12 ++--
 plugins/python/python.c                 | 90 +++++--------------------
 2 files changed, 24 insertions(+), 78 deletions(-)

diff --git a/plugins/python/nbdkit-python-plugin.pod
b/plugins/python/nbdkit-python-plugin.pod
index 51e0f57..0fd4dcb 100644
--- a/plugins/python/nbdkit-python-plugin.pod
+++ b/plugins/python/nbdkit-python-plugin.pod
@@ -184,25 +184,25 @@ contents will be garbage collected.
  def get_size(h):
    # return the size of the disk
 
-=item C<can_write>
+=item C<is_rotational>
 
 (Optional)
 
- def can_write(h):
+ def is_rotational(h):
    # return a boolean
 
-=item C<can_flush>
+=item C<can_write>
 
 (Optional)
 
- def can_flush(h):
+ def can_write(h):
    # return a boolean
 
-=item C<is_rotational>
+=item C<can_flush>
 
 (Optional)
 
- def is_rotational(h):
+ def can_flush(h):
    # return a boolean
 
 =item C<can_trim>
diff --git a/plugins/python/python.c b/plugins/python/python.c
index 20e05e0..9445343 100644
--- a/plugins/python/python.c
+++ b/plugins/python/python.c
@@ -684,110 +684,56 @@ py_zero (void *handle, uint32_t count, uint64_t offset,
uint32_t flags)
 }
 
 static int
-py_can_write (void *handle)
+boolean_callback (void *handle, const char *can_fn, const char *plain_fn)
 {
   PyObject *obj = handle;
   PyObject *fn;
   PyObject *r;
   int ret;
 
-  if (callback_defined ("can_write", &fn)) {
+  if (callback_defined (can_fn, &fn)) {
     PyErr_Clear ();
 
     r = PyObject_CallFunctionObjArgs (fn, obj, NULL);
     Py_DECREF (fn);
-    if (check_python_failure ("can_write") == -1)
+    if (check_python_failure (can_fn) == -1)
       return -1;
     ret = r == Py_True;
     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).
+  /* No Python ‘can_fn’ (eg. ‘can_write’), but if there's a Python
+   * ‘plain_fn’ (eg. ‘pwrite’) callback defined, return 1.  (In C
+   * modules, nbdkit would do this).
    */
-  else if (callback_defined ("pwrite", NULL))
+  else if (plain_fn && callback_defined (plain_fn, NULL))
     return 1;
   else
     return 0;
 }
 
 static int
-py_can_flush (void *handle)
+py_is_rotational (void *handle)
 {
-  PyObject *obj = handle;
-  PyObject *fn;
-  PyObject *r;
-  int ret;
-
-  if (callback_defined ("can_flush", &fn)) {
-    PyErr_Clear ();
-
-    r = PyObject_CallFunctionObjArgs (fn, obj, NULL);
-    Py_DECREF (fn);
-    if (check_python_failure ("can_flush") == -1)
-      return -1;
-    ret = r == Py_True;
-    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;
+  return boolean_callback (handle, "is_rotational", NULL);
 }
 
 static int
-py_is_rotational (void *handle)
+py_can_write (void *handle)
 {
-  PyObject *obj = handle;
-  PyObject *fn;
-  PyObject *r;
-  int ret;
-
-  if (callback_defined ("is_rotational", &fn)) {
-    PyErr_Clear ();
+  return boolean_callback (handle, "can_write", "pwrite");
+}
 
-    r = PyObject_CallFunctionObjArgs (fn, obj, NULL);
-    Py_DECREF (fn);
-    if (check_python_failure ("is_rotational") == -1)
-      return -1;
-    ret = r == Py_True;
-    Py_DECREF (r);
-    return ret;
-  }
-  else
-    return 0;
+static int
+py_can_flush (void *handle)
+{
+  return boolean_callback (handle, "can_flush", "flush");
 }
 
 static int
 py_can_trim (void *handle)
 {
-  PyObject *obj = handle;
-  PyObject *fn;
-  PyObject *r;
-  int ret;
-
-  if (callback_defined ("can_trim", &fn)) {
-    PyErr_Clear ();
-
-    r = PyObject_CallFunctionObjArgs (fn, obj, NULL);
-    Py_DECREF (fn);
-    if (check_python_failure ("can_trim") == -1)
-      return -1;
-    ret = r == Py_True;
-    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;
+  return boolean_callback (handle, "can_trim", "trim");
 }
 
 #define py_config_help \
@@ -812,9 +758,9 @@ static struct nbdkit_plugin plugin = {
   .close             = py_close,
 
   .get_size          = py_get_size,
+  .is_rotational     = py_is_rotational,
   .can_write         = py_can_write,
   .can_flush         = py_can_flush,
-  .is_rotational     = py_is_rotational,
   .can_trim          = py_can_trim,
 
   .pread             = py_pread,
-- 
2.23.0

However this does not implement can_cache, since that is not a simple
boolean.
---
 plugins/python/nbdkit-python-plugin.pod | 14 +++++++++-
 plugins/python/python.c                 | 34 +++++++++++++++++++++++++
 2 files changed, 47 insertions(+), 1 deletion(-)

diff --git a/plugins/python/nbdkit-python-plugin.pod
b/plugins/python/nbdkit-python-plugin.pod
index 0fd4dcb..ee58cd7 100644
--- a/plugins/python/nbdkit-python-plugin.pod
+++ b/plugins/python/nbdkit-python-plugin.pod
@@ -293,6 +293,19 @@ because there is nothing to optimize if
 S<C<flags & nbdkit.FLAG_MAY_TRIM>> is false), use
 S<C<nbdkit.set_error (errno.EOPNOTSUPP)>>.
 
+=item C<cache>
+
+(Optional)
+
+ def cache(h, count, offset, flags):
+   # no return value
+
+The body of your C<cache> function should prefetch data in the
+indicated range.
+
+If the cache operation fails, your function should throw an exception,
+optionally using C<nbdkit.set_error> first.
+
 =back
 
 =head2 Missing callbacks
@@ -321,7 +334,6 @@ C<can_zero>,
 C<can_fast_zero>,
 C<can_extents>,
 C<can_multi_conn>,
-C<cache>,
 C<extents>.
 
 These are not yet supported.
diff --git a/plugins/python/python.c b/plugins/python/python.c
index 9445343..6d5a0b7 100644
--- a/plugins/python/python.c
+++ b/plugins/python/python.c
@@ -683,6 +683,39 @@ py_zero (void *handle, uint32_t count, uint64_t offset,
uint32_t flags)
   return -1;
 }
 
+static int
+py_cache (void *handle, uint32_t count, uint64_t offset, uint32_t flags)
+{
+  PyObject *obj = handle;
+  PyObject *fn;
+  PyObject *r;
+
+  if (callback_defined ("cache", &fn)) {
+    PyErr_Clear ();
+
+    switch (py_api_version) {
+    case 1:
+      nbdkit_error ("%s can only be called when using api_version >=
2",
+                    "cache");
+      return -1;
+    case 2:
+      r = PyObject_CallFunction (fn, "OiLI", obj, count, offset,
flags, NULL);
+      break;
+    default: abort ();
+    }
+    Py_DECREF (fn);
+    if (check_python_failure ("cache") == -1)
+      return -1;
+    Py_DECREF (r);
+  }
+  else {
+    nbdkit_error ("%s not implemented", "cache");
+    return -1;
+  }
+
+  return 0;
+}
+
 static int
 boolean_callback (void *handle, const char *can_fn, const char *plain_fn)
 {
@@ -768,6 +801,7 @@ static struct nbdkit_plugin plugin = {
   .flush             = py_flush,
   .trim              = py_trim,
   .zero              = py_zero,
+  .cache             = py_cache,
 };
 
 NBDKIT_REGISTER_PLUGIN (plugin)
-- 
2.23.0

---
 plugins/python/nbdkit-python-plugin.pod | 16 ++++++++++++++--
 plugins/python/python.c                 | 14 ++++++++++++++
 2 files changed, 28 insertions(+), 2 deletions(-)

diff --git a/plugins/python/nbdkit-python-plugin.pod
b/plugins/python/nbdkit-python-plugin.pod
index ee58cd7..ab8f5d9 100644
--- a/plugins/python/nbdkit-python-plugin.pod
+++ b/plugins/python/nbdkit-python-plugin.pod
@@ -212,6 +212,20 @@ 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<can_fast_zero>
+
+(Optional)
+
+ def can_fast_zero(h):
+   # return a boolean
+
 =item C<pread>
 
 (Required)
@@ -330,8 +344,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>,
 C<extents>.
diff --git a/plugins/python/python.c b/plugins/python/python.c
index 6d5a0b7..9693180 100644
--- a/plugins/python/python.c
+++ b/plugins/python/python.c
@@ -769,6 +769,18 @@ 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");
+}
+
+static int
+py_can_fast_zero (void *handle)
+{
+  return boolean_callback (handle, "can_fast_zero", NULL);
+}
+
 #define py_config_help \
   "script=<FILENAME>     (required) The Python plugin to
run.\n" \
   "[other arguments may be used by the plugin that you load]"
@@ -795,6 +807,8 @@ 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,
+  .can_fast_zero     = py_can_fast_zero,
 
   .pread             = py_pread,
   .pwrite            = py_pwrite,
-- 
2.23.0

---
 plugins/python/nbdkit-python-plugin.pod | 8 +++++++-
 plugins/python/python.c                 | 7 +++++++
 2 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/plugins/python/nbdkit-python-plugin.pod
b/plugins/python/nbdkit-python-plugin.pod
index ab8f5d9..df503f8 100644
--- a/plugins/python/nbdkit-python-plugin.pod
+++ b/plugins/python/nbdkit-python-plugin.pod
@@ -191,6 +191,13 @@ contents will be garbage collected.
  def is_rotational(h):
    # return a boolean
 
+=item C<can_multi_conn>
+
+(Optional)
+
+ def can_multi_conn(h):
+   # return a boolean
+
 =item C<can_write>
 
 (Optional)
@@ -345,7 +352,6 @@ C<magic_config_key>,
 C<can_fua>,
 C<can_cache>,
 C<can_extents>,
-C<can_multi_conn>,
 C<extents>.
 
 These are not yet supported.
diff --git a/plugins/python/python.c b/plugins/python/python.c
index 9693180..9119b8a 100644
--- a/plugins/python/python.c
+++ b/plugins/python/python.c
@@ -751,6 +751,12 @@ py_is_rotational (void *handle)
   return boolean_callback (handle, "is_rotational", NULL);
 }
 
+static int
+py_can_multi_conn (void *handle)
+{
+  return boolean_callback (handle, "can_multi_conn", NULL);
+}
+
 static int
 py_can_write (void *handle)
 {
@@ -804,6 +810,7 @@ static struct nbdkit_plugin plugin = {
 
   .get_size          = py_get_size,
   .is_rotational     = py_is_rotational,
+  .can_multi_conn    = py_can_multi_conn,
   .can_write         = py_can_write,
   .can_flush         = py_can_flush,
   .can_trim          = py_can_trim,
-- 
2.23.0

---
 plugins/python/nbdkit-python-plugin.pod | 18 +++++++-
 plugins/python/python.c                 | 58 +++++++++++++++++++++++++
 2 files changed, 74 insertions(+), 2 deletions(-)

diff --git a/plugins/python/nbdkit-python-plugin.pod
b/plugins/python/nbdkit-python-plugin.pod
index df503f8..1946329 100644
--- a/plugins/python/nbdkit-python-plugin.pod
+++ b/plugins/python/nbdkit-python-plugin.pod
@@ -233,6 +233,22 @@ contents will be garbage collected.
  def can_fast_zero(h):
    # return a boolean
 
+=item C<can_fua>
+
+(Optional)
+
+ def can_fua(h):
+   # return nbdkit.FUA_NONE or nbdkit.FUA_EMULATE
+   # or nbdkit.FUA_NATIVE
+
+=item C<can_cache>
+
+(Optional)
+
+ def can_cache(h):
+   # return nbdkit.CACHE_NONE or nbdkit.CACHE_EMULATE
+   # or nbdkit.CACHE_NATIVE
+
 =item C<pread>
 
 (Required)
@@ -349,8 +365,6 @@ C<longname>,
 C<description>,
 C<config_help>,
 C<magic_config_key>,
-C<can_fua>,
-C<can_cache>,
 C<can_extents>,
 C<extents>.
 
diff --git a/plugins/python/python.c b/plugins/python/python.c
index 9119b8a..0fa4464 100644
--- a/plugins/python/python.c
+++ b/plugins/python/python.c
@@ -787,6 +787,62 @@ py_can_fast_zero (void *handle)
   return boolean_callback (handle, "can_fast_zero", NULL);
 }
 
+static int
+py_can_fua (void *handle)
+{
+  PyObject *obj = handle;
+  PyObject *fn;
+  PyObject *r;
+  int ret;
+
+  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;
+    ret = PyLong_AsLong (r);
+    Py_DECREF (r);
+    return ret;
+  }
+  /* No Python ‘can_fua’, but check if there's a Python ‘flush’
+   * callback defined.  (In C modules, nbdkit would do this).
+   */
+  else if (callback_defined ("flush", NULL))
+    return NBDKIT_FUA_EMULATE;
+  else
+    return NBDKIT_FUA_NONE;
+}
+
+static int
+py_can_cache (void *handle)
+{
+  PyObject *obj = handle;
+  PyObject *fn;
+  PyObject *r;
+  int ret;
+
+  if (callback_defined ("can_cache", &fn)) {
+    PyErr_Clear ();
+
+    r = PyObject_CallFunctionObjArgs (fn, obj, NULL);
+    Py_DECREF (fn);
+    if (check_python_failure ("can_cache") == -1)
+      return -1;
+    ret = PyLong_AsLong (r);
+    Py_DECREF (r);
+    return ret;
+  }
+  /* No Python ‘can_cache’, but check if there's a Python ‘cache’
+   * callback defined.  (In C modules, nbdkit would do this).
+   */
+  else if (callback_defined ("cache", NULL))
+    return NBDKIT_CACHE_NATIVE;
+  else
+    return NBDKIT_CACHE_NONE;
+}
+
 #define py_config_help \
   "script=<FILENAME>     (required) The Python plugin to
run.\n" \
   "[other arguments may be used by the plugin that you load]"
@@ -816,6 +872,8 @@ static struct nbdkit_plugin plugin = {
   .can_trim          = py_can_trim,
   .can_zero          = py_can_zero,
   .can_fast_zero     = py_can_fast_zero,
+  .can_fua           = py_can_fua,
+  .can_cache         = py_can_cache,
 
   .pread             = py_pread,
   .pwrite            = py_pwrite,
-- 
2.23.0

This tests the Python plugin thoroughly by issuing client commands
through libnbd and checking we get the expected results.
---
 tests/Makefile.am           |  13 +--
 tests/test-python-plugin.py | 134 ++++++++++++++++++++++++++++
 tests/test-python.py        | 172 ++++++++++++++++++++++++++++++++++++
 tests/test.py               |  60 -------------
 4 files changed, 309 insertions(+), 70 deletions(-)

diff --git a/tests/Makefile.am b/tests/Makefile.am
index 13cd8f3..88849f5 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -115,7 +115,8 @@ EXTRA_DIST = \
 	test-pattern-largest-for-qemu.sh \
 	test-python-exception.sh \
 	test.pl \
-	test.py \
+	test-python.py \
+	test-python-plugin.py \
 	test-rate.sh \
 	test-rate-dynamic.sh \
 	test.rb \
@@ -787,18 +788,10 @@ endif HAVE_PERL
 if HAVE_PYTHON
 
 TESTS += \
+	test-python.py \
 	test-python-exception.sh \
 	test-shebang-python.sh \
 	$(NULL)
-LIBGUESTFS_TESTS += test-python
-
-test_python_SOURCES = test-lang-plugins.c test.h
-test_python_CFLAGS = \
-	-DLANG='"python"'
-DSCRIPT='"$(srcdir)/test.py"' \
-	$(WARNINGS_CFLAGS) \
-	$(LIBGUESTFS_CFLAGS) \
-	$(NULL)
-test_python_LDADD = libtest.la $(LIBGUESTFS_LIBS)
 
 endif HAVE_PYTHON
 
diff --git a/tests/test-python-plugin.py b/tests/test-python-plugin.py
new file mode 100644
index 0000000..053a380
--- /dev/null
+++ b/tests/test-python-plugin.py
@@ -0,0 +1,134 @@
+#!/usr/bin/env python3
+# nbdkit
+# Copyright (C) 2019 Red Hat Inc.
+#
+# Redistribution and use in source and binary forms, with or without
+# modification, are permitted provided that the following conditions are
+# met:
+#
+# * Redistributions of source code must retain the above copyright
+# notice, this list of conditions and the following disclaimer.
+#
+# * Redistributions in binary form must reproduce the above copyright
+# notice, this list of conditions and the following disclaimer in the
+# documentation and/or other materials provided with the distribution.
+#
+# * Neither the name of Red Hat nor the names of its contributors may be
+# used to endorse or promote products derived from this software without
+# specific prior written permission.
+#
+# THIS SOFTWARE IS PROVIDED BY RED HAT AND CONTRIBUTORS ''AS
IS'' AND
+# ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO,
+# THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A
+# PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL RED HAT OR
+# CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+# SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+# LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF
+# USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND
+# ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY,
+# OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT
+# OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
+# SUCH DAMAGE.
+
+# See test-python.py.
+
+import nbdkit
+import sys
+import pickle
+import codecs
+
+def api_version ():
+    return 2
+
+cfg = {}
+
+def config (k, v):
+    global cfg
+    if k == "cfg":
+        cfg = pickle.loads (codecs.decode (v.encode(), "base64"))
+
+def config_complete ():
+    print ("set_error = %r" % nbdkit.set_error)
+
+def open (readonly):
+    return {
+        'disk': bytearray (cfg.get ('size', 0))
+    }
+
+def get_size (h):
+    return len (h['disk'])
+
+def is_rotational(h):
+    return cfg.get ('is_rotational', False)
+
+def can_multi_conn (h):
+    return cfg.get ('can_multi_conn', False)
+
+def can_write (h):
+    return cfg.get ('can_write', True)
+
+def can_flush(h):
+    return cfg.get ('can_flush', False)
+
+def can_trim(h):
+    return cfg.get ('can_trim', False)
+
+def can_zero(h):
+    return cfg.get ('can_zero', False)
+
+def can_fast_zero(h):
+    return cfg.get ('can_fast_zero', False)
+
+def can_fua(h):
+    fua = cfg.get ('can_fua', "none")
+    if fua == "none":
+        return nbdkit.FUA_NONE
+    elif fua == "emulate":
+        return nbdkit.FUA_EMULATE
+    elif fua == "native":
+        return nbdkit.FUA_NATIVE
+
+def can_cache(h):
+    cache = cfg.get ('can_cache', "none")
+    if cache == "none":
+        return nbdkit.CACHE_NONE
+    elif cache == "emulate":
+        return nbdkit.CACHE_EMULATE
+    elif cache == "native":
+        return nbdkit.CACHE_NATIVE
+
+def pread(h, count, offset, flags):
+    assert flags == 0
+    return h['disk'][offset:offset+count]
+
+def pwrite(h, buf, offset, flags):
+    expect_fua = cfg.get ('pwrite_expect_fua', False)
+    actual_fua = bool (flags & nbdkit.FLAG_FUA)
+    assert expect_fua == actual_fua
+    end = offset + len(buf)
+    h['disk'][offset:end] = buf
+
+def flush(h, flags):
+    assert flags == 0
+
+def trim(h, count, offset, flags):
+    expect_fua = cfg.get ('trim_expect_fua', False)
+    actual_fua = bool (flags & nbdkit.FLAG_FUA)
+    assert expect_fua == actual_fua
+    h['disk'][offset:offset+count] = bytearray(count)
+
+def zero(h, count, offset, flags):
+    expect_fua = cfg.get ('zero_expect_fua', False)
+    actual_fua = bool (flags & nbdkit.FLAG_FUA)
+    assert expect_fua == actual_fua
+    expect_may_trim = cfg.get ('zero_expect_may_trim', False)
+    actual_may_trim = bool (flags & nbdkit.FLAG_MAY_TRIM)
+    assert expect_may_trim == actual_may_trim
+    expect_fast_zero = cfg.get ('zero_expect_fast_zero', False)
+    actual_fast_zero = bool (flags & nbdkit.FLAG_FAST_ZERO)
+    assert expect_fast_zero == actual_fast_zero
+    h['disk'][offset:offset+count] = bytearray(count)
+
+def cache(h, count, offset, flags):
+    assert flags == 0
+    # do nothing
diff --git a/tests/test-python.py b/tests/test-python.py
new file mode 100755
index 0000000..2f565ba
--- /dev/null
+++ b/tests/test-python.py
@@ -0,0 +1,172 @@
+#!/usr/bin/env python3
+# nbdkit
+# Copyright (C) 2019 Red Hat Inc.
+#
+# Redistribution and use in source and binary forms, with or without
+# modification, are permitted provided that the following conditions are
+# met:
+#
+# * Redistributions of source code must retain the above copyright
+# notice, this list of conditions and the following disclaimer.
+#
+# * Redistributions in binary form must reproduce the above copyright
+# notice, this list of conditions and the following disclaimer in the
+# documentation and/or other materials provided with the distribution.
+#
+# * Neither the name of Red Hat nor the names of its contributors may be
+# used to endorse or promote products derived from this software without
+# specific prior written permission.
+#
+# THIS SOFTWARE IS PROVIDED BY RED HAT AND CONTRIBUTORS ''AS
IS'' AND
+# ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO,
+# THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A
+# PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL RED HAT OR
+# CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+# SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+# LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF
+# USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND
+# ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY,
+# OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT
+# OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
+# SUCH DAMAGE.
+
+# This tests the Python plugin thoroughly by issuing client commands
+# through libnbd and checking we get the expected results.  It uses an
+# associated plugin (test-python-plugin.sh).
+
+import os
+import sys
+import pickle
+import codecs
+
+# Python has proven very difficult to valgrind, therefore it is disabled.
+if "NBDKIT_VALGRIND" in os.environ:
+    print ("$s: skipping Python test under valgrind" % __file__)
+    sys.exit (77)
+
+try:
+    import nbd
+except:
+    print ("%s: skipped because cannot import python libnbd" %
__file__)
+    sys.exit (77)
+
+try:
+    srcdir = os.environ['SRCDIR']
+except:
+    print ("%s: FAIL: test failed because $SRCDIR is not set" %
__file__)
+    sys.exit (1)
+
+def test (cfg):
+    h = nbd.NBD ()
+    cfg = codecs.encode (pickle.dumps (cfg), "base64").decode()
+    cmd = ["nbdkit", "-v", "-s",
"--exit-with-parent",
+           "python", srcdir + "/test-python-plugin.py",
+           "cfg=" + cfg]
+    h.connect_command (cmd)
+    return h
+
+# Test we can send an empty pickled test configuration and do nothing
+# else.  This is just to ensure the machinery of the test works.
+h = test ({})
+
+# Test the size.
+h = test ({"size": 512})
+assert h.get_size() == 512
+h = test ({"size": 1024*1024})
+assert h.get_size() == 1024*1024
+
+# Test each flag call.
+h = test ({"size": 512, "is_rotational": True})
+assert h.is_rotational()
+h = test ({"size": 512, "is_rotational": False})
+assert not h.is_rotational()
+
+h = test ({"size": 512, "can_multi_conn": True})
+assert h.can_multi_conn()
+h = test ({"size": 512, "can_multi_conn": False})
+assert not h.can_multi_conn()
+
+h = test ({"size": 512, "can_write": True})
+assert not h.is_read_only()
+h = test ({"size": 512, "can_write": False})
+assert h.is_read_only()
+
+h = test ({"size": 512, "can_flush": True})
+assert h.can_flush()
+h = test ({"size": 512, "can_flush": False})
+assert not h.can_flush()
+
+h = test ({"size": 512, "can_trim": True})
+assert h.can_trim()
+h = test ({"size": 512, "can_trim": False})
+assert not h.can_trim()
+
+# nbdkit can always zero because it emulates it.
+#h = test ({"size": 512, "can_zero": True})
+#assert h.can_zero()
+#h = test ({"size": 512, "can_zero": False})
+#assert not h.can_zero()
+
+h = test ({"size": 512, "can_fast_zero": True})
+assert h.can_fast_zero()
+h = test ({"size": 512, "can_fast_zero": False})
+assert not h.can_fast_zero()
+
+h = test ({"size": 512, "can_fua": "none"})
+assert not h.can_fua()
+h = test ({"size": 512, "can_fua": "emulate"})
+assert h.can_fua()
+h = test ({"size": 512, "can_fua": "native"})
+assert h.can_fua()
+
+h = test ({"size": 512, "can_cache": "none"})
+assert not h.can_cache()
+h = test ({"size": 512, "can_cache": "emulate"})
+assert h.can_cache()
+h = test ({"size": 512, "can_cache": "native"})
+assert h.can_cache()
+
+# Not yet implemented: can_extents.
+
+# Test pread.
+h = test ({"size": 512})
+buf = h.pread (512, 0)
+
+# Test pwrite + flags.
+h = test ({"size": 512})
+h.pwrite (buf, 0)
+
+h = test ({"size": 512, "can_fua": "native",
"pwrite_expect_fua": True})
+h.pwrite (buf, 0, nbd.CMD_FLAG_FUA)
+
+# Test flush.
+h = test ({"size": 512, "can_flush": True})
+h.flush ()
+
+# Test trim + flags.
+h = test ({"size": 512, "can_trim": True})
+h.trim (512, 0)
+
+h = test ({"size": 512,
+           "can_trim": True, "can_fua": "native",
"trim_expect_fua": True})
+h.trim (512, 0, nbd.CMD_FLAG_FUA)
+
+# Test zero + flags.
+h = test ({"size": 512, "can_zero": True})
+h.zero (512, 0, nbd.CMD_FLAG_NO_HOLE)
+
+h = test ({"size": 512,
+           "can_zero": True, "can_fua": "native",
"zero_expect_fua": True})
+h.zero (512, 0, nbd.CMD_FLAG_NO_HOLE | nbd.CMD_FLAG_FUA)
+
+h = test ({"size": 512, "can_zero": True,
"zero_expect_may_trim": True})
+h.zero (512, 0, 0) # absence of nbd.CMD_FLAG_NO_HOLE
+
+h = test ({"size": 512,
+           "can_zero": True, "can_fast_zero": True,
+           "zero_expect_fast_zero": True})
+h.zero (512, 0, nbd.CMD_FLAG_NO_HOLE | nbd.CMD_FLAG_FAST_ZERO)
+
+# Test cache.
+h = test ({"size": 512, "can_cache": "native"})
+h.cache (512, 0)
diff --git a/tests/test.py b/tests/test.py
deleted file mode 100644
index ac80d96..0000000
--- a/tests/test.py
+++ /dev/null
@@ -1,60 +0,0 @@
-import nbdkit
-
-disk = bytearray(1024*1024)
-
-
-def api_version():
-    return 2
-
-
-def config_complete():
-    print ("set_error = %r" % nbdkit.set_error)
-
-
-def open(readonly):
-    return 1
-
-
-def get_size(h):
-    global disk
-    return len(disk)
-
-
-def can_write(h):
-    return True
-
-
-def can_flush(h):
-    return True
-
-
-def is_rotational(h):
-    return False
-
-
-def can_trim(h):
-    return True
-
-
-def pread(h, count, offset, flags):
-    global disk
-    return disk[offset:offset+count]
-
-
-def pwrite(h, buf, offset, flags):
-    global disk
-    end = offset + len(buf)
-    disk[offset:end] = buf
-
-
-def flush(h, flags):
-    pass
-
-
-def trim(h, count, offset, flags):
-    pass
-
-
-def zero(h, count, offset, flags):
-    global disk
-    disk[offset:offset+count] = bytearray(count)
-- 
2.23.0

On Fri, Nov 22, 2019 at 9:54 PM Richard W.M. Jones <rjones@redhat.com>
wrote:
>
> These are accessible from the plugin by:
>
>   import nbdkit
>
>   if flags & nbdkit.FLAG_MAY_TRIM:
>   &c.
Nice way to expose the flags!
> Many (all?) of these are not yet useful for plugins, some will never
> be useful, but they only consume a tiny bit of memory and it's nice to
> have the complete set available for future use.
> ---
>  plugins/python/python.c | 27 +++++++++++++++++++++++++++
>  1 file changed, 27 insertions(+)
>
> diff --git a/plugins/python/python.c b/plugins/python/python.c
> index d65ac45..69cf4e9 100644
> --- a/plugins/python/python.c
> +++ b/plugins/python/python.c
> @@ -231,6 +231,33 @@ create_nbdkit_module (void)
>      nbdkit_error ("could not create the nbdkit API module");
>      exit (EXIT_FAILURE);
>    }
> +
> +  /* Constants corresponding to various flags. */
> +  PyModule_AddIntConstant (m,
"THREAD_MODEL_SERIALIZE_CONNECTIONS",
> +                           NBDKIT_THREAD_MODEL_SERIALIZE_CONNECTIONS);
This can fail and needs cleanup. Very unlikely and a lot of code in
the standard library have
this issue.

In sanlock we do:

    if (PyModule_AddIntConstant(m, "LSFLAG_ADD", SANLK_LSF_ADD))
        return -1;

And the caller decref the module object and return NULL.

See
https://github.com/nirs/sanlock/blob/53f7f0284c084ac2e4542fd1f71d0406075adb5d/python/sanlock.c#L1846
https://github.com/nirs/sanlock/blob/53f7f0284c084ac2e4542fd1f71d0406075adb5d/python/sanlock.c#L1763
> +  PyModule_AddIntConstant (m,
"THREAD_MODEL_SERIALIZE_ALL_REQUESTS",
> +                           NBDKIT_THREAD_MODEL_SERIALIZE_ALL_REQUESTS);
> +  PyModule_AddIntConstant (m, "THREAD_MODEL_SERIALIZE_REQUESTS",
> +                           NBDKIT_THREAD_MODEL_SERIALIZE_REQUESTS);
> +  PyModule_AddIntConstant (m, "THREAD_MODEL_PARALLEL",
> +                           NBDKIT_THREAD_MODEL_PARALLEL);
> +
> +  PyModule_AddIntConstant (m, "FLAG_MAY_TRIM", 
NBDKIT_FLAG_MAY_TRIM);
> +  PyModule_AddIntConstant (m, "FLAG_FUA",      
NBDKIT_FLAG_FUA);
> +  PyModule_AddIntConstant (m, "FLAG_REQ_ONE",  
NBDKIT_FLAG_REQ_ONE);
> +  PyModule_AddIntConstant (m, "FLAG_FAST_ZERO",
NBDKIT_FLAG_FAST_ZERO);
> +
> +  PyModule_AddIntConstant (m, "FUA_NONE",      
NBDKIT_FUA_NONE);
> +  PyModule_AddIntConstant (m, "FUA_EMULATE",   
NBDKIT_FUA_EMULATE);
> +  PyModule_AddIntConstant (m, "FUA_NATIVE",    
NBDKIT_FUA_NATIVE);
> +
> +  PyModule_AddIntConstant (m, "CACHE_NONE",    
NBDKIT_CACHE_NONE);
> +  PyModule_AddIntConstant (m, "CACHE_EMULATE", 
NBDKIT_CACHE_EMULATE);
> +  PyModule_AddIntConstant (m, "CACHE_NATIVE",  
NBDKIT_CACHE_NATIVE);
> +
> +  PyModule_AddIntConstant (m, "EXTENT_HOLE",   
NBDKIT_EXTENT_HOLE);
> +  PyModule_AddIntConstant (m, "EXTENT_ZERO",   
NBDKIT_EXTENT_ZERO);
> +
>    return m;
>  }
>
> --
> 2.23.0
>

On 11/22/19 1:53 PM, Richard W.M. Jones wrote:
> It is unclear why we were constructing this by hand, but using the
> following tip we can use PyObject_CallFunction:
> https://stackoverflow.com/a/21221335
> ---
>   plugins/python/python.c | 17 ++++++-----------
>   1 file changed, 6 insertions(+), 11 deletions(-)
> 
>       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);
Our other two uses of PyObject_CallObject pass NULL args, so they are okay.
> +    r = PyObject_CallFunction (fn, "OiLO",
> +                               obj, count, offset,
> +                               may_trim ? Py_True : Py_False, NULL);
ACK.  And definitely easier to read :)
>       Py_DECREF (fn);
> -    Py_DECREF (args);
>       if (last_error == EOPNOTSUPP || last_error == ENOTSUP) {
>         /* When user requests this particular error, we want to
> -         gracefully fall back, and to accomodate both a normal return
> -         and an exception. */
> +       * gracefully fall back, and to accomodate both a normal return
While we're at it, s/accomodate/accommodate/
> +       * and an exception.
> +       */
>         nbdkit_debug ("zero requested falling back to pwrite");
>         Py_XDECREF (r);
>         PyErr_Clear ();
> 
-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

On 11/22/19 1:53 PM, Richard W.M. Jones wrote:
> To avoid breaking existing plugins, Python plugins wishing to use
> version 2 of the API must opt in by declaring:
> 
>    def api_version():
>      return 2
> 
> (Plugins which do not do this are assumed to want API version 1).
Could we also permit the python code to declare a global variable 
instead of a function?  But a function is just fine (and easier to 
integrate the way we do all our other callbacks).

> +++ b/plugins/python/example.py
> @@ -34,6 +34,13 @@ import errno
>   disk = bytearray(1024 * 1024)
>   
>   
> +# There are several variants of the API.  nbdkit will call this
> +# function first to determine which one you want to use.  This is the
> +# latest version at the time this example was written.
> +def api_version():
> +    return 2
Matches the C counterpart of #define NBDKIT_API_VERSION 2 at the top of 
a plugin.
> +
> +
>   # This just prints the extra command line parameters, but real plugins
>   # should parse them and reject any unknown parameters.
>   def config(key, value):
> @@ -54,20 +61,20 @@ def get_size(h):
>       return len(disk)
>   
>   
> -def pread(h, count, offset):
> +def pread(h, count, offset, flags):
>       global disk
>       return disk[offset:offset+count]
Do we really want to be passing 'flags' as an integer that the python 
script must decode?  Could we instead pass the flags as named kwargs? 
For pread, there are no defined flags, so that would mean we stick with

def pread(h, count, offset):
>   
>   
> -def pwrite(h, buf, offset):
> +def pwrite(h, buf, offset, flags):
but for pwrite, it would look like:

def pwrite(h, buf, offset, fua=False):
>   
> -def zero(h, count, offset, may_trim):
> +def zero(h, count, offset, flags):
and for zero (once fast zero is supported later in the series), it could 
look like:

def zero(h, count, offset, may_trim=False, fua=False, fast=False):

The user could also write:

def zero(h, count, offset, **flags)

and manually extract key/value pairs out of the flags, to be robust to 
unknown flags (although our API somewhat promises that we never pass 
flags to the data-handling callbacks unless the can_FOO callbacks 
already indicated that the plugin was willing to support the flag)

> +++ b/plugins/python/nbdkit-python-plugin.pod
> @@ -82,6 +82,19 @@ I<--dump-plugin> option, eg:
>    python_version=3.7.0
>    python_pep_384_abi_version=3
>   
> +=head2 API versions
> +
> +The nbdkit API has evolved and new versions are released periodically.
> +To ensure backwards compatibility plugins have to opt in to the new
> +version.  From Python you do this by declaring a function in your
> +module:
> +
> + def api_version():
> +   return 2
> +
> +(where 2 is the latest version at the time this documentation was
> +written).  All newly written Python modules must have this function.
> +
>   =head2 Executable script
>   
>   If you want you can make the script executable and include a
"shebang"
> @@ -120,6 +133,10 @@ nbdkit-plugin(3).
Unrelated side topic: in your recent addition of eval.sh, you wondered 
if we should promote it to a full-blown plugin rather than just an 
example script.  But reading 'man nbdkit-sh-plugin', there is no mention
of turning an executable script into a full-blown plugin via a shebang, 
the way that python documents it.  [I guess 'man nbdkit' sort of 
mentions it under Shebang scripts]

> - def pwrite(h, buf, offset):
> + def pwrite(h, buf, offset, flags):
>      length = len (buf)
>      # no return value
>   
>   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>.
> +C<offset>.  C<flags> may contain C<nbdkit.FLAG_FUA>.
Should we mention that FLAG_FUA is only set if the python callback 
can_fua returned 1?  Or is that somewhat redundant with the fact that we 
already document that someone writing a python plugin should be familiar 
with the docs for C plugins, and that guarantee is already in the C 
plugin docs?
> - def zero(h, count, offset, may_trim):
> + def zero(h, count, offset, flags):
>      # 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.
> +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. 
C<flags> is
> +a bitmask which may include C<nbdkit.FLAG_MAY_TRIM>,
> +C<nbdkit.FLAG_FUA>, C<nbdkit.FLAG_FAST_ZERO>.
Well, technically FAST_ZERO can't be set until later in the series when 
you plumb in the can_fast_zero callback... :)

>   static int
> -py_pwrite (void *handle, const void *buf,
> -           uint32_t count, uint64_t offset)
> +py_pwrite (void *handle, const void *buf, uint32_t count, uint64_t offset,
> +           uint32_t flags)
>   {
>     PyObject *obj = handle;
>     PyObject *fn;
> @@ -515,9 +546,19 @@ py_pwrite (void *handle, const void *buf,
>     if (callback_defined ("pwrite", &fn)) {
>       PyErr_Clear ();
>   
> -    r = PyObject_CallFunction (fn, "ONL", obj,
> -                               PyByteArray_FromStringAndSize (buf, count),
> -                               offset, NULL);
> +    switch (py_api_version) {
> +    case 1:
> +      r = PyObject_CallFunction (fn, "ONL", obj,
> +                                 PyByteArray_FromStringAndSize (buf,
count),
> +                                 offset, NULL);
Here, we could assert (flags == 0) (the FUA flag should not be set if 
the plugin uses v1 API).

> @@ -565,7 +614,15 @@ py_trim (void *handle, uint32_t count, uint64_t
offset)
>     if (callback_defined ("trim", &fn)) {
>       PyErr_Clear ();
>   
> -    r = PyObject_CallFunction (fn, "OiL", obj, count, offset,
NULL);
> +    switch (py_api_version) {
> +    case 1:
> +      r = PyObject_CallFunction (fn, "OiL", obj, count, offset,
NULL);
> +      break;
Again, we may want to assert that flags does not include FUA here.

>   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;
> @@ -590,9 +647,20 @@ py_zero (void *handle, uint32_t count, uint64_t
offset, int may_trim)
>       PyErr_Clear ();
>   
>       last_error = 0;
> -    r = PyObject_CallFunction (fn, "OiLO",
> -                               obj, count, offset,
> -                               may_trim ? Py_True : Py_False, NULL);
> +    switch (py_api_version) {
> +    case 1: {
> +      int may_trim = flags & NBDKIT_FLAG_MAY_TRIM;
and maybe assert that no other flags are set.

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

On 11/22/19 1:53 PM, Richard W.M. Jones wrote:
> ---
>   plugins/python/nbdkit-python-plugin.pod | 21 +++++++++++++++++++--
>   1 file changed, 19 insertions(+), 2 deletions(-)
ACK
> 
> diff --git a/plugins/python/nbdkit-python-plugin.pod
b/plugins/python/nbdkit-python-plugin.pod
> index 882c0d8..51e0f57 100644
> --- a/plugins/python/nbdkit-python-plugin.pod
> +++ b/plugins/python/nbdkit-python-plugin.pod
> @@ -304,8 +304,25 @@ S<C<nbdkit.set_error (errno.EOPNOTSUPP)>>.
>   These are not needed because you can just use ordinary Python
>   constructs.
>   
> -=item Missing: C<name>, C<version>, C<longname>,
C<description>,
> -C<config_help>, C<can_fua>, C<can_cache>, C<cache>
> +=item Missing: C<thread_model>
> +
> +Probably not implementable while Python has a global interpreter lock.
> +
> +=item Missing:
> +C<name>,
> +C<version>,
> +C<longname>,
> +C<description>,
> +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>,
> +C<cache>,
> +C<extents>.
>   
>   These are not yet supported.
>   
> 
-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

On 11/22/19 1:54 PM, Richard W.M. Jones wrote:
> This change is pure refactoring and should have no effect.
> ---
>   plugins/python/nbdkit-python-plugin.pod | 12 ++--
>   plugins/python/python.c                 | 90 +++++--------------------
>   2 files changed, 24 insertions(+), 78 deletions(-)
> 
> diff --git a/plugins/python/nbdkit-python-plugin.pod
b/plugins/python/nbdkit-python-plugin.pod
> index 51e0f57..0fd4dcb 100644
> --- a/plugins/python/nbdkit-python-plugin.pod
> +++ b/plugins/python/nbdkit-python-plugin.pod
> @@ -184,25 +184,25 @@ contents will be garbage collected.
>    def get_size(h):
>      # return the size of the disk
>   
> -=item C<can_write>
> +=item C<is_rotational>
>   
>   (Optional)
>   
> - def can_write(h):
> + def is_rotational(h):
>      # return a boolean
>   
> -=item C<can_flush>
> +=item C<can_write>
>   
>   (Optional)
>   
> - def can_flush(h):
> + def can_write(h):
>      # return a boolean
>   
> -=item C<is_rotational>
> +=item C<can_flush>
>   
>   (Optional)
>   
> - def is_rotational(h):
> + def can_flush(h):
>      # return a boolean
Why the shuffle? To match ordering in other docs?  But not the end of 
the world.

Otherwise, nice reduction in lines of code (it may get trickier with 
can_FOO that return tristate, but for this patch you really did simplify 
true bool/error return functions).
> @@ -812,9 +758,9 @@ static struct nbdkit_plugin plugin = {
>     .close             = py_close,
>   
>     .get_size          = py_get_size,
> +  .is_rotational     = py_is_rotational,
>     .can_write         = py_can_write,
>     .can_flush         = py_can_flush,
> -  .is_rotational     = py_is_rotational,
>     .can_trim          = py_can_trim,
>   
>     .pread             = py_pread,
> 
-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

On 11/22/19 1:54 PM, Richard W.M. Jones wrote:
> However this does not implement can_cache, since that is not a simple
> boolean.
> ---
>   plugins/python/nbdkit-python-plugin.pod | 14 +++++++++-
>   plugins/python/python.c                 | 34 +++++++++++++++++++++++++
>   2 files changed, 47 insertions(+), 1 deletion(-)
> 
> +static int
> +py_cache (void *handle, uint32_t count, uint64_t offset, uint32_t flags)
> +{
> +  PyObject *obj = handle;
> +  PyObject *fn;
> +  PyObject *r;
> +
> +  if (callback_defined ("cache", &fn)) {
> +    PyErr_Clear ();
> +
> +    switch (py_api_version) {
> +    case 1:
> +      nbdkit_error ("%s can only be called when using api_version
>= 2",
> +                    "cache");
> +      return -1;
Why? The signature doesn't change.  Yes, it's unusual to write an API 
version 1 C plugin that provides a .cache callback, but it is not 
impossible.  Instead of erroring out, we could just always support this 
function in both versions of Python plugin.

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

On Fri, Nov 22, 2019 at 9:55 PM Richard W.M. Jones <rjones@redhat.com>
wrote:
>
> This tests the Python plugin thoroughly by issuing client commands
> through libnbd and checking we get the expected results.
> ---
>  tests/Makefile.am           |  13 +--
>  tests/test-python-plugin.py | 134 ++++++++++++++++++++++++++++
>  tests/test-python.py        | 172 ++++++++++++++++++++++++++++++++++++
>  tests/test.py               |  60 -------------
>  4 files changed, 309 insertions(+), 70 deletions(-)
>
> diff --git a/tests/Makefile.am b/tests/Makefile.am
> index 13cd8f3..88849f5 100644
> --- a/tests/Makefile.am
> +++ b/tests/Makefile.am
> @@ -115,7 +115,8 @@ EXTRA_DIST = \
>         test-pattern-largest-for-qemu.sh \
>         test-python-exception.sh \
>         test.pl \
> -       test.py \
> +       test-python.py \
> +       test-python-plugin.py \
>         test-rate.sh \
>         test-rate-dynamic.sh \
>         test.rb \
> @@ -787,18 +788,10 @@ endif HAVE_PERL
>  if HAVE_PYTHON
>
>  TESTS += \
> +       test-python.py \
>         test-python-exception.sh \
>         test-shebang-python.sh \
>         $(NULL)
> -LIBGUESTFS_TESTS += test-python
> -
> -test_python_SOURCES = test-lang-plugins.c test.h
> -test_python_CFLAGS = \
> -       -DLANG='"python"'
-DSCRIPT='"$(srcdir)/test.py"' \
> -       $(WARNINGS_CFLAGS) \
> -       $(LIBGUESTFS_CFLAGS) \
> -       $(NULL)
> -test_python_LDADD = libtest.la $(LIBGUESTFS_LIBS)
>
>  endif HAVE_PYTHON
>
> diff --git a/tests/test-python-plugin.py b/tests/test-python-plugin.py
> new file mode 100644
> index 0000000..053a380
> --- /dev/null
> +++ b/tests/test-python-plugin.py
> @@ -0,0 +1,134 @@
> +#!/usr/bin/env python3
> +# nbdkit
> +# Copyright (C) 2019 Red Hat Inc.
> +#
> +# Redistribution and use in source and binary forms, with or without
> +# modification, are permitted provided that the following conditions are
> +# met:
> +#
> +# * Redistributions of source code must retain the above copyright
> +# notice, this list of conditions and the following disclaimer.
> +#
> +# * Redistributions in binary form must reproduce the above copyright
> +# notice, this list of conditions and the following disclaimer in the
> +# documentation and/or other materials provided with the distribution.
> +#
> +# * Neither the name of Red Hat nor the names of its contributors may be
> +# used to endorse or promote products derived from this software without
> +# specific prior written permission.
> +#
> +# THIS SOFTWARE IS PROVIDED BY RED HAT AND CONTRIBUTORS ''AS
IS'' AND
> +# ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO,
> +# THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A
> +# PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL RED HAT OR
> +# CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
> +# SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
> +# LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF
> +# USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND
> +# ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY,
> +# OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT
> +# OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
> +# SUCH DAMAGE.
> +
> +# See test-python.py.
> +
> +import nbdkit
> +import sys
> +import pickle
> +import codecs
> +
> +def api_version ():
> +    return 2
> +
> +cfg = {}
> +
> +def config (k, v):
> +    global cfg
> +    if k == "cfg":
> +        cfg = pickle.loads (codecs.decode (v.encode(),
"base64"))
> +
> +def config_complete ():
> +    print ("set_error = %r" % nbdkit.set_error)
> +
> +def open (readonly):
> +    return {
> +        'disk': bytearray (cfg.get ('size', 0))
> +    }
> +
> +def get_size (h):
> +    return len (h['disk'])
> +
> +def is_rotational(h):
> +    return cfg.get ('is_rotational', False)
> +
> +def can_multi_conn (h):
> +    return cfg.get ('can_multi_conn', False)
> +
> +def can_write (h):
> +    return cfg.get ('can_write', True)
> +
> +def can_flush(h):
> +    return cfg.get ('can_flush', False)
> +
> +def can_trim(h):
> +    return cfg.get ('can_trim', False)
> +
> +def can_zero(h):
> +    return cfg.get ('can_zero', False)
> +
> +def can_fast_zero(h):
> +    return cfg.get ('can_fast_zero', False)
> +
> +def can_fua(h):
> +    fua = cfg.get ('can_fua', "none")
> +    if fua == "none":
> +        return nbdkit.FUA_NONE
> +    elif fua == "emulate":
> +        return nbdkit.FUA_EMULATE
> +    elif fua == "native":
> +        return nbdkit.FUA_NATIVE
> +
> +def can_cache(h):
> +    cache = cfg.get ('can_cache', "none")
> +    if cache == "none":
> +        return nbdkit.CACHE_NONE
> +    elif cache == "emulate":
> +        return nbdkit.CACHE_EMULATE
> +    elif cache == "native":
> +        return nbdkit.CACHE_NATIVE
> +
> +def pread(h, count, offset, flags):
> +    assert flags == 0
> +    return h['disk'][offset:offset+count]
Very nice and simple test plugin!

But this returns always a bytearray, which is also what nbdkit python plugin
expects. But real code using HTTPConnection return bytes:
>>> c = http.client.HTTPSConnection("www.python.org")
>>> c.request("GET", "/")
>>> r = c.getresponse()
>>> r.read()[:10]
b'<!doctype '

I think the plugin should support both bytearray, memoryview, or
bytes. Supporting objects
implementing the buffer protocol would be best.
> +
> +def pwrite(h, buf, offset, flags):
> +    expect_fua = cfg.get ('pwrite_expect_fua', False)
> +    actual_fua = bool (flags & nbdkit.FLAG_FUA)
> +    assert expect_fua == actual_fua
> +    end = offset + len(buf)
> +    h['disk'][offset:end] = buf
> +
> +def flush(h, flags):
> +    assert flags == 0
> +
> +def trim(h, count, offset, flags):
> +    expect_fua = cfg.get ('trim_expect_fua', False)
> +    actual_fua = bool (flags & nbdkit.FLAG_FUA)
> +    assert expect_fua == actual_fua
> +    h['disk'][offset:offset+count] = bytearray(count)
> +
> +def zero(h, count, offset, flags):
> +    expect_fua = cfg.get ('zero_expect_fua', False)
> +    actual_fua = bool (flags & nbdkit.FLAG_FUA)
> +    assert expect_fua == actual_fua
> +    expect_may_trim = cfg.get ('zero_expect_may_trim', False)
> +    actual_may_trim = bool (flags & nbdkit.FLAG_MAY_TRIM)
> +    assert expect_may_trim == actual_may_trim
> +    expect_fast_zero = cfg.get ('zero_expect_fast_zero', False)
> +    actual_fast_zero = bool (flags & nbdkit.FLAG_FAST_ZERO)
> +    assert expect_fast_zero == actual_fast_zero
> +    h['disk'][offset:offset+count] = bytearray(count)
> +
> +def cache(h, count, offset, flags):
> +    assert flags == 0
> +    # do nothing
> diff --git a/tests/test-python.py b/tests/test-python.py
> new file mode 100755
> index 0000000..2f565ba
> --- /dev/null
> +++ b/tests/test-python.py
> @@ -0,0 +1,172 @@
> +#!/usr/bin/env python3
> +# nbdkit
> +# Copyright (C) 2019 Red Hat Inc.
> +#
> +# Redistribution and use in source and binary forms, with or without
> +# modification, are permitted provided that the following conditions are
> +# met:
> +#
> +# * Redistributions of source code must retain the above copyright
> +# notice, this list of conditions and the following disclaimer.
> +#
> +# * Redistributions in binary form must reproduce the above copyright
> +# notice, this list of conditions and the following disclaimer in the
> +# documentation and/or other materials provided with the distribution.
> +#
> +# * Neither the name of Red Hat nor the names of its contributors may be
> +# used to endorse or promote products derived from this software without
> +# specific prior written permission.
> +#
> +# THIS SOFTWARE IS PROVIDED BY RED HAT AND CONTRIBUTORS ''AS
IS'' AND
> +# ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO,
> +# THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A
> +# PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL RED HAT OR
> +# CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
> +# SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
> +# LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF
> +# USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND
> +# ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY,
> +# OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT
> +# OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
> +# SUCH DAMAGE.
> +
> +# This tests the Python plugin thoroughly by issuing client commands
> +# through libnbd and checking we get the expected results.  It uses an
> +# associated plugin (test-python-plugin.sh).
test-python-plugin.py

This text can use python docstring:

"""
This tests ...
"""
> +
> +import os
> +import sys
> +import pickle
> +import codecs
> +
> +# Python has proven very difficult to valgrind, therefore it is disabled.
> +if "NBDKIT_VALGRIND" in os.environ:
> +    print ("$s: skipping Python test under valgrind" % __file__)
> +    sys.exit (77)
> +
> +try:
> +    import nbd
> +except:
except ImportError:
> +    print ("%s: skipped because cannot import python libnbd" %
__file__)
> +    sys.exit (77)
> +
> +try:
> +    srcdir = os.environ['SRCDIR']
> +except:
except KeyError:
> +    print ("%s: FAIL: test failed because $SRCDIR is not set" %
__file__)
> +    sys.exit (1)
> +
> +def test (cfg):
test will fool testing frameworks, expecting that this is a test function.
Maybe call this handle()? or case()?
> +    h = nbd.NBD ()
> +    cfg = codecs.encode (pickle.dumps (cfg), "base64").decode()
base64.b64encode() is better, avoiding unwanted newlines.
> +    cmd = ["nbdkit", "-v", "-s",
"--exit-with-parent",
> +           "python", srcdir +
"/test-python-plugin.py",
> +           "cfg=" + cfg]
> +    h.connect_command (cmd)
> +    return h
> +
> +# Test we can send an empty pickled test configuration and do nothing
> +# else.  This is just to ensure the machinery of the test works.
> +h = test ({})
So we have now running nbdkit that will exit the python collects when h
is implicitly closed when creating a new handle?

This is fragile, but can be solved with the help of a testing framework.
> +
> +# Test the size.
> +h = test ({"size": 512})
> +assert h.get_size() == 512
> +h = test ({"size": 1024*1024})
> +assert h.get_size() == 1024*1024
These tests will fail when on the first error, which is less useful.

With very little work we can convert this to pytest:

def test_size():
   h = test()
   assert h.get_size() == 512

To run the test you use:

pytest test-python.py
> +
> +# Test each flag call.
> +h = test ({"size": 512, "is_rotational": True})
> +assert h.is_rotational()
> +h = test ({"size": 512, "is_rotational": False})
> +assert not h.is_rotational()
When this fails, you will get unhelpful output:

    assert not True

But with pytest you get much more useful error, something like:

    def test_is_rotational():
        h = handle ({"size": 512, "is_rotational":
True})
>       assert not h.is_rotational
E       assert not True
E        +  where True = <test.handle.<locals>.H object at
0x7faa3358e450>.is_rotational

(I faked the handle object for this example)
> +
> +h = test ({"size": 512, "can_multi_conn": True})
> +assert h.can_multi_conn()
> +h = test ({"size": 512, "can_multi_conn": False})
> +assert not h.can_multi_conn()
> +
> +h = test ({"size": 512, "can_write": True})
> +assert not h.is_read_only()
> +h = test ({"size": 512, "can_write": False})
> +assert h.is_read_only()
> +
> +h = test ({"size": 512, "can_flush": True})
> +assert h.can_flush()
> +h = test ({"size": 512, "can_flush": False})
> +assert not h.can_flush()
> +
> +h = test ({"size": 512, "can_trim": True})
> +assert h.can_trim()
> +h = test ({"size": 512, "can_trim": False})
> +assert not h.can_trim()
> +
> +# nbdkit can always zero because it emulates it.
> +#h = test ({"size": 512, "can_zero": True})
> +#assert h.can_zero()
> +#h = test ({"size": 512, "can_zero": False})
> +#assert not h.can_zero()
> +
> +h = test ({"size": 512, "can_fast_zero": True})
> +assert h.can_fast_zero()
> +h = test ({"size": 512, "can_fast_zero": False})
> +assert not h.can_fast_zero()
> +
> +h = test ({"size": 512, "can_fua": "none"})
> +assert not h.can_fua()
> +h = test ({"size": 512, "can_fua":
"emulate"})
> +assert h.can_fua()
> +h = test ({"size": 512, "can_fua":
"native"})
> +assert h.can_fua()
> +
> +h = test ({"size": 512, "can_cache":
"none"})
> +assert not h.can_cache()
> +h = test ({"size": 512, "can_cache":
"emulate"})
> +assert h.can_cache()
> +h = test ({"size": 512, "can_cache":
"native"})
> +assert h.can_cache()
> +
> +# Not yet implemented: can_extents.
> +
> +# Test pread.
> +h = test ({"size": 512})
> +buf = h.pread (512, 0)
> +
> +# Test pwrite + flags.
> +h = test ({"size": 512})
> +h.pwrite (buf, 0)
> +
> +h = test ({"size": 512, "can_fua": "native",
"pwrite_expect_fua": True})
> +h.pwrite (buf, 0, nbd.CMD_FLAG_FUA)
> +
> +# Test flush.
> +h = test ({"size": 512, "can_flush": True})
> +h.flush ()
> +
> +# Test trim + flags.
> +h = test ({"size": 512, "can_trim": True})
> +h.trim (512, 0)
> +
> +h = test ({"size": 512,
> +           "can_trim": True, "can_fua":
"native", "trim_expect_fua": True})
This becomes messy. With the dict does not fit in one line, or
contains more than
few keys it is more readable to use one key: value pair per line.
> +h.trim (512, 0, nbd.CMD_FLAG_FUA)
> +
> +# Test zero + flags.
> +h = test ({"size": 512, "can_zero": True})
> +h.zero (512, 0, nbd.CMD_FLAG_NO_HOLE)
> +
> +h = test ({"size": 512,
> +           "can_zero": True, "can_fua":
"native", "zero_expect_fua": True})
> +h.zero (512, 0, nbd.CMD_FLAG_NO_HOLE | nbd.CMD_FLAG_FUA)
> +
> +h = test ({"size": 512, "can_zero": True,
"zero_expect_may_trim": True})
> +h.zero (512, 0, 0) # absence of nbd.CMD_FLAG_NO_HOLE
> +
> +h = test ({"size": 512,
> +           "can_zero": True, "can_fast_zero": True,
> +           "zero_expect_fast_zero": True})
> +h.zero (512, 0, nbd.CMD_FLAG_NO_HOLE | nbd.CMD_FLAG_FAST_ZERO)
> +
> +# Test cache.
> +h = test ({"size": 512, "can_cache":
"native"})
> +h.cache (512, 0)
> diff --git a/tests/test.py b/tests/test.py
> deleted file mode 100644
> index ac80d96..0000000
> --- a/tests/test.py
> +++ /dev/null
> @@ -1,60 +0,0 @@
> -import nbdkit
> -
> -disk = bytearray(1024*1024)
> -
> -
> -def api_version():
> -    return 2
> -
> -
> -def config_complete():
> -    print ("set_error = %r" % nbdkit.set_error)
> -
> -
> -def open(readonly):
> -    return 1
> -
> -
> -def get_size(h):
> -    global disk
> -    return len(disk)
> -
> -
> -def can_write(h):
> -    return True
> -
> -
> -def can_flush(h):
> -    return True
> -
> -
> -def is_rotational(h):
> -    return False
> -
> -
> -def can_trim(h):
> -    return True
> -
> -
> -def pread(h, count, offset, flags):
> -    global disk
> -    return disk[offset:offset+count]
> -
> -
> -def pwrite(h, buf, offset, flags):
> -    global disk
> -    end = offset + len(buf)
> -    disk[offset:end] = buf
> -
> -
> -def flush(h, flags):
> -    pass
> -
> -
> -def trim(h, count, offset, flags):
> -    pass
> -
> -
> -def zero(h, count, offset, flags):
> -    global disk
> -    disk[offset:offset+count] = bytearray(count)
> --
> 2.23.0
>
Otherwise very nice tests!

Nir

On Fri, Nov 22, 2019 at 9:54 PM Richard W.M. Jones <rjones@redhat.com>
wrote:
>
> It is unclear why we were constructing this by hand, but using the
> following tip we can use PyObject_CallFunction:
> https://stackoverflow.com/a/21221335
> ---
>  plugins/python/python.c | 17 ++++++-----------
>  1 file changed, 6 insertions(+), 11 deletions(-)
>
> diff --git a/plugins/python/python.c b/plugins/python/python.c
> index 148097f..d65ac45 100644
> --- a/plugins/python/python.c
> +++ b/plugins/python/python.c
> @@ -557,26 +557,21 @@ py_zero (void *handle, uint32_t count, uint64_t
offset, int may_trim)
>  {
>    PyObject *obj = handle;
>    PyObject *fn;
> -  PyObject *args;
>    PyObject *r;
>
>    if (callback_defined ("zero", &fn)) {
>      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);
> +    r = PyObject_CallFunction (fn, "OiLO",
> +                               obj, count, offset,
> +                               may_trim ? Py_True : Py_False, NULL);
Much nicer.
>      Py_DECREF (fn);
> -    Py_DECREF (args);
>      if (last_error == EOPNOTSUPP || last_error == ENOTSUP) {
>        /* When user requests this particular error, we want to
> -         gracefully fall back, and to accomodate both a normal return
> -         and an exception. */
> +       * gracefully fall back, and to accomodate both a normal return
> +       * and an exception.
> +       */
>        nbdkit_debug ("zero requested falling back to pwrite");
>        Py_XDECREF (r);
>        PyErr_Clear ();
> --
> 2.23.0
>