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

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