Re: [Libguestfs] [PATCH nbdkit 1/2] server: Add .after_fork callback, mainly for plugins to create threads.

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

>+=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)

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

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]

>+=head2 C<.after_fork> >+ >+ int after_ready (void); Typo in the function name.

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