8:40 a.m.
Here's my second attempt at a filter API.
As before, .config and .config_complete can only call the
next->.config or next->.config_complete methods in the chain
respectively.
The change is with the connected functions (get_size, pread, pwrite,
etc.). Here we pass a struct of next functions allowing the filter to
call any functions on the plugin, so for example a write can turn into
read + write (and vice versa although that might not be a good idea).
Also .open and .close cannot be intercepted by the filter. They're
just used to allow the filter to allocate a handle.
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
10:52 a.m.
On 01/16/2018 08:40 AM, Richard W.M. Jones wrote:
Here's my second attempt at a filter API.
As before, .config and .config_complete can only call the
next->.config or next->.config_complete methods in the chain
respectively.
The change is with the connected functions (get_size, pread, pwrite,
etc.). Here we pass a struct of next functions allowing the filter to
call any functions on the plugin, so for example a write can turn into
read + write (and vice versa although that might not be a good idea).
A read turning into a read+write makes total sense when doing
copy-on-read thin-provisioning (the read from a remote site moves it by
writing a copy into a local site, so that future reads at that same
address are faster). But yeah, we're trusting that filters will not
abuse expected semantics.
#ifndef NBDKIT_FILTER_H
#define NBDKIT_FILTER_H
/* 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.
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.
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? 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)?
nbdkit-filter.pod
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?
=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?
=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.
In general, this looks like a bit better approach than your first
attempt, in that it lets filters do a lot more during an open connection.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
11:05 a.m.
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/
2536
days inactive
2536
days old
2 comments
2 participants
participants (2)
-
Eric Blake -
Richard W.M. Jones