Libguestfs - Jul 2020 - Re: [nbdkit PATCH] filters: Improve docs on .prepare prerequisites

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@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.

 There is no C<next_ops-E<gt>prepare> or
C<next_ops-E<gt>finalize>.
 Unlike other filter methods, prepare and finalize are not chained
-- 
2.27.0

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@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).

Rich.
>  There is no C<next_ops-E<gt>prepare> or
C<next_ops-E<gt>finalize>.
>  Unlike other filter methods, prepare and finalize are not chained
> -- 
> 2.27.0
> 
> _______________________________________________
> Libguestfs mailing list
> Libguestfs@redhat.com
> https://www.redhat.com/mailman/listinfo/libguestfs
-- 
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

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@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