[Libguestfs] [PATCH NOT WORKING nbdkit v2 1/2] server: Add .ready_to_serve plugin method.

[Eric Blake]

On 10/11/19 4:42 AM, Richard W.M. Jones wrote:
> This method can be used for plugins to get control after the server
> has forked and changed user but before it accepts a connection.  This
> is very late and the only real use for this is for a plugin to create
> background threads for its own use.
> ---

> +++ b/docs/nbdkit-filter.pod

>   
> +=head2 C<.ready_to_serve>
> +
> + int (*ready_to_serve) (nbdkit_next_ready_to_serve *next, void *nxdata);
> +
> +This intercepts the plugin C<.ready_to_serve> method.
> +
> +If there is an error, C<.ready_to_serve> should call C<nbdkit_error>
> +with an error message and return C<-1>.

Do we need to require the filter to be involved in the recursion?

With .open and .config, we do, because there are parameters to the 
function, where the filter may change the parameter handed to the plugin 
(for example, changing read-write to read-only or altering a config 
key).  But for other functions where there is no parameter, we let 
nbdkit control the recursion (.prepare, .finalize, .close).  There is no 
other parameter here to be changed, so having nbdkit run the recursion 
may make more sense.

Should there be a counterpart function for cleanup?  We have:

.load/.unload - called once per nbdkit process, too early for forks
[here's where .ready_to_serve fits]
.open/.close - called once per connection, too early for next_ops
.close/.finalize - called once per connection, full access

The new .ready_to_serve is called once per nbdkit process, but at a 
point where it knows all .loads are complete and it is safe to fork.  I 
guess .unload can also do a pthread_join() on any thread created during 
.ready_to_serve.  We needed .finalize separate from .close because the 
filter may still need to manipulate the plugin, but there is no 
manipulation of the plugin needed during .load or .ready_to_serve, so 
having a trio of related functions rather than two nested pairs of 
functions still works.

> +++ b/docs/nbdkit-plugin.pod
> @@ -142,6 +142,13 @@ before any connections are accepted.  However, during C<nbdkit
>   C<.config_complete> (so a plugin which determines the results from a
>   script must be prepared for a missing script).
>   
> +=item C<.ready_to_serve>
> +
> +This callback is called after nbdkit has forked into the background
> +and changed user, and before it accepts any client connection.  If the
> +plugin needs to create its own background threads then this is a good
> +place to do that.
> +

Should there be any caveat about the threads needing to be joined no 
later than .unload, so that there is no risk of the helper threads 
segfaulting after the .so is unloaded?

>   =item C<.open>
>   
>   A new client has connected.
> @@ -408,7 +415,8 @@ Any other bare parameters give errors.
>   
>   This optional callback is called after all the configuration has been
>   passed to the plugin.  It is a good place to do checks, for example
> -that the user has passed the required parameters to the plugin.
> +that the user has passed the required parameters to the plugin.  It is
> +also a good place to do general start-up work.

Should this emphasize that .load must not leave other threads running on 
its completion, because it is invoked pre-fork()?

>   
>   If there is an error, C<.config_complete> should call C<nbdkit_error>
>   with an error message and return C<-1>.
> @@ -437,6 +445,23 @@ are silently ignored.
>   If there is an error, C<.thread_model> should call C<nbdkit_error>
>   with an error message and return C<-1>.
>   
> +=head2 C<.ready_to_serve>
> +
> + int ready_to_serve (void);
> +
> +This optional callback is called after nbdkit has forked into the
> +background and changed user, and before it accepts any client
> +connection.
> +
> +If the plugin needs to create its own background threads then this is
> +a good place to do that.  However because nbdkit may have already
> +forked into the background and so it is difficult to communicate
> +errors back to the user, this is B<not> a good place to do general
> +start-up work (use C<.config_complete> instead).
> +
> +If there is an error, C<.ready_to_serve> should call C<nbdkit_error>
> +with an error message and return C<-1>.
> +
>   =head2 C<.open>
>   
>    void *open (int readonly);
> diff --git a/include/nbdkit-filter.h b/include/nbdkit-filter.h
> index c1930c1..7b3a64f 100644

> +++ b/server/filters.c
> @@ -190,6 +190,29 @@ plugin_magic_config_key (struct backend *b)
>     return b->next->magic_config_key (b->next);
>   }
>   
> +static int
> +next_ready_to_serve (void *nxdata)
> +{
> +  struct backend *b = nxdata;
> +  b->ready_to_serve (b);
> +  return 0;
> +}
> +
> +static void
> +filter_ready_to_serve (struct backend *b)
> +{
> +  struct backend_filter *f = container_of (b, struct backend_filter, backend);
> +
> +  debug ("%s: ready_to_serve", b->name);
> +
> +  if (f->filter.ready_to_serve) {
> +    if (f->filter.ready_to_serve (next_ready_to_serve, b->next) == -1)
> +      exit (EXIT_FAILURE);
> +  }
> +  else
> +    b->next->ready_to_serve (b->next);

Should we add a backend_ready_to_serve() helper function?  Do we want 
nbdkit to run the recursion itself, instead of requiring the filters to 
call next_ready_to_serve?


> +++ b/tests/test-layers-filter.c
> @@ -65,7 +65,7 @@ test_layers_filter_config (nbdkit_next_config *next, void *nxdata,
>   
>   static int
>   test_layers_filter_config_complete (nbdkit_next_config_complete *next,
> -                                     void *nxdata)
> +                                    void *nxdata)
>   {
>     DEBUG_FUNCTION;
>     return next (nxdata);
> @@ -74,6 +74,14 @@ test_layers_filter_config_complete (nbdkit_next_config_complete *next,
>   #define test_layers_filter_config_help          \
>     "test_layers_" layer "_config_help"
>   
> +static int
> +test_layers_filter_ready_to_serve (nbdkit_next_ready_to_serve *next,
> +                                     void *nxdata)

Indentation looks off.

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