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

[Nir Soffer]

On Fri, Nov 22, 2019 at 10:55 PM Eric Blake <eblake at 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