[PATCH nbdkit v2 00/10] Implement nbdkit API v2 for Python plugins.
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
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 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
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
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/53f7f0284c084ac2e4542fd1f71d0406075adb5... https://github.com/nirs/sanlock/blob/53f7f0284c084ac2e4542fd1f71d0406075adb5...
+ 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 Fri, Nov 22, 2019 at 10:05:15PM +0200, Nir Soffer wrote:
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/53f7f0284c084ac2e4542fd1f71d0406075adb5... https://github.com/nirs/sanlock/blob/53f7f0284c084ac2e4542fd1f71d0406075adb5...
Yes I can make this change. It's worth it in case there is some subtle Python API change in future that we need to be aware of. We can in fact exit if load() fails so I guess we don't need to bother doing the PY_DECREF for this case. Thanks, Rich.
+ 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
-- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-p2v converts physical machines to virtual machines. Boot with a live CD or over the network (PXE) and turn machines into KVM guests. http://libguestfs.org/virt-v2v
On Fri, Nov 22, 2019 at 10:52 PM Richard W.M. Jones <rjones@redhat.com> wrote:
On Fri, Nov 22, 2019 at 10:05:15PM +0200, Nir Soffer wrote:
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/53f7f0284c084ac2e4542fd1f71d0406075adb5... https://github.com/nirs/sanlock/blob/53f7f0284c084ac2e4542fd1f71d0406075adb5...
Yes I can make this change. It's worth it in case there is some subtle Python API change in future that we need to be aware of.
We can in fact exit if load() fails so I guess we don't need to bother doing the PY_DECREF for this case.
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.
Thanks,
Rich.
+ 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
-- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-p2v converts physical machines to virtual machines. Boot with a live CD or over the network (PXE) and turn machines into KVM guests. http://libguestfs.org/virt-v2v
On Fri, Nov 22, 2019 at 11:26:55PM +0200, Nir Soffer wrote:
On Fri, Nov 22, 2019 at 10:52 PM Richard W.M. Jones <rjones@redhat.com> wrote:
On Fri, Nov 22, 2019 at 10:05:15PM +0200, Nir Soffer wrote:
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/53f7f0284c084ac2e4542fd1f71d0406075adb5... https://github.com/nirs/sanlock/blob/53f7f0284c084ac2e4542fd1f71d0406075adb5...
Yes I can make this change. It's worth it in case there is some subtle Python API change in future that we need to be aware of.
We can in fact exit if load() fails so I guess we don't need to bother doing the PY_DECREF for this case.
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.
This is called before we enter any Python code so I don't think that will work. (In general Python is embedded in C in nbdkit, so everything works in reverse to usual Python/C extensions.) Rich.
Thanks,
Rich.
+ 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
-- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-p2v converts physical machines to virtual machines. Boot with a live CD or over the network (PXE) and turn machines into KVM guests. http://libguestfs.org/virt-v2v
-- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-df lists disk usage of guests without needing to install any software inside the virtual machine. Supports Linux and Windows. http://people.redhat.com/~rjones/virt-df/
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 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 Fri, Nov 22, 2019 at 02:55:31PM -0600, Eric Blake wrote:
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]
I believe it's not possible to do it for sh plugins. For (eg) python plugins it works like this: #!/usr/sbin/nbdkit python -> runs nbdkit python <script name> -> <script name> is interpreted as magic script= parameter -> the python plugin works by loading the script using PyRun_SimpleFileEx which interprets the contents of the script as python code However for shell it doesn't work: #!/usr/sbin/nbdkit sh -> runs nbdkit sh <script name> -> <script name> is interpreted as magic script= parameter -> the sh plugin works by actually executing the script -> executing the script repeats the steps over from the top, causing an infinite loop If you can think of a way to make this work it would be a useful feature IMO. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com Fedora Windows cross-compiler. Compile Windows programs, test, and build Windows installers. Over 100 libraries supported. http://fedoraproject.org/wiki/MinGW
On Fri, Nov 22, 2019 at 11:07 PM Richard W.M. Jones <rjones@redhat.com> wrote:
On Fri, Nov 22, 2019 at 02:55:31PM -0600, Eric Blake wrote:
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]
I believe it's not possible to do it for sh plugins.
For (eg) python plugins it works like this:
#!/usr/sbin/nbdkit python -> runs nbdkit python <script name> -> <script name> is interpreted as magic script= parameter -> the python plugin works by loading the script using PyRun_SimpleFileEx which interprets the contents of the script as python code
However for shell it doesn't work:
#!/usr/sbin/nbdkit sh -> runs nbdkit sh <script name> -> <script name> is interpreted as magic script= parameter -> the sh plugin works by actually executing the script -> executing the script repeats the steps over from the top, causing an infinite loop
If you can think of a way to make this work it would be a useful feature IMO.
Even if we can find a way, it sounds too complicated, and I don't see the use case. Nir
On Sat, Nov 23, 2019 at 01:58:02AM +0200, Nir Soffer wrote:
On Fri, Nov 22, 2019 at 11:07 PM Richard W.M. Jones <rjones@redhat.com> wrote:
On Fri, Nov 22, 2019 at 02:55:31PM -0600, Eric Blake wrote:
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]
I believe it's not possible to do it for sh plugins.
For (eg) python plugins it works like this:
#!/usr/sbin/nbdkit python -> runs nbdkit python <script name> -> <script name> is interpreted as magic script= parameter -> the python plugin works by loading the script using PyRun_SimpleFileEx which interprets the contents of the script as python code
However for shell it doesn't work:
#!/usr/sbin/nbdkit sh -> runs nbdkit sh <script name> -> <script name> is interpreted as magic script= parameter -> the sh plugin works by actually executing the script -> executing the script repeats the steps over from the top, causing an infinite loop
If you can think of a way to make this work it would be a useful feature IMO.
Even if we can find a way, it sounds too complicated, and I don't see the use case.
I think an aim of nbdkit is to make it as widely usable as possible. That's for example why we have apparent esoterica like plugins written in shell script. We want to in the future ship the (or "an") imageio plugin separately from virt-v2v. A possibility is it might be shipped as a single Python file which starts with ‘#!/usr/bin/env nbdkit python’ which would allow anyone to run it directly. They wouldn't need to understand anything about nbdkit or its command line. So that's the use case (which to be fair is not actually used much today at all). The context above is I can't think of a way to extend this feature to nbdkit-sh-plugin. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-df lists disk usage of guests without needing to install any software inside the virtual machine. Supports Linux and Windows. http://people.redhat.com/~rjones/virt-df/
On Sat, Nov 23, 2019 at 5:46 PM Richard W.M. Jones <rjones@redhat.com> wrote:
On Sat, Nov 23, 2019 at 01:58:02AM +0200, Nir Soffer wrote:
On Fri, Nov 22, 2019 at 11:07 PM Richard W.M. Jones <rjones@redhat.com> wrote:
On Fri, Nov 22, 2019 at 02:55:31PM -0600, Eric Blake wrote:
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]
I believe it's not possible to do it for sh plugins.
For (eg) python plugins it works like this:
#!/usr/sbin/nbdkit python -> runs nbdkit python <script name> -> <script name> is interpreted as magic script= parameter -> the python plugin works by loading the script using PyRun_SimpleFileEx which interprets the contents of the script as python code
However for shell it doesn't work:
#!/usr/sbin/nbdkit sh -> runs nbdkit sh <script name> -> <script name> is interpreted as magic script= parameter -> the sh plugin works by actually executing the script -> executing the script repeats the steps over from the top, causing an infinite loop
If you can think of a way to make this work it would be a useful feature IMO.
Even if we can find a way, it sounds too complicated, and I don't see the use case.
I think an aim of nbdkit is to make it as widely usable as possible. That's for example why we have apparent esoterica like plugins written in shell script.
We want to in the future ship the (or "an") imageio plugin separately from virt-v2v. A possibility is it might be shipped as a single Python file which starts with ‘#!/usr/bin/env nbdkit python’ which would allow anyone to run it directly. They wouldn't need to understand anything about nbdkit or its command line.
But the imageio plugin is not a script you can run, and if you want to make it a standalone tool it needs a lot of work that is not related to virt-v2v like adding command line parsing or dealing with file formats. Such tool mostly already exists in ovirt sdk examples: https://github.com/oVirt/ovirt-engine-sdk/blob/master/sdk/examples/upload_di... So for rhv-upload there is no use case for standalone script.
So that's the use case (which to be fair is not actually used much today at all). The context above is I can't think of a way to extend this feature to nbdkit-sh-plugin.
But today we require the plugin name in nbdkit command line: ./nbdkit -f -v python zero.py It could be nice to be able to do: ./nbdkit -f -v zero.py Detect that the filename is a python/lua/shell/perl,... file and then load the right plugin. We can use special comments in the plugin to provide more info about the plugin, for example arguments that can be used with the plugin. $ cat example.py # example python plugin # nbdkit-plugin: python # nbdkit-argument: filename code... $ ./nbdkit -f -v example.py --help show help about example.py usage... $ ./nbdkit -f -v example.py filename=/dev/zero Internally nbdkit will replace this command with: nbdkit -f -v python example.py filename=/dev/zero And continue the normal flow. So the actual nbdkit command is: nbdkit plugin-name|script ... Nir
On Sun, Nov 24, 2019 at 03:31:35PM +0200, Nir Soffer wrote:
But the imageio plugin is not a script you can run, and if you want to make it a standalone tool it needs a lot of work that is not related to virt-v2v like adding command line parsing or dealing with file formats.
Such tool mostly already exists in ovirt sdk examples: https://github.com/oVirt/ovirt-engine-sdk/blob/master/sdk/examples/upload_di...
So for rhv-upload there is no use case for standalone script.
Sure, not right now, but what I'm saying is that a standalone imageio plugin would be possible.
So that's the use case (which to be fair is not actually used much today at all). The context above is I can't think of a way to extend this feature to nbdkit-sh-plugin.
But today we require the plugin name in nbdkit command line:
./nbdkit -f -v python zero.py
It could be nice to be able to do:
./nbdkit -f -v zero.py
No need because with shebang scripts you can do: ./zero.py [-f] [-v] [params ...] and for Python plugins this works today. (It doesn't work with nbdkit-sh-plugin.) Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-p2v converts physical machines to virtual machines. Boot with a live CD or over the network (PXE) and turn machines into KVM guests. http://libguestfs.org/virt-v2v
On 11/23/19 9:46 AM, Richard W.M. Jones wrote:
I think an aim of nbdkit is to make it as widely usable as possible. That's for example why we have apparent esoterica like plugins written in shell script.
We want to in the future ship the (or "an") imageio plugin separately from virt-v2v. A possibility is it might be shipped as a single Python file which starts with ‘#!/usr/bin/env nbdkit python’ which
That is not portable; not all kernels permit more than one argument in #!. You can work around it with modern env (both BSD and Coreutils have recently added -S), as: #!/usr/bin/env -S nbdkit python which will then invoke the desired 'nbdkit' 'python' 'file' regardless of whether the OS invoked 'env' '-S nbdkit python' (Linux) or 'env' '-S' 'nbdkit' 'python' (others). But that in turn assumes env -S is portable (it's fairly new, so not yet). Otherwise, interpreted plugin shebangs really do have to hard-code the expected path of nbdkit rather than relying on env, unless we find a way to make: nbdkit file determine that when 'nbdkit-plugin-file.so' does not exist but 'file' does, then inspect file to determine if 'nbdkit-plugin-interpreter.so' exists based on the first line of file. Except that would mean the first line changes to: #!/usr/bin/env nbdkit which lacks the desired interpreter, so we'd still have to add something like documenting that if the first line of the #! contains 'nbdkit', then the second line is some form of magic comment that includes the substring 'PLUGIN=...' (we have to allow arbitrary slop as prefix or suffix after a plugin name, to allow the second line to be a comment or otherwise innocuous interpreted line in whatever desired target language), so we end up with: #!/usr/bin/env nbdkit # PLUGIN=python where the initial invocation of '/usr/bin/env' 'nbdkit' 'file' then passes on to '/path/to/nbdkit' 'file' which sees that 'file' is not a plugin, but does have magic, so it then retries as if invoked as '/path/to/nbdkit' 'python' 'file' -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
On 11/22/19 3:07 PM, Richard W.M. Jones wrote:
On Fri, Nov 22, 2019 at 02:55:31PM -0600, Eric Blake wrote:
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]
I believe it's not possible to do it for sh plugins.
For (eg) python plugins it works like this:
#!/usr/sbin/nbdkit python -> runs nbdkit python <script name> -> <script name> is interpreted as magic script= parameter -> the python plugin works by loading the script using PyRun_SimpleFileEx which interprets the contents of the script as python code
However for shell it doesn't work:
#!/usr/sbin/nbdkit sh -> runs nbdkit sh <script name> -> <script name> is interpreted as magic script= parameter -> the sh plugin works by actually executing the script -> executing the script repeats the steps over from the top, causing an infinite loop
If you can think of a way to make this work it would be a useful feature IMO.
Hmm. Right now, we document that: nbdkit sh file requires 'file' to be executable, and that we call exec*("file", { "file", "op", ... }) and that the interpreter need not be sh (file can start with any #! to run a script that will be executed by another interpreter). In order to get a shebang redirect to work, though, it seems like we must NOT directly re-execute the file, but instead force the file to be parsed by "/bin/sh" (or have some other means of identifying exactly which interpreter to use, outside of the normal #! means), where we would invoke exec*("/bin/sh", {"file", "op", ...}) and thereby skip the kernel's attempt to re-execute under a different interpreter. Perhaps we could get this by teaching the sh plugin to read() the first line of the script; if it starts with #! and contains 'nbdkit sh', then we force exec through "sh" (hmm, is there a way to force exec through bash instead?). Otherwise, we exec through the file-name as-is. Then our sh plugin would support two modes: the existing mode (we exec any interpreter by letting the kernel interpret the #! line, but you can't get shebang forwarding over to nbdkit), and the new mode (we exec a shell directly to bypass kernel shebang interpretation, but now you can write a shell script that gets shebang forwarding over to nbdkit). It's also a bummer that different platforms vary on how #! lines are parsed - you can't portably pass more than a single argument, so there is no portable way to write '#!/path/to/nbdkit sh shell=/bin/bash' as a convenient way to choose which shell to use when exec'ing the script while bypassing the #!, short of having nbdkit reimplement what GNU coreutils recently added as '/bin/env -S' (and that reimplementation would have to be in nbdkit proper, not in the sh plugin, because such a shebang line would result in 'nbdkit' 'sh shell=/bin/bash' 'file' 'args', but there is no '/path/to/nbdkit-plugin-sh shell=/bin/bash.so' plugin to be loaded). If we choose to make the sh plugin smart enough to parse line 1 to see if it is a shebang containing the substring 'nbdkit sh', we could also make it parse line 2 to pick up some magic comment line of 'SHELL=/bin/bash' as the binary to exec. That's a bit more complicated to document, but might serve to let us use the sh plugin for the two separate modes documented above (mode 1 to run an arbitrary executable file which may use shebang to call out its own interpreter, mode 2 to run an arbitrary file with a fixed interpreter allowing that file to start with a shebang to call out to nbdkit). Or maybe we create two different plugins (keep the existing 'sh' plugin with existing semantics, and add a new 'shell' plugin that shares 99% of the code but forces execution with a fixed shell). The other thing to consider is what brought this topic up - a question of whether plugins/sh/eval.sh is worth a conversion into a standalone plugin. With other languages, our choice is between: nbdkit python script ./script (where #! reinvokes nbdkit python script) But we don't have a way to write: nbdkit script where nbdkit recognizes that 'script' is not a .so file, but IS a file that calls out python in its #!, and therefore attempts to load the python.so plugin. The question with the eval.sh plugin is if we can write: nbdkit eval pread=... and have nbdkit figure out that 'eval' is not a plugin but IS a file with a #! calling out 'nbdkit sh', and therefore load the 'sh' plugin with 'script=eval'. In other words, maybe it's time to teach nbdkit general magic on how to handle the plugin argument (if an .so is found then load it; if not, then inspect the file to see which .so to use instead). Or maybe we just write a C form of a plugin named 'eval' that does the same things as what 'sh eval.sh' does, sharing as much code as possible, but where you have no shebang form because you never pass a script= argument to the eval plugin. All interesting thoughts, but I'm not sure I got us any closer to a nice conclusion. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
On Fri, Nov 22, 2019 at 02:55:31PM -0600, Eric Blake wrote:
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).
I couldn't work out how to do this, plus we have the callback_defined function which makes this easy, so yes, I did the easy thing :-)
@@ -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?
While I'm no Python programmer, it does look as if existing Python core APIs are happy to use bitmasks, eg: https://docs.python.org/3/library/os.html#os.open and this is also easy to implement and keeps it similar to the C API.
- 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?
I think it would be better if we simply referred back to the C documentation to avoid duplication. Also an advantage of using bitmasks. That does require a rather larger change to the documentation though.
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).
Is that true? The plugin asserts that it wants the v1 API, but we are still using the v2 C API, whatever the plugin asks for? I'm cautious about adding asserts unless they can never happen now or in the future, because we don't really want an assert() happening on a customer site running virt-v2v with the rhv-upload Python plugin. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-df lists disk usage of guests without needing to install any software inside the virtual machine. Supports Linux and Windows. http://people.redhat.com/~rjones/virt-df/
On Fri, Nov 22, 2019 at 11:14 PM Richard W.M. Jones <rjones@redhat.com> wrote:
On Fri, Nov 22, 2019 at 02:55:31PM -0600, Eric Blake wrote:
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).
I couldn't work out how to do this, plus we have the callback_defined function which makes this easy, so yes, I did the easy thing :-)
@@ -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?
While I'm no Python programmer, it does look as if existing Python core APIs are happy to use bitmasks, eg:
https://docs.python.org/3/library/os.html#os.open
and this is also easy to implement and keeps it similar to the C API.
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 3:14 PM, Richard W.M. Jones wrote:
@@ -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?
While I'm no Python programmer, it does look as if existing Python core APIs are happy to use bitmasks, eg:
https://docs.python.org/3/library/os.html#os.open
and this is also easy to implement and keeps it similar to the C API.
Fair enough. I'm just making sure that our Python interface is at least idiomatic to a Python programmer, rather than blatantly being a naive translation of a C programmer.
- 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?
I think it would be better if we simply referred back to the C documentation to avoid duplication. Also an advantage of using bitmasks. That does require a rather larger change to the documentation though.
Pointing to the C docs, and focusing on just the bindings-induced differences, is fine.
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).
Is that true? The plugin asserts that it wants the v1 API, but we are still using the v2 C API, whatever the plugin asks for?
Hmm. If the user implements a 'can_fua' callback that returns 1 even though they forgot to declare 'api_version', then the flag can indeed be set. Perhaps that means our 'can_fua' C wrapper should have a version 1/2 difference in behavior (in v1, raise an error reminding the user to fix their plugin to declare 'api_version', in v2 pass back the result), so that we could indeed then use the assert with a guarantee that it will not trigger on an arbitrary plugin.
I'm cautious about adding asserts unless they can never happen now or in the future, because we don't really want an assert() happening on a customer site running virt-v2v with the rhv-upload Python plugin.
Understood. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
On 11/22/19 3:30 PM, Eric Blake wrote:
+ 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).
Is that true? The plugin asserts that it wants the v1 API, but we are still using the v2 C API, whatever the plugin asks for?
Hmm. If the user implements a 'can_fua' callback that returns 1 even though they forgot to declare 'api_version', then the flag can indeed be set. Perhaps that means our 'can_fua' C wrapper should have a version 1/2 difference in behavior (in v1, raise an error reminding the user to fix their plugin to declare 'api_version', in v2 pass back the result), so that we could indeed then use the assert with a guarantee that it will not trigger on an arbitrary plugin.
In fact, that would be similar to other things we've had to open-code in python.c to mirror what plugins.c does automatically: basically, plugins.c has: /* The plugin must use API version 2 and have .can_fua return NBDKIT_FUA_NATIVE before we will pass the FUA flag on. */ if (p->plugin.can_fua) { r = p->plugin.can_fua (handle); if (r > NBDKIT_FUA_EMULATE && p->plugin._api_version == 1) r = NBDKIT_FUA_EMULATE; return r; and so python.c has to have the same logic when dealing with a v1 plugin but exposing the v2 interface back to nbdkit. Compare to things like py_can_write(), which has: /* No Python can_write callback, but there's a Python pwrite callback * defined, so return 1. (In C modules, nbdkit would do this). */ else if (callback_defined ("pwrite", NULL)) return 1; for a similar duplication in logic to what plugins.c can do. I suspect I'll run into this later in the series. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
On Fri, Nov 22, 2019 at 10:55 PM Eric Blake <eblake@redhat.com> wrote:
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 is the same thing I was thinking about. This makes it more clear that the api version is constant, and not something the plugin should change while it is being used. Same for all can_xxx functions, NBD does not support changing any of these anyway after negotiation. v2 is an opportunity to do this change if we want to. ...
-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
I was going to comment about the same thing.
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):
This is nicer - but not extensible.
The user could also write:
def zero(h, count, offset, **flags)
This is less nice, but easy to extend without adding v3. But I would call it **options instead.
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)
... Nir
On 11/22/19 3:20 PM, Nir Soffer wrote:
+# 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 is the same thing I was thinking about. This makes it more clear that the api version is constant, and not something the plugin should change while it is being used.
Hmm - api_version() really is constant for the entire life of nbdkit. We call it exactly once. Figuring out how to read a Python variable instead of calling a function would be slightly more in line with the fact that in C code it is a #define constant rather than a function pointer callback. But it is that much more glue code to figure out how to check for a python global variable, compared to the glue code we already have for calling a python function.
Same for all can_xxx functions, NBD does not support changing any of these anyway after negotiation.
While can_xxx functions are somewhat dynamic (we only call them once per connection, but connection A can be readonly while connection B is read-write, changing the can_write result, for example). So those have to remain functions.
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):
This is nicer - but not extensible.
Why not? Any future flag additions would still appear as new key=value kwargs.
The user could also write:
def zero(h, count, offset, **flags)
This is less nice, but easy to extend without adding v3. But I would call it **options instead.
I really need to post my patches where I played with kwargs, for contrast to this version. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
On Fri, Nov 22, 2019 at 11:35 PM Eric Blake <eblake@redhat.com> wrote:
On 11/22/19 3:20 PM, Nir Soffer wrote:
+# 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 is the same thing I was thinking about. This makes it more clear that the api version is constant, and not something the plugin should change while it is being used.
Hmm - api_version() really is constant for the entire life of nbdkit. We call it exactly once. Figuring out how to read a Python variable instead of calling a function would be slightly more in line with the fact that in C code it is a #define constant rather than a function pointer callback. But it is that much more glue code to figure out how to check for a python global variable, compared to the glue code we already have for calling a python function.
Same for all can_xxx functions, NBD does not support changing any of these anyway after negotiation.
While can_xxx functions are somewhat dynamic (we only call them once per connection, but connection A can be readonly while connection B is read-write, changing the can_write result, for example). So those have to remain functions.
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):
This is nicer - but not extensible.
Why not? Any future flag additions would still appear as new key=value kwargs.
Because there is no **kwargs argument, you will get TypeError when adding new argument. We will need new api version whenever we add new argument. This is extensible: def zero(h, count, offset, may_trim=False, fua=False, fast=False, **kwargs): Nir
On 11/22/19 3:46 PM, Nir Soffer wrote:
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):
This is nicer - but not extensible.
Why not? Any future flag additions would still appear as new key=value kwargs.
Because there is no **kwargs argument, you will get TypeError when adding new argument. We will need new api version whenever we add new argument.
Not if you use PyObject_Call(fn, args, kwargs) or similar to pass 'may_trim=False' as a kwarg instead of a positional. As long as new flags are always passed via keyword instead of position, and we don't pass any keywords that the plugin has not opted into, then we can add new keywords and call plugins with fewer flags than what our current nbdkit supports.
This is extensible:
def zero(h, count, offset, may_trim=False, fua=False, fast=False, **kwargs):
But that is also called via PyObject_Call(fn, args, kwargs) - as long as we pass kwargs correctly, the C code doesn't care whether the Python definition has a ** catchall or not, because the rules for translating the C code's two lists (one of positional, the other of kwargs) into the Python signature work for multiple different python signatures. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
On Fri, Nov 22, 2019 at 11:55 PM Eric Blake <eblake@redhat.com> wrote:
On 11/22/19 3:46 PM, Nir Soffer wrote:
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):
This is nicer - but not extensible.
Why not? Any future flag additions would still appear as new key=value kwargs.
Because there is no **kwargs argument, you will get TypeError when adding new argument. We will need new api version whenever we add new argument.
Not if you use PyObject_Call(fn, args, kwargs) or similar to pass 'may_trim=False' as a kwarg instead of a positional. As long as new flags are always passed via keyword instead of position, and we don't pass any keywords that the plugin has not opted into, then we can add new keywords and call plugins with fewer flags than what our current nbdkit supports.
This is extensible:
def zero(h, count, offset, may_trim=False, fua=False, fast=False, **kwargs):
But that is also called via PyObject_Call(fn, args, kwargs) - as long as we pass kwargs correctly, the C code doesn't care whether the Python definition has a ** catchall or not, because the rules for translating the C code's two lists (one of positional, the other of kwargs) into the Python signature work for multiple different python signatures.
I see, this will work and is extensible, but it may be harder to write correct plugin code. You may forget to define can_foo() and then define foo=None or define can_foo() and forget to define the foo argument. Nir
On Fri, Nov 22, 2019 at 11:35 PM Eric Blake <eblake@redhat.com> wrote:
On 11/22/19 3:20 PM, Nir Soffer wrote:
+# 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 is the same thing I was thinking about. This makes it more clear that the api version is constant, and not something the plugin should change while it is being used.
Hmm - api_version() really is constant for the entire life of nbdkit. We call it exactly once. Figuring out how to read a Python variable instead of calling a function would be slightly more in line with the fact that in C code it is a #define constant rather than a function pointer callback. But it is that much more glue code to figure out how to check for a python global variable, compared to the glue code we already have for calling a python function.
The extra glue code is (without error handling): PyObject *d = PyModule_GetDict(PyObject *module); PyObject *v = PyDict_GetItemString(d, "API_VERSION"); long api_version = PyLong_AsLong(v);
On Fri, Nov 22, 2019 at 11:53 PM Nir Soffer <nsoffer@redhat.com> wrote:
On Fri, Nov 22, 2019 at 11:35 PM Eric Blake <eblake@redhat.com> wrote:
On 11/22/19 3:20 PM, Nir Soffer wrote:
+# 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 is the same thing I was thinking about. This makes it more clear that the api version is constant, and not something the plugin should change while it is being used.
Hmm - api_version() really is constant for the entire life of nbdkit. We call it exactly once. Figuring out how to read a Python variable instead of calling a function would be slightly more in line with the fact that in C code it is a #define constant rather than a function pointer callback. But it is that much more glue code to figure out how to check for a python global variable, compared to the glue code we already have for calling a python function.
The extra glue code is (without error handling):
PyObject *d = PyModule_GetDict(PyObject *module); PyObject *v = PyDict_GetItemString(d, "API_VERSION"); long api_version = PyLong_AsLong(v);
Or simpler (with error handling): PyObject *v = PyObject_GetAttrString(m, "API_VERSION"); if (v == NULL) return 1; long value = PyLong_AsLong(v); Py_DECREF(m); return value; On error -1 is returned and PyErr_Occurred() will return the exception type.
On Sat, Nov 23, 2019 at 12:30:38AM +0200, Nir Soffer wrote:
On Fri, Nov 22, 2019 at 11:53 PM Nir Soffer <nsoffer@redhat.com> wrote:
On Fri, Nov 22, 2019 at 11:35 PM Eric Blake <eblake@redhat.com> wrote:
On 11/22/19 3:20 PM, Nir Soffer wrote:
+# 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 is the same thing I was thinking about. This makes it more clear that the api version is constant, and not something the plugin should change while it is being used.
Hmm - api_version() really is constant for the entire life of nbdkit. We call it exactly once. Figuring out how to read a Python variable instead of calling a function would be slightly more in line with the fact that in C code it is a #define constant rather than a function pointer callback. But it is that much more glue code to figure out how to check for a python global variable, compared to the glue code we already have for calling a python function.
The extra glue code is (without error handling):
PyObject *d = PyModule_GetDict(PyObject *module); PyObject *v = PyDict_GetItemString(d, "API_VERSION"); long api_version = PyLong_AsLong(v);
Or simpler (with error handling):
PyObject *v = PyObject_GetAttrString(m, "API_VERSION"); if (v == NULL) return 1;
long value = PyLong_AsLong(v); Py_DECREF(m);
return value;
On error -1 is returned and PyErr_Occurred() will return the exception type.
Thanks, I'll change this as suggested above. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-df lists disk usage of guests without needing to install any software inside the virtual machine. Supports Linux and Windows. http://people.redhat.com/~rjones/virt-df/
--- 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
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
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
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 Fri, Nov 22, 2019 at 03:46:56PM -0600, Eric Blake wrote:
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.
I was going to say because it matches the order of the C documentation, but in fact it does not. We do really need to settle on a single ordering across all documentation, but that's for another day ... Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-top is 'top' for virtual machines. Tiny program with many powerful monitoring features, net stats, disk stats, logging, etc. http://people.redhat.com/~rjones/virt-top
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
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
--- 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: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 Sat, Nov 23, 2019 at 1:42 AM Nir Soffer <nsoffer@redhat.com> wrote:
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.
This patch adds support for buffer protocol: https://www.redhat.com/archives/libguestfs/2019-November/msg00200.html (I forgot to add nbdkit to the title)
+ +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 Sat, Nov 23, 2019 at 01:42:15AM +0200, Nir Soffer wrote:
On Fri, Nov 22, 2019 at 9:55 PM Richard W.M. Jones <rjones@redhat.com> wrote:
+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.
I thought bytearray & bytes were the same thing :-/ Would adding this break existing nbdkit Python scripts? Should this be considered for v2 of the API? Are there performance implications / improvements from doing this?
test-python-plugin.py
This text can use python docstring:
""" This tests ... """
Good point, I'll fix this. We could probably also have nbdkit do something with the docstring, such as printing it in --help output, although that's something for another patch series.
+ h = nbd.NBD () + cfg = codecs.encode (pickle.dumps (cfg), "base64").decode()
base64.b64encode() is better, avoiding unwanted newlines.
Ah OK, I originally added strip(), but this is better.
+ 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. [...] pytest test-python.py
I'll probably use unittest though because it's built into Python and because it's what we use in libguestfs, hivex etc but yes good idea. Thanks, Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com Fedora Windows cross-compiler. Compile Windows programs, test, and build Windows installers. Over 100 libraries supported. http://fedoraproject.org/wiki/MinGW
On Sat, Nov 23, 2019 at 3:10 PM Richard W.M. Jones <rjones@redhat.com> wrote:
On Sat, Nov 23, 2019 at 01:42:15AM +0200, Nir Soffer wrote:
On Fri, Nov 22, 2019 at 9:55 PM Richard W.M. Jones <rjones@redhat.com> wrote:
+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.
I thought bytearray & bytes were the same thing :-/
Would adding this break existing nbdkit Python scripts? Should this be considered for v2 of the API? Are there performance implications / improvements from doing this?
There is performance implication for v1 plugins like rhv-upload-plugin that need to copy the bytes from imageio server on python side: r = http.getresponse() ... return bytearray(r.read()) This is sad because on the C side we mempcpy the data again. So with this patch we avoid one copy of the two. To avoid all unneeded copies, we need to change pread() to: def pread(h, buf, offset): So the python side we can do: f.readinto(buf) Or: sock.recv_info(buf) It does not work for HTTPResponse, so in this case we have to do: buf[:] = r.read() Since we work on v2 now, I think we should consider this change. An uglier alternative is: def preadinto(h, buf, offset): Matching python read() and readinto() interface.
test-python-plugin.py
This text can use python docstring:
""" This tests ... """
Good point, I'll fix this. We could probably also have nbdkit do something with the docstring, such as printing it in --help output, although that's something for another patch series.
+ h = nbd.NBD () + cfg = codecs.encode (pickle.dumps (cfg), "base64").decode()
base64.b64encode() is better, avoiding unwanted newlines.
Ah OK, I originally added strip(), but this is better.
+ 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. [...] pytest test-python.py
I'll probably use unittest though because it's built into Python and because it's what we use in libguestfs, hivex etc but yes good idea.
Thanks,
Rich.
-- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com Fedora Windows cross-compiler. Compile Windows programs, test, and build Windows installers. Over 100 libraries supported. http://fedoraproject.org/wiki/MinGW
On Sat, Nov 23, 2019 at 06:11:47PM +0200, Nir Soffer wrote:
On Sat, Nov 23, 2019 at 3:10 PM Richard W.M. Jones <rjones@redhat.com> wrote:
On Sat, Nov 23, 2019 at 01:42:15AM +0200, Nir Soffer wrote:
On Fri, Nov 22, 2019 at 9:55 PM Richard W.M. Jones <rjones@redhat.com> wrote:
+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.
I thought bytearray & bytes were the same thing :-/
Would adding this break existing nbdkit Python scripts? Should this be considered for v2 of the API? Are there performance implications / improvements from doing this?
There is performance implication for v1 plugins like rhv-upload-plugin that need to copy the bytes from imageio server on python side:
r = http.getresponse() ... return bytearray(r.read())
This is sad because on the C side we mempcpy the data again. So with this patch we avoid one copy of the two.
To avoid all unneeded copies, we need to change pread() to:
def pread(h, buf, offset):
So the python side we can do:
f.readinto(buf)
Or:
sock.recv_info(buf)
It does not work for HTTPResponse, so in this case we have to do:
buf[:] = r.read()
Since we work on v2 now, I think we should consider this change.
Right, we can consider this for v2, while leaving v1 callers alone.
An uglier alternative is:
def preadinto(h, buf, offset):
Matching python read() and readinto() interface.
Is this different somehow from def pread(h, buf, offset) defined above? Rich.
test-python-plugin.py
This text can use python docstring:
""" This tests ... """
Good point, I'll fix this. We could probably also have nbdkit do something with the docstring, such as printing it in --help output, although that's something for another patch series.
+ h = nbd.NBD () + cfg = codecs.encode (pickle.dumps (cfg), "base64").decode()
base64.b64encode() is better, avoiding unwanted newlines.
Ah OK, I originally added strip(), but this is better.
+ 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. [...] pytest test-python.py
I'll probably use unittest though because it's built into Python and because it's what we use in libguestfs, hivex etc but yes good idea.
Thanks,
Rich.
-- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com Fedora Windows cross-compiler. Compile Windows programs, test, and build Windows installers. Over 100 libraries supported. http://fedoraproject.org/wiki/MinGW
-- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com libguestfs lets you edit virtual machines. Supports shell scripting, bindings from many languages. http://libguestfs.org
On Sun, Nov 24, 2019, 12:42 Richard W.M. Jones <rjones@redhat.com> wrote:
On Sat, Nov 23, 2019 at 3:10 PM Richard W.M. Jones <rjones@redhat.com> wrote:
On Sat, Nov 23, 2019 at 01:42:15AM +0200, Nir Soffer wrote:
On Fri, Nov 22, 2019 at 9:55 PM Richard W.M. Jones <
rjones@redhat.com> wrote:
+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
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.
I thought bytearray & bytes were the same thing :-/
Would adding this break existing nbdkit Python scripts? Should this be considered for v2 of the API? Are there performance implications / improvements from doing this?
There is performance implication for v1 plugins like rhv-upload-plugin
need to copy the bytes from imageio server on python side:
r = http.getresponse() ... return bytearray(r.read())
This is sad because on the C side we mempcpy the data again. So with
On Sat, Nov 23, 2019 at 06:11:47PM +0200, Nir Soffer wrote: python plugin that this patch
we avoid one copy of the two.
To avoid all unneeded copies, we need to change pread() to:
def pread(h, buf, offset):
So the python side we can do:
f.readinto(buf)
Or:
sock.recv_info(buf)
It does not work for HTTPResponse, so in this case we have to do:
buf[:] = r.read()
Since we work on v2 now, I think we should consider this change.
Right, we can consider this for v2, while leaving v1 callers alone.
An uglier alternative is:
def preadinto(h, buf, offset):
Matching python read() and readinto() interface.
Is this different somehow from def pread(h, buf, offset) defined above?
We keep nicer pread for compatibility and add preadinto for people that care about performence. This is the current approach in the standard library.
Rich.
test-python-plugin.py
This text can use python docstring:
""" This tests ... """
Good point, I'll fix this. We could probably also have nbdkit do something with the docstring, such as printing it in --help output, although that's something for another patch series.
+ h = nbd.NBD () + cfg = codecs.encode (pickle.dumps (cfg), "base64").decode()
base64.b64encode() is better, avoiding unwanted newlines.
Ah OK, I originally added strip(), but this is better.
+ 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. [...] pytest test-python.py
I'll probably use unittest though because it's built into Python and because it's what we use in libguestfs, hivex etc but yes good idea.
Thanks,
Rich.
-- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com Fedora Windows cross-compiler. Compile Windows programs, test, and build Windows installers. Over 100 libraries supported. http://fedoraproject.org/wiki/MinGW
-- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com libguestfs lets you edit virtual machines. Supports shell scripting, bindings from many languages. http://libguestfs.org
There is performance implication for v1 plugins like rhv-upload-plugin
need to copy the bytes from imageio server on python side:
r = http.getresponse() ... return bytearray(r.read())
This is sad because on the C side we mempcpy the data again. So with
On Sat, Nov 23, 2019 at 06:11:47PM +0200, Nir Soffer wrote: that this patch
we avoid one copy of the two.
To avoid all unneeded copies, we need to change pread() to:
def pread(h, buf, offset):
So the python side we can do:
f.readinto(buf)
Or:
sock.recv_info(buf)
It does not work for HTTPResponse, so in this case we have to do:
buf[:] = r.read()
Since we work on v2 now, I think we should consider this change.
Right, we can consider this for v2, while leaving v1 callers alone.
An uglier alternative is:
def preadinto(h, buf, offset):
Matching python read() and readinto() interface.
So if I'm understand this correctly, is it true that Python would be writing directly to the C buffer allocated by nbdkit? This sounds like it would be beneficial. Would you like to do a patch for this? For v2 it's fine to just have: def pread(h, buf, offset) since that is the same as the C API. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-df lists disk usage of guests without needing to install any software inside the virtual machine. Supports Linux and Windows. http://people.redhat.com/~rjones/virt-df/
participants (3)
-
Eric Blake -
Nir Soffer -
Richard W.M. Jones