Libguestfs - Jan 2018 - Re: [PATCH 7/9] Implement filters.

On 01/17/2018 02:53 PM, Richard W.M. Jones wrote:
> Also implements the --filters parameter.
> ---
>  docs/nbdkit.pod |  21 +-
>  nbdkit.in       |  17 +-
>  src/Makefile.am |   1 +
>  src/filters.c   | 606
++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  src/internal.h  |  23 ++-
>  src/main.c      | 114 +++++++++--
>  src/plugins.c   |  11 +-
>  7 files changed, 772 insertions(+), 21 deletions(-)
> 
> +/* The next_functions structure contains pointers to backend
> + * functions.  However because these functions are all expecting a
> + * backend and a connection, we cannot call them directly, but must
> + * write some next_* functions that unpack the two parameters from a
> + * single ‘void *nxdata’ struct pointer (‘b_conn’).
> + */
> +
> +/* Literally a backend + a connection pointer.  This is the
> + * implementation if ‘void *nxdata’ in the filter API.
s/if/of/
> +++ b/src/main.c
> @@ -120,6 +121,7 @@ static const struct option long_options[] = {
>    { "export",     1, NULL, 'e' },
>    { "export-name",1, NULL, 'e' },
>    { "exportname", 1, NULL, 'e' },
> +  { "filter",     1, NULL, 0 },
If you assign a value > 256 to options that only have a long
spelling...
> @@ -245,6 +252,18 @@ main (int argc, char *argv[])
>          exit (EXIT_FAILURE);
>  #endif
>        }
> +      else if (strcmp (long_options[option_index].name,
"filter") == 0) {
...then you can simply add additional case labels in your switch
statement that handle each option, rather than having to make an
ever-increasing if chain.  That's a separate cleanup, so I'll go ahead
and propose it on top of your series (I'm also planning on rebasing my
FUA flags on top of your series, since so far it is looking pretty good).

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

On Wed, Jan 17, 2018 at 04:31:31PM -0600, Eric Blake
wrote:
> (I'm also planning on rebasing my
> FUA flags on top of your series, since so far it is looking pretty good).
I think after some thought I'm going to add a filter.open method which
will include a next parameter (actually 2x next parameters, for
next_open and nbdkit_next).

It's necessary to implement a copy-on-write layer that we can have a
read-write filter which enables readonly on the layer below.  Thus it
needs to call the layer below with open (readonly=1).

For the partition filter it makes sense that during the filter open
call we can read the partition table from the lower layer, ie.
issuing pread call(s) to the plugin.  Any other way to do this is very
clunky.

However there are risks here.  In particular when we currently are in
the .open call we haven't fully brought up the connection.  We haven't
even done the option negotiation, but the plugin will be expected to
handle pread() and other calls.  I need to examine the code properly
to see if this matters.

Also filter.close could be extended in the same way since closing a
filter might involve finalizing data in some way.

Also this means we do need to define the order in which open and close
calls are done across filters & plugins.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-p2v converts physical machines to virtual machines.  Boot with a
live CD or over the network (PXE) and turn machines into KVM guests.
http://libguestfs.org/virt-v2v

On 01/18/2018 03:15 AM, Richard W.M. Jones wrote:
> On Wed, Jan 17, 2018 at 04:31:31PM -0600, Eric Blake wrote:
>> (I'm also planning on rebasing my
>> FUA flags on top of your series, since so far it is looking pretty
good).
> 
> I think after some thought I'm going to add a filter.open method which
> will include a next parameter (actually 2x next parameters, for
> next_open and nbdkit_next).
> 
> It's necessary to implement a copy-on-write layer that we can have a
> read-write filter which enables readonly on the layer below.  Thus it
> needs to call the layer below with open (readonly=1).
> 
> For the partition filter it makes sense that during the filter open
> call we can read the partition table from the lower layer, ie.
> issuing pread call(s) to the plugin.  Any other way to do this is very
> clunky.
> 
> However there are risks here.  In particular when we currently are in
> the .open call we haven't fully brought up the connection.  We
haven't
> even done the option negotiation, but the plugin will be expected to
> handle pread() and other calls.  I need to examine the code properly
> to see if this matters.
I think we're okay there, as long as we follow my earlier life-cycle
comment: we must open things bottom-up (complete the plugin's open()
before calling the filter's open()), then process all other messages
top-down (filter gets first shot; for things like config and pread, the
filter chooses whether to call the plugin; for others like close, we
guarantee that the filter is closed first, then the plugin is closed,
without relying on the filter to forward things).
> 
> Also filter.close could be extended in the same way since closing a
> filter might involve finalizing data in some way.
> 
> Also this means we do need to define the order in which open and close
> calls are done across filters & plugins.
Yep, that means we're in agreement on layering order guarantees.

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