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). 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 also supports 'may_trim', but pass it via 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. The introspection framework here will also make it easy to probe for future flag additions. Signed-off-by: Eric Blake <eblake@redhat.com> --- At least the plugin for rhv-upload [1] used the right naming: [1] https://github.com/libguestfs/libguestfs/blob/53b31df7c/v2v/rhv-upload-plugi... plugins/python/nbdkit-python-plugin.pod | 35 +++++++++++---- plugins/python/python.c | 80 +++++++++++++++++++++++++++++++-- plugins/python/example.py | 2 +- tests/test.py | 2 +- 4 files changed, 105 insertions(+), 14 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 35e8df2..d944905 100644 --- a/plugins/python/python.c +++ b/plugins/python/python.c @@ -64,6 +64,8 @@ static PyObject *module; static int last_error; +static int zero_may_trim = -1; + static PyObject * set_error (PyObject *self, PyObject *args) { @@ -108,6 +110,68 @@ 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); + + pname = PyString_FromString ("inspect"); + if (!pname) + return -1; + inspect = PyImport_Import (pname); + Py_DECREF (pname); + + if (!inspect) + return -1; + +#if PY_MAJOR_VERSION >= 3 + pname = PyString_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; + + /* Yay, we got the signature; now inspect if it contains keyword 'name' */ + args = PyTuple_GetItem(spec, 0); + if (check_list(args, name)) + r = 1; + else { + /* inspecting kwonly args is only available in python 3 */ + args = PyTuple_GetItem(spec, 5); + if (check_list(args, name)) + r = 1; + } + + Py_DECREF (spec); + return r; +} + /* Convert bytes/str/unicode into a string. Caller must free. */ static char * python_to_string (PyObject *str) @@ -530,21 +594,31 @@ py_zero (void *handle, uint32_t count, uint64_t offset, int may_trim) PyObject *obj = handle; PyObject *fn; PyObject *args; + PyObject *kwargs = NULL; PyObject *r; if (callback_defined ("zero", &fn)) { + 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); + args = PyTuple_New (3); 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); + if (zero_may_trim) + kwargs = Py_BuildValue("{s:i}", "may_trim", may_trim); + r = PyObject_Call (fn, args, kwargs); Py_DECREF (fn); Py_DECREF (args); + Py_XDECREF (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