[Libguestfs] [PATCH nbdkit v2 03/10] python: Implement nbdkit API version 2.

[Eric Blake]

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