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

On Fri, Nov 22, 2019 at 9:54 PM Richard W.M. Jones <rjones(a)redhat.com&gt; wrote: Much nicer.

On Fri, Nov 22, 2019 at 9:54 PM Richard W.M. Jones <rjones(a)redhat.com&gt; wrote: Nice way to expose the flags! 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/53f7f0284c084ac2e4542fd1f71d0406075a... https://github.com/nirs/sanlock/blob/53f7f0284c084ac2e4542fd1f71d0406075a...

On Fri, Nov 22, 2019 at 10:52 PM Richard W.M. Jones <rjones(a)redhat.com&gt; wrote: exit can hide the original error if we are not careful, so it is best avoided. It is nice to decref and return NULL, the user code will fail with nice traceback about (likely) MemoryError. And it does not cost much to do the right thing.

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

On Fri, Nov 22, 2019 at 11:14 PM Richard W.M. Jones <rjones(a)redhat.com&gt; wrote: Lot of python code is a thin wrapper over C code, and low level details like flags leak out to python, so this is a reasonable API. But having more pythonic API can make python developer little happier when working with this code. I don't know if it is worth the extra effort. Nir

On 11/22/19 1:53 PM, Richard W.M. Jones wrote: ACK -- 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: 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). -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org

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 | 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

On Fri, Nov 22, 2019 at 9:55 PM Richard W.M. Jones <rjones(a)redhat.com&gt; wrote: 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: b'<!doctype ' I think the plugin should support both bytearray, memoryview, or bytes. Supporting objects implementing the buffer protocol would be best. test-python-plugin.py This text can use python docstring: """ This tests ... """ except ImportError: except KeyError: test will fool testing frameworks, expecting that this is a test function. Maybe call this handle()? or case()? base64.b64encode() is better, avoiding unwanted newlines. 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. 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 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}) E assert not True E + where True = <test.handle.<locals>.H object at 0x7faa3358e450>.is_rotational (I faked the handle object for this example) 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. Otherwise very nice tests! Nir