2:20 p.m.
On 7/9/20 12:08 PM, Richard W.M. Jones wrote:
On Thu, Jul 09, 2020 at 11:37:53AM -0500, Eric Blake wrote:
> Since .prepare is called before client negotiation has completed,
> filters have an additional burden to ensure prerequisite functions are
> called in order to avoid triggering assertions in backend.c.
>
> See also: https://bugzilla.redhat.com/show_bug.cgi?id=1855330,
> https://www.redhat.com/archives/libguestfs/2020-July/msg00041.html
>
> Signed-off-by: Eric Blake <eblake(a)redhat.com>
> ---
> docs/nbdkit-filter.pod | 18 ++++++++++++------
> 1 file changed, 12 insertions(+), 6 deletions(-)
>
> diff --git a/docs/nbdkit-filter.pod b/docs/nbdkit-filter.pod
> index acac3e50..3d201309 100644
> --- a/docs/nbdkit-filter.pod
> +++ b/docs/nbdkit-filter.pod
> @@ -383,14 +383,20 @@ connection (C<.finalize>).
>
> For example if you need to scan the underlying disk to check for a
> partition table, you could do it in your C<.prepare> method (calling
> -the plugin's C<.pread> method via C<next_ops>). Or if you need to
> -cleanly update superblock data in the image on close you can do it in
> -your C<.finalize> method (calling the plugin's C<.pwrite> method).
> -Doing these things in the filter's C<.open> or C<.close> method is
not
> -possible.
> +the plugin's C<.get_size> and C<.pread> methods via
C<next_ops>). Or
> +if you need to cleanly update superblock data in the image on close
> +you can do it in your C<.finalize> method (calling the plugin's
> +C<.pwrite> method). Doing these things in the filter's C<.open> or
> +C<.close> method is not possible.
>
> For C<.prepare>, the value of C<readonly> is the same as was passed to
> -C<.open>, declaring how this filter will be used.
> +C<.open>, declaring how this filter will be used. When calling plugin
> +functions during C<.prepare>, the filter must ensure that prerequisite
> +functions have succeeded (for example, calling C<.pwrite>) requires
> +checking both C<.get_size> and C<.can_write>); while these
> +prerequisites are automatically handled in most other cases thanks to
> +client negotiation, the timing of C<.prepare> comes before client
> +negotiation has completed.
I think this isn't sufficient. I think a filter which does:
int64_t my_filter_get_size () { return size; }
int my_filter_prepare (int readonly) { return 0; }
will fail as h->exportsize is only updated by a call to
next_ops->get_size. This is basically what the tar filter was doing
on the second connection (before I fixed it).
True. But I'm not sure how best to word it. Ultimately, a filter may
choose to bypass any part of the negotiation (such as overriding
.can_write or .get_size because it plans on giving a different answer),
and where it does not later ask for the same underlying functionality
from the plugin, that's not a problem. So the real rule is more like:
for any action that you plan to use the underlying plugin for, you must
ultimately go through the same negotiation prerequisites, even if by the
time you are in .get_ready you have enough information to short circuit
some of that information. If nothing else, since the tar filter plans
on using .pread for each handle, but only reads sizes during the first
handle, subsequent handles should do a cursory check of .get_size to
ensure the underlying plugin hasn't resized behind tar's back.
I'll give this a couple more days of thought to see if I can derive
better wording to go in the docs.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org