11:37 a.m.
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.
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
12:08 p.m.
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).
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(a)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
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
8:43 p.m.
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(a)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(a)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
1659
days inactive
1672
days old
3 comments
2 participants
participants (2)
-
Eric Blake -
Richard W.M. Jones