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

[Richard W.M. Jones]

On Fri, Oct 11, 2019 at 07:59:30AM -0500, Eric Blake wrote:
> 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.

Two issues I think: Do we let filters see it at all?  (I did think
about leaving it out, but I included it because we cover every API.)
Do we let filters control the recursion.  I think you are probably
right that it should be nbdkit doing that.

Also another hypothetical issue: filters might want to start
background threads.  For filters that is complicated for two reasons:
(1) It might not be a good idea for the same reasons as plugins, and
(2) A new thread doesn't have access to the next_ops since it runs
outside a connection context.

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

As you say, .unload is the counterpart.  (There are other problems
with .unload running too late, but they are independent of this patch.)

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

Even with the join, nbdkit-vddk-plugin still segfaulted :-( I couldn't
work out if it was a problem with the design or the general existing
problem that nbdkit doesn't have a well-controlled shutdown path.

Thanks for the other comments.  I don't think we need this just yet so
let's leave it for another time.

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/