[Libguestfs] [nbdkit PATCH 0/7] Initial implementation of FUA flag passthrough

[Richard W.M. Jones]

On Tue, Jan 16, 2018 at 07:59:01AM -0600, Eric Blake wrote:
> On 01/16/2018 02:25 AM, Richard W.M. Jones wrote:
> > I had an idea about how we might do this and still keep a single
> > .pwrite method for plugins.  Maybe it's too complicated but here goes:
> > 
> 
> > 
> > This is binary-compatible with old plugins, but not source compatible, so:
> > 
> > 
> > (2) Plugsin may optionally define NBDKIT_PLUGIN_LEVEL before including
> > <nbdkit-plugin.h>.  Code wishing to use the flags variant of pwrite
> > can do:
> > 
> > #define NBDKIT_PLUGIN_LEVEL 2
> > #include <nbdkit-plugin.h>
> > 
> > which would modify the definition of the struct (again), something
> > like:
> > 
> > #ifndef NBDKIT_PLUGIN_LEVEL
> > #define NBDKIT_PLUGIN_LEVEL 1
> > #endif
> 
> So the difference between your proposal and my patch 4 is that in your
> proposal, a new plugin has to explicitly opt-in to the new ABI by
> defining NBDKIT_PLUGIN_LEVEL 2, while in mine, a new plugin opts-in by
> assigning to .pwrite_fua instead of .pwrite.  Yours is slightly cleaner
> in making it obvious that there is only one (and not two) pwrite
> callback per plugin, and either way, the plugin chooses which variant it
> wants.

Yes that's a fair summary.

I think they have to "opt in" in some way because current plugins as
written shouldn't be broken, so they should always see the current
pwrite function.

(Note that the API/ABI guarantees only apply to C code, not to the
other languages)

> > (3) Core nbdkit code always defines NBDKIT_PLUGIN_LEVEL == 2 so that
> > it always sees the new function, but it may need to call the old
> > function:
> > 
> > int
> > plugin_pwrite (...)
> > {
> >   if (plugin.pwrite != NULL)
> >     plugin.pwrite (handle, buf, count, offset, flags);
> >   else if (plugin.pwrite_old1 != NULL)
> >     plugin.pwrite_old1 (handle, buf, count, offset);
> > }
> 
> This part happens whether by your proposal or by mine (see patch 5) - a
> new nbdkit is always able to call the correct one out of two callbacks
> for both old and new plugins.

Right.

> Both proposals are still able to install a shim for the old name when
> the plugin uses only the new semantics; if I use your proposal, my patch
> 7 would be rewritten to only create the shims when a plugin requests
> NBDKIT_PLUGIN_LEVEL 2 (whereas by my proposal, I had to define the shims
> always).

Right.

> Documentation may be a bit tricky under your proposal, but I'm not
> seeing it as much different than mine.  Then there's the decision of
> whether to go with a single flags parameter (closer to the wire
> protocol, but the plugin has to decode bits; and hopefully we do proper
> feature advertisement so that a client never passes a flag that the
> plugin won't understand) or with multiple bool parameters (what my
> proposal did for .pwrite_fua).

TBH I would only bother to document NBDKIT_PLUGIN_LEVEL 2, ie.
the documentation would be changed to:

+  #define NBDKIT_PLUGIN_LEVEL 2
   #include <nbdkit-plugin.h>

and would only document the new pwrite.

> > (4) Internal plugins which don't need the FUA flag can continue to use
> > the level 1 API, which means that we keep checking that the old API
> > still works.
> 
> My proposal did that as well - most plugins still used the older
> spelling without the fua flag.

Yup.

> > 
> > 
> > What do you think?
> 
> I don't see any fundamental differences in end behavior, so much as
> which style we find nicer to document and expose to plugin writers.  I'm
> happy to go with whichever of the two styles you prefer.

I kind of like only have one pwrite method (my version IOW), as the
result is much cleaner, but I could be biased :-)

> The other thing to consider is how we translate all of this to other
> language bindings.  For languages that allow function overloading, I
> suppose we want the declaration of 'pwrite' to take either function
> signature, then the glue code that converts back to C knows based on the
> function signature whether the plugin is written for old- or new-style
> API; for languages without function overloading, they will have to
> mirror some way of declaring which of the two APIs they are interested in.

We can break the language bindings, IOW making them just use the newer
form.  However as you say for languages like Perl it's easy enough to
add an optional FUA parameter so it wouldn't even be a break.

I have a large (almost pure) refactoring change which I'll send to
you in a moment as it would affect anything you write, although it
should only require mechanical changes.

Rich.

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