4:48 p.m.
In preparation for adding other optional flag arguments to the
python bindings, start by making the existing 'may_trim' flag
to 'zero' be optional. That is, the plugin need not define
the parameter if it does not make any semantic difference (ie.
if the plugin ignores the hint and never trims); while if the
parameter exists, we now pass it as a keyword argument rather
than as a positional argument. This requires the plugin to
give the parameter a specific name, and works whether or not
the plugin provides a default for the parameter (although we do
now recommend that plugins provide a default value, as it makes
plugins built against newer nbdkit more likely to run even on
older nbdkit, even though that's not the direction we typically
guarantee). We can't see through a plugin that used '**kwargs',
but that is less likely.
If we are super-worried about back-compatibility to older
plugins which hard-coded 4 required parameters, we could also
tweak the introspection to assume that a 'zero' with 4
parameters and no defaults, where we do not recognize the
fourth parameter name, is an old-style plugin, and call it
with may_trim passed via a positional rather than a keyword
argument. However, I suspect most plugins just copied from
our examples, which means they used the right parameter naming
to just keep working; furthermore, while we have been clear
that backwards API compatibility is essential for C, we have
not made the same guarantee for language plugins.
The introspection framework here will also make it easy to
probe for future flag additions (support for 'fua' being the
obvious candidate in the short term).
This patch also fixes failure to check for (unlikely) errors
during creation of 'args' within py_zero().
Signed-off-by: Eric Blake <eblake(a)redhat.com>
---
plugins/python/nbdkit-python-plugin.pod | 35 ++++++++---
plugins/python/python.c | 78 +++++++++++++++++++++++--
plugins/python/example.py | 2 +-
tests/test.py | 2 +-
4 files changed, 102 insertions(+), 15 deletions(-)
diff --git a/plugins/python/nbdkit-python-plugin.pod
b/plugins/python/nbdkit-python-plugin.pod
index 3680fd65..f504cf7a 100644
--- a/plugins/python/nbdkit-python-plugin.pod
+++ b/plugins/python/nbdkit-python-plugin.pod
@@ -40,11 +40,24 @@ C<__main__> module):
def pread(h, count, offset):
# see below
-Note that the subroutines must have those literal names (like C<open>),
-because the C part looks up and calls those functions directly. You
-may want to include documentation and globals (eg. for storing global
-state). Any other top level statements are run when the script is
-loaded, just like ordinary Python.
+Note that the functions must have those literal names (like C<open>),
+because the C part looks up and calls those functions directly. Where
+this documentation lists a parameter as mandatory, you can name the
+parameter what you would like (the C part sets that parameter
+positionally); however, where this documentation lists a parameter
+with a default value, you must either omit that parameter or use that
+exact parameter name (the C part inspects the function signature to
+see whether your callback implemented that parameter, and if so, sets
+the parameter via keyword). In this way, newer versions of nbdkit can
+add additional flags with default values that can be useful to newer
+plugins, but still run older plugins without requiring them to be
+rewritten to add support for the new flags. Note that using
+C<**kwargs> in your function instead of named parameters would defeat
+the C code that inspects the signature.
+
+You may want to include documentation and globals (eg. for storing
+global state). Any other top level statements are run when the script
+is loaded, just like ordinary Python.
=head2 Python versions
@@ -260,13 +273,17 @@ exception, optionally using C<nbdkit.set_error> first.
(Optional)
- def zero(h, count, offset, may_trim):
+ def zero(h, count, offset, may_trim=False):
# no return value
The body of your C<zero> function should ensure that C<count> bytes
-of the disk, starting at C<offset>, will read back as zero. If
-C<may_trim> is true, the operation may be optimized as a trim as long
-as subsequent reads see zeroes.
+of the disk, starting at C<offset>, will read back as zero.
+
+Your function may support an optional C<may_trim> parameter; if it is
+present and the caller sets it to True, then your callback may
+optimize the operation by using a trim, as long as subsequent reads
+see zeroes. If you omit the optional parameter, or if the caller sets
+it to False, writing zeroes should not punch holes.
NBD only supports whole writes, so your function should try to
write the whole region (perhaps requiring a loop). If the write
diff --git a/plugins/python/python.c b/plugins/python/python.c
index 7052aac0..d18bcb5e 100644
--- a/plugins/python/python.c
+++ b/plugins/python/python.c
@@ -1,5 +1,5 @@
/* nbdkit
- * Copyright (C) 2013-2018 Red Hat Inc.
+ * Copyright (C) 2013-2019 Red Hat Inc.
*
* Redistribution and use in source and binary forms, with or without
* modification, are permitted provided that the following conditions are
@@ -109,6 +109,43 @@ callback_defined (const char *name, PyObject **obj_rtn)
return 1;
}
+/* Does a callback support the given keyword parameter? */
+static int
+callback_has_parameter (PyObject *fn, const char *name)
+{
+ int r = 0;
+ PyObject *inspect, *pname, *spec, *params;
+
+ assert (script != NULL);
+ assert (module != NULL);
+
+ pname = PyUnicode_FromString ("inspect");
+ if (!pname)
+ return -1;
+ inspect = PyImport_Import (pname);
+ Py_DECREF (pname);
+
+ if (!inspect)
+ return -1;
+
+ spec = PyObject_CallMethod (inspect, "signature", "O", fn);
+ Py_DECREF (inspect);
+ if (!spec)
+ return -1;
+
+ /* We got the signature; now check for the requested keyword */
+ params = PyObject_GetAttrString (spec, "parameters");
+ if (!params) {
+ Py_DECREF (spec);
+ return -1;
+ }
+ r = PyMapping_HasKeyString (params, name);
+
+ Py_DECREF (spec);
+ Py_DECREF (params);
+ return r;
+}
+
/* Convert bytes/str/unicode into a string. Caller must free. */
static char *
python_to_string (PyObject *str)
@@ -564,16 +601,49 @@ py_zero (void *handle, uint32_t count, uint64_t offset, int
may_trim)
{
PyObject *obj = handle;
PyObject *fn;
+ PyObject *args;
+ PyObject *kwargs;
PyObject *r;
if (callback_defined ("zero", &fn)) {
+ static int zero_may_trim = -1;
+
+ if (zero_may_trim < 0)
+ zero_may_trim = callback_has_parameter (fn, "may_trim");
+ if (zero_may_trim < 0) {
+ check_python_failure ("zero");
+ return -1;
+ }
+
PyErr_Clear ();
last_error = 0;
- r = PyObject_CallFunction (fn, "OiLO",
- obj, count, offset,
- may_trim ? Py_True : Py_False);
+ args = Py_BuildValue ("OiL", obj, count, offset);
+ if (!args) {
+ check_python_failure ("zero");
+ Py_DECREF (fn);
+ return -1;
+ }
+ kwargs = PyDict_New ();
+ if (!kwargs) {
+ check_python_failure ("zero");
+ Py_DECREF (args);
+ Py_DECREF (fn);
+ return -1;
+ }
+ if (zero_may_trim &&
+ PyDict_SetItemString (kwargs, "may_trim",
+ may_trim ? Py_True : Py_False) == -1) {
+ check_python_failure ("zero");
+ Py_DECREF (kwargs);
+ Py_DECREF (args);
+ Py_DECREF (fn);
+ return -1;
+ }
+ r = PyObject_Call (fn, args, kwargs);
Py_DECREF (fn);
+ Py_DECREF (args);
+ Py_DECREF (kwargs);
if (last_error == EOPNOTSUPP || last_error == ENOTSUP) {
/* When user requests this particular error, we want to
* gracefully fall back, and to accommodate both a normal return
diff --git a/plugins/python/example.py b/plugins/python/example.py
index 60f9d7f0..9205f107 100644
--- a/plugins/python/example.py
+++ b/plugins/python/example.py
@@ -65,7 +65,7 @@ def pwrite(h, buf, offset):
disk[offset:end] = buf
-def zero(h, count, offset, may_trim):
+def zero(h, count, offset, may_trim=False):
global disk
if may_trim:
disk[offset:offset+count] = bytearray(count)
diff --git a/tests/test.py b/tests/test.py
index 9a2e947d..a83f1181 100644
--- a/tests/test.py
+++ b/tests/test.py
@@ -43,7 +43,7 @@ def pwrite(h, buf, offset):
disk[offset:end] = buf
-def zero(h, count, offset, may_trim=False):
+def zero(h, count, offset):
global disk
disk[offset:offset+count] = bytearray(count)
--
2.21.0