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

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

On Fri, Nov 22, 2019 at 02:55:31PM -0600, Eric Blake
wrote:
> 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]
I believe it's not possible to do it for sh plugins.

For (eg) python plugins it works like this:

  #!/usr/sbin/nbdkit python
  -> runs nbdkit python <script name>
  -> <script name> is interpreted as magic script= parameter
  -> the python plugin works by loading the script using
     PyRun_SimpleFileEx which interprets the contents of the
     script as python code

However for shell it doesn't work:

  #!/usr/sbin/nbdkit sh
  -> runs nbdkit sh <script name>
  -> <script name> is interpreted as magic script= parameter
  -> the sh plugin works by actually executing the script
  -> executing the script repeats the steps over from the top,
     causing an infinite loop

If you can think of a way to make this work it would be a useful
feature IMO.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
Fedora Windows cross-compiler. Compile Windows programs, test, and
build Windows installers. Over 100 libraries supported.
http://fedoraproject.org/wiki/MinGW

On Fri, Nov 22, 2019 at 02:55:31PM -0600, Eric Blake
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).
I couldn't work out how to do this, plus we have the callback_defined
function which makes this easy, so yes, I did the easy thing :-)
> >@@ -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?
While I'm no Python programmer, it does look as if existing Python
core APIs are happy to use bitmasks, eg:

  https://docs.python.org/3/library/os.html#os.open

and this is also easy to implement and keeps it similar to the C API.
> >- 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?
I think it would be better if we simply referred back to the C
documentation to avoid duplication.  Also an advantage of using
bitmasks.  That does require a rather larger change to the
documentation though.
> >  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).
Is that true?  The plugin asserts that it wants the v1 API, but we are
still using the v2 C API, whatever the plugin asks for?

I'm cautious about adding asserts unless they can never happen now or
in the future, because we don't really want an assert() happening on a
customer site running virt-v2v with the rhv-upload Python plugin.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-df lists disk usage of guests without needing to install any
software inside the virtual machine.  Supports Linux and Windows.
http://people.redhat.com/~rjones/virt-df/

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

On Fri, Nov 22, 2019 at 11:14 PM Richard W.M. Jones <rjones@redhat.com>
wrote:
>
> On Fri, Nov 22, 2019 at 02:55:31PM -0600, Eric Blake 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).
>
> I couldn't work out how to do this, plus we have the callback_defined
> function which makes this easy, so yes, I did the easy thing :-)
>
> > >@@ -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?
>
> While I'm no Python programmer, it does look as if existing Python
> core APIs are happy to use bitmasks, eg:
>
>   https://docs.python.org/3/library/os.html#os.open
>
> and this is also easy to implement and keeps it similar to the C API.
Lot of python code is a thin wrapper over C code, and low level
details like flags
leak out to python, so this is a reasonable API.

But having more pythonic API can make python developer little happier when
working with this code. I don't know if it is worth the extra effort.

Nir

On 11/22/19 3:14 PM, Richard W.M. Jones wrote:
>>> @@ -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?
> 
> While I'm no Python programmer, it does look as if existing Python
> core APIs are happy to use bitmasks, eg:
> 
>    https://docs.python.org/3/library/os.html#os.open
> 
> and this is also easy to implement and keeps it similar to the C API.
Fair enough.  I'm just making sure that our Python interface is at least 
idiomatic to a Python programmer, rather than blatantly being a naive 
translation of a C programmer.
> 
>>> - 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?
> 
> I think it would be better if we simply referred back to the C
> documentation to avoid duplication.  Also an advantage of using
> bitmasks.  That does require a rather larger change to the
> documentation though.
Pointing to the C docs, and focusing on just the bindings-induced 
differences, is fine.
> 
>>>   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).
> 
> Is that true?  The plugin asserts that it wants the v1 API, but we are
> still using the v2 C API, whatever the plugin asks for?
Hmm.  If the user implements a 'can_fua' callback that returns 1 even 
though they forgot to declare 'api_version', then the flag can indeed be
set.  Perhaps that means our 'can_fua' C wrapper should have a version 
1/2 difference in behavior (in v1, raise an error reminding the user to 
fix their plugin to declare 'api_version', in v2 pass back the result), 
so that we could indeed then use the assert with a guarantee that it 
will not trigger on an arbitrary plugin.
> 
> I'm cautious about adding asserts unless they can never happen now or
> in the future, because we don't really want an assert() happening on a
> customer site running virt-v2v with the rhv-upload Python plugin.
Understood.

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

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:07 PM Richard W.M. Jones <rjones@redhat.com>
wrote:
>
> On Fri, Nov 22, 2019 at 02:55:31PM -0600, Eric Blake wrote:
> > 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]
>
> I believe it's not possible to do it for sh plugins.
>
> For (eg) python plugins it works like this:
>
>   #!/usr/sbin/nbdkit python
>   -> runs nbdkit python <script name>
>   -> <script name> is interpreted as magic script= parameter
>   -> the python plugin works by loading the script using
>      PyRun_SimpleFileEx which interprets the contents of the
>      script as python code
>
> However for shell it doesn't work:
>
>   #!/usr/sbin/nbdkit sh
>   -> runs nbdkit sh <script name>
>   -> <script name> is interpreted as magic script= parameter
>   -> the sh plugin works by actually executing the script
>   -> executing the script repeats the steps over from the top,
>      causing an infinite loop
>
> If you can think of a way to make this work it would be a useful
> feature IMO.
Even if we can find a way, it sounds too complicated, and I don't see
the use case.

Nir

On 11/22/19 3:07 PM, Richard W.M. Jones wrote:
> On Fri, Nov 22, 2019 at 02:55:31PM -0600, Eric Blake wrote:
>> 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]
> 
> I believe it's not possible to do it for sh plugins.
> 
> For (eg) python plugins it works like this:
> 
>    #!/usr/sbin/nbdkit python
>    -> runs nbdkit python <script name>
>    -> <script name> is interpreted as magic script= parameter
>    -> the python plugin works by loading the script using
>       PyRun_SimpleFileEx which interprets the contents of the
>       script as python code
> 
> However for shell it doesn't work:
> 
>    #!/usr/sbin/nbdkit sh
>    -> runs nbdkit sh <script name>
>    -> <script name> is interpreted as magic script= parameter
>    -> the sh plugin works by actually executing the script
>    -> executing the script repeats the steps over from the top,
>       causing an infinite loop
> 
> If you can think of a way to make this work it would be a useful
> feature IMO.
Hmm.  Right now, we document that:

nbdkit sh file

requires 'file' to be executable, and that we call
exec*("file", {
"file", "op", ... })

and that the interpreter need not be sh (file can start with any #! to 
run a script that will be executed by another interpreter).

In order to get a shebang redirect to work, though, it seems like we 
must NOT directly re-execute the file, but instead force the file to be 
parsed by "/bin/sh" (or have some other means of identifying exactly 
which interpreter to use, outside of the normal #! means), where we 
would invoke exec*("/bin/sh", {"file", "op", ...})
and thereby skip the
kernel's attempt to re-execute under a different interpreter.

Perhaps we could get this by teaching the sh plugin to read() the first 
line of the script; if it starts with #! and contains 'nbdkit sh', then 
we force exec through "sh" (hmm, is there a way to force exec through 
bash instead?).  Otherwise, we exec through the file-name as-is.  Then 
our sh plugin would support two modes: the existing mode (we exec any 
interpreter by letting the kernel interpret the #! line, but you can't 
get shebang forwarding over to nbdkit), and the new mode (we exec a 
shell directly to bypass kernel shebang interpretation, but now you can 
write a shell script that gets shebang forwarding over to nbdkit).

It's also a bummer that different platforms vary on how #! lines are 
parsed - you can't portably pass more than a single argument, so there 
is no portable way to write '#!/path/to/nbdkit sh shell=/bin/bash' as a 
convenient way to choose which shell to use when exec'ing the script 
while bypassing the #!, short of having nbdkit reimplement what GNU 
coreutils recently added as '/bin/env -S' (and that reimplementation 
would have to be in nbdkit proper, not in the sh plugin, because such a 
shebang line would result in 'nbdkit' 'sh shell=/bin/bash'
'file'
'args', but there is no '/path/to/nbdkit-plugin-sh
shell=/bin/bash.so'
plugin to be loaded).

If we choose to make the sh plugin smart enough to parse line 1 to see 
if it is a shebang containing the substring 'nbdkit sh', we could also 
make it parse line 2 to pick up some magic comment line of 
'SHELL=/bin/bash' as the binary to exec.  That's a bit more
complicated
to document, but might serve to let us use the sh plugin for the two 
separate modes documented above (mode 1 to run an arbitrary executable 
file which may use shebang to call out its own interpreter, mode 2 to 
run an arbitrary file with a fixed interpreter allowing that file to 
start with a shebang to call out to nbdkit).  Or maybe we create two 
different plugins (keep the existing 'sh' plugin with existing 
semantics, and add a new 'shell' plugin that shares 99% of the code but 
forces execution with a fixed shell).

The other thing to consider is what brought this topic up - a question 
of whether plugins/sh/eval.sh is worth a conversion into a standalone 
plugin.  With other languages, our choice is between:
nbdkit python script
./script (where #! reinvokes nbdkit python script)

But we don't have a way to write:
nbdkit script

where nbdkit recognizes that 'script' is not a .so file, but IS a file 
that calls out python in its #!, and therefore attempts to load the 
python.so plugin.

The question with the eval.sh plugin is if we can write:

nbdkit eval pread=...

and have nbdkit figure out that 'eval' is not a plugin but IS a file 
with a #! calling out 'nbdkit sh', and therefore load the 'sh'
plugin
with 'script=eval'.  In other words, maybe it's time to teach nbdkit
general magic on how to handle the plugin argument (if an .so is found 
then load it; if not, then inspect the file to see which .so to use 
instead).

Or maybe we just write a C form of a plugin named 'eval' that does the 
same things as what 'sh eval.sh' does, sharing as much code as possible,
but where you have no shebang form because you never pass a script= 
argument to the eval plugin.

All interesting thoughts, but I'm not sure I got us any closer to a nice 
conclusion.

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