8:09 a.m.
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