[Libguestfs] [nbdkit] Proposed (new) filter API

[Richard W.M. Jones]

On Tue, Jan 16, 2018 at 10:52:12AM -0600, Eric Blake wrote:
> > /* This header also defines some useful functions like nbdkit_debug
> >  * and nbdkit_parse_size which are appropriate for filters to use.
> 
> Too much copy-and-paste?
> 
> >  */
> > #include <nbdkit-plugin.h>
> 
> I guess you get those useful functions by inclusion, rather than by
> direct definition.

Yes, that comment describes why we include the header.  I guess I
could delete the comment if it's confusing.

> > 
> > struct nbdkit_next {
> >   int64_t (*get_size) (void *nxdata);
> > 
> >   int (*can_write) (void *nxdata);
> >   int (*can_flush) (void *nxdata);
> >   int (*is_rotational) (void *nxdata);
> >   int (*can_trim) (void *nxdata);
> > 
> >   int (*pread) (void *nxdata, void *buf, uint32_t count, uint64_t offset);
> >   int (*pwrite) (void *nxdata,
> >                  const void *buf, uint32_t count, uint64_t offset);
> >   int (*flush) (void *nxdata);
> >   int (*trim) (void *nxdata, uint32_t count, uint64_t offset);
> >   int (*zero) (void *nxdata, uint32_t count, uint64_t offset, int may_trim);
> > };
> 
> While we want to support plugins that use the old API, should we declare
> that filters only use the new API?  Or does my work to add FUA
> passthrough need to be careful on how a filter does the work;
> particularly if mixing a filter without FUA knowledge with a backend
> that can do it?  I guess as long as we stabilize all of the filter and
> FUA code into the same nbdkit release, it's okay if we tweak the filter
> definition slightly in the next few patches.

I was hoping we could combine the FUA patches (or rather, push them
all together) so that in the final version the filter .pwrite
definition above would contain a FUA/flags parameter.

> > struct nbdkit_filter {
> >   /* Do not set these fields directly; use NBDKIT_REGISTER_FILTER.
> >    * They exist so that we can support filters compiled against
> >    * one version of the header with a runtime compiled against a
> >    * different version with more (or fewer) fields.
> >    */
> >   uint64_t _struct_size;
> >   int _api_version;
> > 
> >   /* New fields will only be added at the end of the struct. */
> >   const char *name;
> >   const char *longname;
> >   const char *version;
> >   const char *description;
> > 
> >   void (*load) (void);
> >   void (*unload) (void);
> > 
> >   int (*config) (nbdkit_next_config *next, void *nxdata,
> >                  const char *key, const char *value);
> >   int (*config_complete) (nbdkit_next_config_complete *next, void *nxdata);
> >   const char *config_help;
> > 
> >   void * (*open) (int readonly);
> 
> Is it conceivable that opening a filter may want to read data from the
> underlying backend?

One case where this could make sense would be with a "partition"
filter (which needs to read the partition table early), and I did
consider that.  However I believe -- although haven't proven by
implementation -- that a partition filter can still be written, it
just needs to read the partition table once in one of the other
functions, whichever is called first, with a bit of mutex-ing.

The advantage to make open not be a passthrough is it somewhat
simplifies the implementation, otherwise it has to deal with the new
handle returned from the next element in the chain.

> Do we have guarantees on lifecycle ordering across
> the chain (if the command line uses filter1 filter2 plugin, we open
> plugin first, filter2 second, filter1 last; then close in reverse order)?

At the moment we open plugin, then filter2, then filter1, and close
them in the same order.  I was hoping not to define the order, since I
don't think it matters(?)

It could also be different for load/unload vs open/close.

> > Different filters can be stacked:
> > 
> >      NBD     ┌─────────┐    ┌─────────┐          ┌────────┐
> >   client ───▶│ filter1 │───▶│ filter2 │── ─ ─ ──▶│ plugin │
> >  request     └─────────┘    └─────────┘          └────────┘
> 
> Does conversion from POD to other formats preserve proper formatting of
> this ASCII-art?

We've used utf-8 in POD in libguestfs since forever.

> > =head2 C<.name>
> > 
> >  const char *name;
> > 
> > This field (a string) is required, and B<must> contain only ASCII
> > alphanumeric characters and be unique amongst all filters.
> 
> Do filters and plugins share the same namespace, or can you have a
> filter whose name matches a plugin?

Different namespaces because they go into different directories
(plugindir vs filterdir).

> > =head2 C<.open>
> > 
> >  void * (*open) (int readonly);
> > 
> > This is called when a new client connection is opened and can be used
> > to allocate any per-connection data structures needed by the filter.
> > The handle (which is not the same as the plugin handle) is passed back
> > to other filter callbacks and could be freed in the C<.close>
> > callback.
> > 
> > Note that the handle is completely opaque to nbdkit, but it must not
> > be NULL.
> > 
> > If there is an error, C<.open> should call C<nbdkit_error> with an
> > error message and return C<NULL>.
> 
> I guess if you wanted to be truly opaque and support NULL, you'd have to
> have something like:
> 
> int (*open) (int readonly, void **result)
> 
> which populates *result when returning 0, and returns -1 on failure.
> But I'm also fine with your choice of stating that the opaque data has
> to be anything other than NULL.

It's consistent with how plugins work now.

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/