Eric Blake
2019-Nov-22 21:35 UTC
Re: [Libguestfs] [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
Nir Soffer
2019-Nov-22 21:46 UTC
Re: [Libguestfs] [PATCH nbdkit v2 03/10] python: Implement nbdkit API version 2.
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
Nir Soffer
2019-Nov-22 21:53 UTC
Re: [Libguestfs] [PATCH nbdkit v2 03/10] python: Implement nbdkit API version 2.
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);
Eric Blake
2019-Nov-22 21:55 UTC
Re: [Libguestfs] [PATCH nbdkit v2 03/10] python: Implement nbdkit API version 2.
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
Nir Soffer
2019-Nov-22 22:30 UTC
Re: [Libguestfs] [PATCH nbdkit v2 03/10] python: Implement nbdkit API version 2.
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.
Apparently Analagous Threads
- Re: [PATCH nbdkit v2 03/10] python: Implement nbdkit API version 2.
- Re: [PATCH nbdkit v2 03/10] python: Implement nbdkit API version 2.
- Re: [PATCH nbdkit v2 03/10] python: Implement nbdkit API version 2.
- Re: [PATCH nbdkit v2 03/10] python: Implement nbdkit API version 2.
- [PATCH nbdkit v2 03/10] python: Implement nbdkit API version 2.