[Libguestfs] [nbdkit PATCH] filters: Improve docs on .prepare prerequisites

[Eric Blake]

On 7/9/20 2:20 PM, Eric Blake wrote:

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

Here's what I finally pushed:

 From fcb322c4e87a926607393349d7b9b0e9a1533fda Mon Sep 17 00:00:00 2001
From: Eric Blake <eblake at redhat.com>
Date: Thu, 9 Jul 2020 11:36:03 -0500
Subject: [nbdkit PATCH] filters: Improve docs on .prepare prerequisites

Filters have a requirement to ensure prerequisite functions are called
before using backend I/O functions, in order to avoid triggering
assertions in backend.c that perform sanity checking on each request.
When a filter has no .prepare and no hard-coded overrides of .can_FOO,
this happens automatically, but it is worth documenting cases where
.prepare must call these prerequisites manually (such as when .prepare
does I/O, or .get_size returns a hard-coded size).

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 at redhat.com>
---
  docs/nbdkit-filter.pod | 22 +++++++++++++++++-----
  1 file changed, 17 insertions(+), 5 deletions(-)

diff --git a/docs/nbdkit-filter.pod b/docs/nbdkit-filter.pod
index acac3e50..32208b4c 100644
--- a/docs/nbdkit-filter.pod
+++ b/docs/nbdkit-filter.pod
@@ -383,15 +383,27 @@ 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.

+Note that nbdkit performs sanity checking on requests made to the
+underlying plugin; for example, C<next_ops-E<gt>pread> cannot be
+called on a given connection unless C<next_ops-E<gt>get_size> has
+first been called at least once in the same connection (to ensure the
+read requests are in bounds), and C<next_ops-E<gt>pwrite> further
+requires an earlier successful call to C<next_ops-E<gt>can_write>.  In
+many filters, these prerequisites will be automatically called during
+client negotiation phase, but there are cases where a filter overrides
+query functions or makes I/O calls into the plugin before handshaking
+is complete, where the filter needs to make those prerequisite calls
+manually during C<.prepare>.
+
  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
  through the C<next_ops> structure.  Instead the core nbdkit server
-- 
2.27.0




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