12:03 a.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, although that's not the direction we typically
guarantee). We can't see through a plugin that used '**kwargs',
but that is less likely.
If we are super-worried about back-compatibility to older
plugins which hard-coded 4 required parameters, we could also
tweak the introspection to assume that a 'zero' with 4
parameters and no defaults, where we do not recognize the
fourth parameter name, is an old-style plugin, and call it
with may_trim passed via a positional rather than a keyword
argument. However, I suspect most plugins just copied from
our examples, which means they used the right parameter naming
to just keep working; furthermore, while we have been clear
that backwards API compatibility is essential for C, we have
not made the same guarantee for language plugins.
The introspection framework here will also make it easy to
probe for future flag additions (support for 'fua' being the
obvious candidate in the short term).
This patch also fixes failure to check for (unlikely) errors
during creation of 'args' within py_zero().
Signed-off-by: Eric Blake <eblake(a)redhat.com>
---
plugins/python/nbdkit-python-plugin.pod | 35 ++++++++---
plugins/python/python.c | 108 +++++++++++++++++++++++++++++---
plugins/python/example.py | 2 +-
tests/test.py | 2 +-
4 files changed, 129 insertions(+), 18 deletions(-)
diff --git a/plugins/python/nbdkit-python-plugin.pod
b/plugins/python/nbdkit-python-plugin.pod
index c3a564e..19f9eff 100644
--- a/plugins/python/nbdkit-python-plugin.pod
+++ b/plugins/python/nbdkit-python-plugin.pod
@@ -51,11 +51,24 @@ C<__main__> module):
def pread(h, count, offset):
# see below
-Note that the subroutines must have those literal names (like C<open>),
-because the C part looks up and calls those functions directly. You
-may want to include documentation and globals (eg. for storing global
-state). Any other top level statements are run when the script is
-loaded, just like ordinary Python.
+Note that the functions must have those literal names (like C<open>),
+because the C part looks up and calls those functions directly. Where
+this documentation lists a parameter as mandatory, you can name the
+parameter what you would like (the C part sets that parameter
+positionally); however, where this documentation lists a parameter
+with a default value, you must either omit that parameter or use that
+exact parameter name (the C part inspects the function signature to
+see whether your callback implemented that parameter, and if so, sets
+the parameter via keyword). In this way, newer versions of nbdkit can
+add additional flags with default values that can be useful to newer
+plugins, but still run older plugins without requiring them to be
+rewritten to add support for the new flags. Note that using
+C<**kwargs> in your function instead of named parameters would defeat
+the C code that inspects the signature.
+
+You may want to include documentation and globals (eg. for storing
+global state). Any other top level statements are run when the script
+is loaded, just like ordinary Python.
=head2 EXECUTABLE SCRIPT
@@ -231,13 +244,17 @@ exception, optionally using C<nbdkit.set_error> first.
(Optional)
- def zero(h, count, offset, may_trim):
+ def zero(h, count, offset, may_trim=False):
# no return value
The body of your C<zero> function should ensure that C<count> bytes
-of the disk, starting at C<offset>, will read back as zero. If
-C<may_trim> is true, the operation may be optimized as a trim as long
-as subsequent reads see zeroes.
+of the disk, starting at C<offset>, will read back as zero.
+
+Your function may support an optional C<may_trim> parameter; if it is
+present and the caller sets it to True, then your callback may
+optimize the operation by using a trim, as long as subsequent reads
+see zeroes. If you omit the optional parameter, or if the caller sets
+it to False, writing zeroes should not punch holes.
NBD only supports whole writes, so your function should try to
write the whole region (perhaps requiring a loop). If the write
diff --git a/plugins/python/python.c b/plugins/python/python.c
index 7eb91d7..07559a5 100644
--- a/plugins/python/python.c
+++ b/plugins/python/python.c
@@ -108,6 +108,73 @@ callback_defined (const char *name, PyObject **obj_rtn)
return 1;
}
+/* Checks whether a list of strings contains the given name */
+static int
+check_list (PyObject *list, const char *name)
+{
+ ssize_t i = 0;
+ PyObject *elt;
+
+ if (!list)
+ return 0;
+ while ((elt = PyList_GetItem (list, i++))) {
+ char *str = PyString_AsString (elt);
+ if (str && !strcmp (str, name))
+ return 1;
+ }
+ return 0;
+}
+
+/* Does a callback support the given keyword parameter? */
+static int
+callback_has_parameter (PyObject *fn, const char *name)
+{
+ int r = 0;
+ PyObject *inspect, *pname, *spec, *args;
+
+ assert (script != NULL);
+ assert (module != NULL);
+
+#ifdef HAVE_PYSTRING_FROMSTRING
+ pname = PyString_FromString ("inspect");
+#else
+ pname = PyUnicode_FromString ("inspect");
+#endif
+ if (!pname)
+ return -1;
+ inspect = PyImport_Import (pname);
+ Py_DECREF (pname);
+
+ if (!inspect)
+ return -1;
+
+#if PY_MAJOR_VERSION >= 3
+ pname = PyUnicode_FromString ("getfullargspec");
+#else
+ pname = PyString_FromString ("getargspec");
+#endif
+ spec = PyObject_CallMethodObjArgs (inspect, pname, fn, NULL);
+ Py_DECREF (pname);
+ Py_DECREF (inspect);
+ if (!spec)
+ return -1;
+
+ /* We got the signature; now check for the requested keyword */
+ args = PyTuple_GetItem (spec, 0);
+ if (check_list (args, name))
+ r = 1;
+#if PY_MAJOR_VERSION >= 3
+ else {
+ args = PyTuple_GetItem (spec, 5);
+ if (check_list (args, name))
+ r = 1;
+ }
+#endif
+
+ Py_DECREF (spec);
+ return r;
+}
+
/* Convert bytes/str/unicode into a string. Caller must free. */
static char *
python_to_string (PyObject *str)
@@ -481,21 +548,48 @@ py_zero (void *handle, uint32_t count, uint64_t offset, int
may_trim)
PyObject *obj = handle;
PyObject *fn;
PyObject *args;
+ PyObject *kwargs;
PyObject *r;
if (callback_defined ("zero", &fn)) {
+ static int zero_may_trim = -1;
+
+ if (zero_may_trim < 0)
+ zero_may_trim = callback_has_parameter (fn, "may_trim");
+ if (zero_may_trim < 0) {
+ check_python_failure ("zero");
+ return -1;
+ }
+
PyErr_Clear ();
last_error = 0;
- args = PyTuple_New (4);
- Py_INCREF (obj); /* decremented by Py_DECREF (args) */
- PyTuple_SetItem (args, 0, obj);
- PyTuple_SetItem (args, 1, PyLong_FromUnsignedLongLong (count));
- PyTuple_SetItem (args, 2, PyLong_FromUnsignedLongLong (offset));
- PyTuple_SetItem (args, 3, PyBool_FromLong (may_trim));
- r = PyObject_CallObject (fn, args);
+ args = Py_BuildValue ("OiL", obj, count, offset);
+ if (!args) {
+ check_python_failure ("zero");
+ Py_DECREF (fn);
+ return -1;
+ }
+ kwargs = PyDict_New ();
+ if (!kwargs) {
+ check_python_failure ("zero");
+ Py_DECREF (args);
+ Py_DECREF (fn);
+ return -1;
+ }
+ if (zero_may_trim &&
+ PyDict_SetItemString (kwargs, "may_trim",
+ may_trim ? Py_True : Py_False) == -1) {
+ check_python_failure ("zero");
+ Py_DECREF (kwargs);
+ Py_DECREF (args);
+ Py_DECREF (fn);
+ return -1;
+ }
+ r = PyObject_Call (fn, args, kwargs);
Py_DECREF (fn);
Py_DECREF (args);
+ Py_DECREF (kwargs);
if (last_error == EOPNOTSUPP) {
/* When user requests this particular error, we want to
gracefully fall back, and to accomodate both a normal return
diff --git a/plugins/python/example.py b/plugins/python/example.py
index 60f9d7f..9205f10 100644
--- a/plugins/python/example.py
+++ b/plugins/python/example.py
@@ -65,7 +65,7 @@ def pwrite(h, buf, offset):
disk[offset:end] = buf
-def zero(h, count, offset, may_trim):
+def zero(h, count, offset, may_trim=False):
global disk
if may_trim:
disk[offset:offset+count] = bytearray(count)
diff --git a/tests/test.py b/tests/test.py
index 630ac2f..518cdd4 100644
--- a/tests/test.py
+++ b/tests/test.py
@@ -41,7 +41,7 @@ def pwrite(h, buf, offset):
disk[offset:end] = buf
-def zero(h, count, offset, may_trim=False):
+def zero(h, count, offset):
global disk
disk[offset:offset+count] = bytearray(count)
--
2.14.3