8:48 a.m.
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/