Libguestfs - Nov 2019 - Re: Extending the nbdkit python plugin

We have an nbdkit plugin that lets you write NBD servers in Python.
An example of an existing Python plugin is here:

  https://github.com/libguestfs/nbdkit/blob/master/plugins/python/example.py#L1

This morning I tried to modify the plugin to use the newer nbdkit API
(version 2).  One of the things that would change would be passing
flags parameters to some functions, eg:

  def pwrite (h, buf, offset):

might become one of these possibilities (where flags is a bitmask):

  def pwrite (h, buf, offset, flags):
  def pwrite (h, buf, offset, flags=0):

The problem is if we did this it would break all existing Python
plugins.  While we don't guarantee the nbdkit API for non-C languages,
we do nevertheless have Python plugins that we care about such as the
rhv-upload-plugin used by virt-v2v.  Having a flag day which breaks
all existing plugins is very awkward.

I tried to simply pass the extra arguments from the C code to the
Python code, and existing plugins break with:

nbdkit: python[1]: error: ./test.py: pwrite: error: pwrite() takes 3 positional
arguments but 4 were given

One possibility is that we could introspect the plugin to find out how
many parameters it takes.  This is possible, but very difficult from
C.  (See https://stackoverflow.com/a/41188411 for how to do it from
Python).

Another possibility is we could encourage existing Python plugins to
add **kwargs to all functions where we might plausibly add extra
parameters in future, ie. the above would become:

  def pwrite (h, buf, offset, **kwargs):

This still requires all Python plugins to change, but at least they
would remain backwards compatible with old and new nbdkit.  However I
couldn't actually work out how to make this work because:

  >>> def test(a, **kwargs):
  ...   pass
  ... 
  >>> test(1)
  >>> test(1,2)
  Traceback (most recent call last):
    File "<stdin>", line 1, in <module>
  TypeError: test() takes 1 positional argument but 2 were given

Yet another possibility is the Python plugin itself should declare
which version of the API it wants, and the C code can then pass the
correct parameters.  (This is in fact how nbdkit C plugins work).
This pushes a bunch of work into the C code, but I guess we can deal
with that.

So really this is a question directed at Python experts.  What's the
right way to go about this?

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/21/19 7:57 AM, Richard W.M. Jones wrote:
> We have an nbdkit plugin that lets you write NBD servers in Python.
> An example of an existing Python plugin is here:
> 
>   
https://github.com/libguestfs/nbdkit/blob/master/plugins/python/example.py#L1
> 
> This morning I tried to modify the plugin to use the newer nbdkit API
> (version 2).  One of the things that would change would be passing
> flags parameters to some functions, eg:
> 
>    def pwrite (h, buf, offset):
> 
> might become one of these possibilities (where flags is a bitmask):
> 
>    def pwrite (h, buf, offset, flags):
>    def pwrite (h, buf, offset, flags=0):
> 
> The problem is if we did this it would break all existing Python
> plugins.  While we don't guarantee the nbdkit API for non-C languages,
> we do nevertheless have Python plugins that we care about such as the
> rhv-upload-plugin used by virt-v2v.  Having a flag day which breaks
> all existing plugins is very awkward.
At one point, I tried using Python introspection to determine which 
signature the user's function has, to determine if the user wrote 
against the v1 protocol. It may also be possible to use python's keyword 
argument parsing to pass all optional arguments like flags by key=value 
pairs (allowing future addition of additional flags).  I'll have to 
revive my patches.
> 
> I tried to simply pass the extra arguments from the C code to the
> Python code, and existing plugins break with:
> 
> nbdkit: python[1]: error: ./test.py: pwrite: error: pwrite() takes 3
positional arguments but 4 were given
> 
> One possibility is that we could introspect the plugin to find out how
> many parameters it takes.  This is possible, but very difficult from
> C.  (See https://stackoverflow.com/a/41188411 for how to do it from
> Python).
> 
> Another possibility is we could encourage existing Python plugins to
> add **kwargs to all functions where we might plausibly add extra
> parameters in future, ie. the above would become:
> 
>    def pwrite (h, buf, offset, **kwargs):
> 
> This still requires all Python plugins to change, but at least they
> would remain backwards compatible with old and new nbdkit.
Yes, that's along the lines of the work I had attempted previously.  But 
I attempted it back when we were using Python2, so I'd have to rework it 
for Python3.
>  However I
> couldn't actually work out how to make this work because:
> 
>    >>> def test(a, **kwargs):
>    ...   pass
>    ...
>    >>> test(1)
>    >>> test(1,2)
>    Traceback (most recent call last):
>      File "<stdin>", line 1, in <module>
>    TypeError: test() takes 1 positional argument but 2 were given
> 
> Yet another possibility is the Python plugin itself should declare
> which version of the API it wants, and the C code can then pass the
> correct parameters.  (This is in fact how nbdkit C plugins work).
> This pushes a bunch of work into the C code, but I guess we can deal
> with that.
> 
> So really this is a question directed at Python experts.  What's the
> right way to go about this?
I'll try and revive my patches; I may be able to save you some time 
based on what I already have in my tree.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

On Thu, Nov 21, 2019 at 01:57:28PM +0000, Richard W.M. Jones
wrote:
> We have an nbdkit plugin that lets you write NBD servers in Python.
> An example of an existing Python plugin is here:
> 
>  
https://github.com/libguestfs/nbdkit/blob/master/plugins/python/example.py#L1
> 
> This morning I tried to modify the plugin to use the newer nbdkit API
> (version 2).  One of the things that would change would be passing
> flags parameters to some functions, eg:
> 
>   def pwrite (h, buf, offset):
> 
> might become one of these possibilities (where flags is a bitmask):
> 
>   def pwrite (h, buf, offset, flags):
>   def pwrite (h, buf, offset, flags=0):
> 
> The problem is if we did this it would break all existing Python
> plugins.  While we don't guarantee the nbdkit API for non-C languages,
> we do nevertheless have Python plugins that we care about such as the
> rhv-upload-plugin used by virt-v2v.  Having a flag day which breaks
> all existing plugins is very awkward.
> 
> I tried to simply pass the extra arguments from the C code to the
> Python code, and existing plugins break with:
> 
> nbdkit: python[1]: error: ./test.py: pwrite: error: pwrite() takes 3
positional arguments but 4 were given
> 
> One possibility is that we could introspect the plugin to find out how
> many parameters it takes.  This is possible, but very difficult from
> C.  (See https://stackoverflow.com/a/41188411 for how to do it from
> Python).
You can do the introspection in python instead of C.

Instead of having the C code directly call the python plugin code,
write a shim plugin impl that uses the new API contract. This shim
python plugin then introspects the real python plugin and calls it
with the appropriate API contract (if possible).
> Another possibility is we could encourage existing Python plugins to
> add **kwargs to all functions where we might plausibly add extra
> parameters in future, ie. the above would become:
> 
>   def pwrite (h, buf, offset, **kwargs):
> 
> This still requires all Python plugins to change, but at least they
> would remain backwards compatible with old and new nbdkit.  However I
> couldn't actually work out how to make this work because:
> 
>   >>> def test(a, **kwargs):
>   ...   pass
>   ... 
>   >>> test(1)
>   >>> test(1,2)
>   Traceback (most recent call last):
>     File "<stdin>", line 1, in <module>
>   TypeError: test() takes 1 positional argument but 2 were given
> 
> Yet another possibility is the Python plugin itself should declare
> which version of the API it wants, and the C code can then pass the
> correct parameters.  (This is in fact how nbdkit C plugins work).
> This pushes a bunch of work into the C code, but I guess we can deal
> with that.
I would define a new 'api_version()' method for your python
plugins.

Existing plugins don't implement that of course so when your
C code invokes that it'll get an exception, and can assume
the classic API contract for pwrite().

If the plugin implements  'api_version()' returning '1' (or
whatever), then this should inform the C code that the plugin
has the 4 arg variant of pwrite().

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

On Thu, Nov 21, 2019 at 02:17:43PM +0000, Daniel P. Berrangé
wrote:
> I would define a new 'api_version()' method for your python
> plugins.
> 
> Existing plugins don't implement that of course so when your
> C code invokes that it'll get an exception, and can assume
> the classic API contract for pwrite().
> 
> If the plugin implements  'api_version()' returning '1' (or
> whatever), then this should inform the C code that the plugin
> has the 4 arg variant of pwrite().
I suspect this has to be the way to go.  One reason is that
the V1 zero API is defined as:

  def zero(h, count, offset, may_trim=False):

but in V2 we'd like to replace the fourth parameter with a flags
bitmask.  I believe pure introspection can never detect this.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-builder quickly builds VMs from scratch
http://libguestfs.org/virt-builder.1.html

This may be confusing at first but is not that difficult once you wrap
your head around it.

There are two types of arguments in Python: positional(*) and
keyword(**). There is a rule that all positional arguments are defined
before the keyword arguments. Also once an argument has a default value
it is considered keyword (not positional). There are some nuances for
which is best to reach to some documentation.

> Another possibility is we could encourage existing Python plugins to
> add **kwargs to all functions where we might plausibly add extra
> parameters in future, ie. the above would become:
> 
>   def pwrite (h, buf, offset, **kwargs):
> 
> This still requires all Python plugins to change, but at least they
> would remain backwards compatible with old and new nbdkit.  However I
> couldn't actually work out how to make this work because:
> 
>   >>> def test(a, **kwargs):
Here you defined single positional argument 'a'.
>   ...   pass
>   ... 
>   >>> test(1)
>   >>> test(1,2)
You are passing two positional arguments. A keyword argument has to have
a name. E.g. this will work: test(1, foo=2)
>   Traceback (most recent call last):
>     File "<stdin>", line 1, in <module>
>   TypeError: test() takes 1 positional argument but 2 were given
> 
Check the attached script for some examples. Hopefully that will help
you get a better grasp.

    Tomas

-- 
Tomáš Golembiovský <tgolembi@redhat.com>

On Thu, Nov 21, 2019 at 04:49:00PM +0100, Tomáš Golembiovský
wrote:
> This may be confusing at first but is not that difficult once you wrap
> your head around it.
> 
> There are two types of arguments in Python: positional(*) and
> keyword(**). There is a rule that all positional arguments are defined
> before the keyword arguments. Also once an argument has a default value
> it is considered keyword (not positional). There are some nuances for
> which is best to reach to some documentation.
Thanks for the explanation.  I think the api_version idea is the way
to go, I have a patch for this coming up.

Rich.
> 
> > Another possibility is we could encourage existing Python plugins to
> > add **kwargs to all functions where we might plausibly add extra
> > parameters in future, ie. the above would become:
> > 
> >   def pwrite (h, buf, offset, **kwargs):
> > 
> > This still requires all Python plugins to change, but at least they
> > would remain backwards compatible with old and new nbdkit.  However I
> > couldn't actually work out how to make this work because:
> > 
> >   >>> def test(a, **kwargs):
> 
> Here you defined single positional argument 'a'.
> 
> >   ...   pass
> >   ... 
> >   >>> test(1)
> >   >>> test(1,2)
> 
> You are passing two positional arguments. A keyword argument has to have
> a name. E.g. this will work: test(1, foo=2)
> 
> >   Traceback (most recent call last):
> >     File "<stdin>", line 1, in <module>
> >   TypeError: test() takes 1 positional argument but 2 were given
> > 
> 
> Check the attached script for some examples. Hopefully that will help
> you get a better grasp.
> 
>     Tomas
> 
> -- 
> Tomáš Golembiovský <tgolembi@redhat.com>
> #!/usr/bin/env python3
> 
> def abc(*args, **kwargs):
>     print(args)
>     print(kwargs)
> 
> def with_positional(a, b, c, *args, **kwargs):
>     print(a, b, c)
>     print(args)
>     print(kwargs)
> 
> # Wrong: cannot define keyword arguments before positional
> #def with_keyword(a=None, b=None, c=None, *args, **kwargs):
> #    pass
> 
> def with_keyword(*args, a=None, b=None, c=None, **kwargs):
>     print(a, b, c)
>     print(args)
>     print(kwargs)
> 
> # This is also possible
> def with_keyword2(*args, a, b, c, **kwargs):
>     pass
>     print(a, b, c)
>     print(args)
>     print(kwargs)
> 
> # (1, 'abc')
> # {'foo': 2, 'bar': 3}
> abc(1, 'abc', foo=2, bar=3)
> # Wrong: cannot pass positional arguments after keyword arguments
> #abc(1, 'abc', foo=2, bar=3, 4, 5)
> print('---')
> 
> # 1 2 3
> # (4, 5)
> # {'foo': 10, 'bar': 11}
> with_positional(1, 2, 3, 4, 5, foo=10, bar=11)
> # Wrong: a,b,c are alrady bound to 4,5,6
> #with_positional(4, 5, 6, foo=10, bar=11, a=1, b=2, c=3)
> 
> # This works as one would expect
> with_positional(a=1, b=2, c=3, foo=10, bar=11)
> # These two are wrong
> #with_positional(4, a=1, b=2, c=3, foo=10, bar=11)
> #with_positional(a=1, b=2, c=3, 4, foo=10, bar=11)
> 
> print('---')
> with_keyword(4, 5, foo=10, bar=11)
> with_keyword(4, 5, foo=10, bar=11, a=1, b=2, c=3)
> 
> # These two are equivalent
> with_keyword2(foo=10, bar=11, a=1, b=2, c=3)
> with_keyword2(a=1, b=2, c=3, foo=10, bar=11)

-- 
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