On Mon, Jun 22, 2020 at 02:00:50PM -0500, Eric Blake wrote:
> >+++ b/docs/nbdkit-filter.pod
>
> >+=head2 C<.after_fork>
> >+
> >+ int (*after_fork) (nbdkit_next_after_fork *next, void *nxdata);
> >+
> >+This intercepts the plugin C<.after_fork> method and can be used by
> >+the filter to start background threads (although these have limited
> >+usefulness since they will not be able to access the plugin).
> >+
> >+If there is an error, C<.after_fork> should call C<nbdkit_error> with
> >+an error message and return C<-1>.
>
> Should the backend code guarantee that .after_fork is called for all
> layers in the backend chain? The only flexibility I see us gaining
> by letting the filter choose when .after_fork is called is
> fine-grained control over what threads the filter can start before
> or after the plugin's .after_fork, but is that flexibility really
> needed, or are we equally safe if the backend just always calls
> .after_fork and the filter doesn't have to worry about calling
> next(nxdata)?
>
> Of course, given that filters are in-tree, we can change this
> signature as needed later on, even if we make it void for now we can
> hook it into passing .after_fork later if we find a use for that
> finer-grained call semantics.
In theory a filter that wanted background threads would have exactly
the same problem as a plugin and would need to add an .after_fork
callback to create them. I can't think of a situation where a filter
would be correct to not call the plugin's .after_fork, but I suppose
it's plausible it might want to do something before or after the
plugin.
The biggest practical problem at the moment however is ...
At this point I would link to the TODO file on github, but it seems
like github is down. If you look at the TODO file you'll see I
mentioned a general problem with background threads in filters.
Anyway as you say they're all in tree so we can worry about this
later.
> >+=item C<.after_fork>
> >+
> >+In normal operation, C<.after_fork> is called after the server has
> >+forked into the background and changed UID and directory. If a plugin
> >+needs to create background threads (or uses an external library that
> >+creates threads) it should do so here, because background threads are
> >+killed by fork.
>
> s/killed/invalidated/ (they are still alive in the parent process,
> but after the fork we are no longer in the parent process to
> interact with them)
OK, I'll change this. It's also of course a point that if a plugin
creates background threads before the fork and they grab locks then
that could lead to problems. So I need to add a note to .get_ready
saying that you shouldn't create background threads there.
> >+
> >+Because the server may have forked into the background, error messages
> >+and failures from C<.after_fork> cannot be seen by the user unless
> >+they look through syslog. An error in C<.after_fork> can appear to
> >+the user as if nbdkit “just died”. So in almost all cases it is
> >+better to use C<.get_ready> instead of this callback, or to do as much
> >+preparation work as possible in C<.get_ready> and only start
> >+background threads here.
> >+
> >+The server doesn't always fork (eg. if the I<-f> flag is used), but
> >+even so this callback will be called. If you want to find out if the
> >+server forked between C<.get_ready> and C<.after_fork> use
> >+L<getpid(2)>.
>
> Thinking about back-compat:
> We promise that plugins compiled against older nbdkit will continue
> to run against newer nbdkit, and that is preserved here.
> What we do not promise is that plugins compiled against newer nbdkit
> will work wehn loaded by older nbdkit. This is one of those cases:
> anything you stick in .after_fork will not be called by older nbdkit
> which didn't know about the callback.
>
> At present, we silently ignore the tail of 'struct plugin' from any
> plugin compiled against newer nbdkit when loaded by older nbdkit.
> Should we tweak that to instead output a warning that the plugin
> might rely on features present only in newer nbdkit and therefore
> might not work in the current nbdkit version? Of course, that
> doesn't help the fact that existing older nbdkit is lacking this
> safety check.
It should probably only do that if the tail of the struct is non-zero,
because who cares if it contains NULL pointers. But that's another
patch.
> Do we need to instead add some sort of API for plugins to be able to
> learn which features are provided by the nbdkit loading the plugin,
> so that a plugin that requires the use of 1.22 nbdkit features (such
> as .after_fork) can gracefully fail during .config_complete (or
> similar) when loaded by a 1.20 nbdkit, rather than mysteriously
> failing to work when .after_fork is not called? [In a distro
> setting, the solution is version dependencies: you could make it so
> that installing the nbdkit-vddk-plugin package forces an upgrade of
> the nbdkit package to 1.22 - but this is more a question for
> situations not covered by distro packaging setups]
TBH this is a non-supported case so if it fails you get to keep all
the pieces.
> >+=head2 C<.after_fork>
> >+
> >+ int after_ready (void);
>
> Typo in the function name.
Oops.
> >+++ b/server/filters.c
> >@@ -193,6 +193,28 @@ filter_get_ready (struct backend *b)
> > b->next->get_ready (b->next);
> > }
> >+static int
> >+next_after_fork (struct backend *b)
> >+{
> >+ b->after_fork (b);
> >+ return 0;
> >+}
> >+
> >+static void
> >+filter_after_fork (struct backend *b)
> >+{
> >+ struct backend_filter *f = container_of (b, struct backend_filter,
> >backend);
> >+
> >+ debug ("%s: after_fork", b->name);
> >+
> >+ if (f->filter.after_fork) {
> >+ if (f->filter.after_fork (next_after_fork, b->next) == -1)
> >+ exit (EXIT_FAILURE);
> >+ }
> >+ else
> >+ b->next->after_fork (b->next);
> >+}
>
> This code changes if we decide that filters could live with 'void'
> instead of next_after_fork, because we mandate the chain traversal
> through the backend rather than leaving it up to the filters.
>
> Still a few tweaks needed, but the idea looks sane.
Rich.
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-df lists disk usage of guests without needing to install any
software inside the virtual machine. Supports Linux and Windows.
http://people.redhat.com/~rjones/virt-df/
_______________________________________________
Libguestfs mailing list
Libguestfs@redhat.com
https://www.redhat.com/mailman/listinfo/libguestfs
