[Libguestfs] [nbdkit PATCH 1/5] filters: Tighter rules on .get_ready/.after_fork
Richard W.M. Jones
rjones at redhat.com
Tue May 11 11:57:02 UTC 2021
On Thu, May 06, 2021 at 08:59:36PM -0500, Eric Blake wrote:
> Allowing a filter to bypass the plugin's .get_ready or .after_fork
> makes no sense, and there are no other arguments to modify.
> Furthermore, now that filters have access to open a plugin context
> independently of a client connection, .after_fork would be an ideal
> place to do so with its existing nbdkit_backend argument (which we
> already documented was stable, although this test actually adds tests
> for that). Still, we will want to add a counterpart API (in a later
> patch) to tear that context down cleanly, where .unload is too late.
> But for that to work, we need to ensure that the plugin completes
> initialization before the filter is allowed to try to open a context
> into the plugin, which means swapping the visitation order to be
> inner-to-outer (similar to .prepare).
> ---
> docs/nbdkit-filter.pod | 24 +++++++++++------------
> include/nbdkit-filter.h | 7 ++-----
> server/filters.c | 26 ++++++-------------------
> filters/exitwhen/exitwhen.c | 9 ++++-----
> filters/extentlist/extentlist.c | 5 ++---
> filters/log/log.c | 9 ++++-----
> filters/multi-conn/multi-conn.c | 5 ++---
> filters/pause/pause.c | 4 ++--
> filters/rate/rate.c | 5 ++---
> filters/stats/stats.c | 5 ++---
> filters/tls-fallback/tls-fallback.c | 5 ++---
> tests/test-layers-filter.c | 12 +++++-------
> tests/test-layers.c | 30 ++++++++++++++---------------
> 13 files changed, 60 insertions(+), 86 deletions(-)
>
> diff --git a/docs/nbdkit-filter.pod b/docs/nbdkit-filter.pod
> index 97f32e10..9816f4e8 100644
> --- a/docs/nbdkit-filter.pod
> +++ b/docs/nbdkit-filter.pod
> @@ -127,8 +127,7 @@ which is required.
> =head1 NEXT PLUGIN
>
> F<nbdkit-filter.h> defines some function types (C<nbdkit_next_config>,
> -C<nbdkit_next_config_complete>, C<nbdkit_next_get_ready>,
> -C<nbdkit_next_after_fork>, C<nbdkit_next_preconnect>,
> +C<nbdkit_next_config_complete>, C<nbdkit_next_preconnect>,
> C<nbdkit_next_list_exports>, C<nbdkit_next_default_export>,
> C<nbdkit_next_open>) and a structure called C<struct nbdkit_next_ops>.
> These abstract the next plugin or filter in the chain. There is also
> @@ -406,28 +405,29 @@ with an error message and return C<-1>.
>
> =head2 C<.get_ready>
>
> - int (*get_ready) (nbdkit_next_get_ready *next, void *nxdata,
> - int thread_model);
> + int (*get_ready) (int thread_model);
>
> -This intercepts the plugin C<.get_ready> method and can be used by the
> -filter to get ready to serve requests.
> +This optional callback is reached if the plugin C<.get_ready> method
> +succeeded (if the plugin failed, nbdkit has already exited), and can
> +be used by the filter to get ready to serve requests.
>
> The C<thread_model> parameter informs the filter about the final
> thread model chosen by nbdkit after considering the results of
> C<.thread_model> of all filters in the chain after
> -C<.config_complete>. This does not need to be passed on to C<next>,
> -as the model can no longer be altered at this point.
> +C<.config_complete>.
>
> If there is an error, C<.get_ready> should call C<nbdkit_error> with
> an error message and return C<-1>.
>
> =head2 C<.after_fork>
>
> - int (*after_fork) (nbdkit_next_after_fork *next, nbdkit_backend *nxdata);
> + int (*after_fork) (nbdkit_backend *backend);
>
> -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).
> +This optional callback is reached after the plugin C<.after_fork>
> +method has succeeded (if the plugin failed, nbdkit has already
> +exited), and can be used by the filter to start background threads.
> +The C<backend> parameter is valid until C<.unload>, for creating manual
> +contexts into the backend with C<nbdkit_next_context_open>.
>
> If there is an error, C<.after_fork> should call C<nbdkit_error> with
> an error message and return C<-1>.
> diff --git a/include/nbdkit-filter.h b/include/nbdkit-filter.h
> index 86c41dd7..662f52cb 100644
> --- a/include/nbdkit-filter.h
> +++ b/include/nbdkit-filter.h
> @@ -66,8 +66,6 @@ typedef struct nbdkit_next_ops nbdkit_next;
> typedef int nbdkit_next_config (nbdkit_backend *nxdata,
> const char *key, const char *value);
> typedef int nbdkit_next_config_complete (nbdkit_backend *nxdata);
> -typedef int nbdkit_next_get_ready (nbdkit_backend *nxdata);
> -typedef int nbdkit_next_after_fork (nbdkit_backend *nxdata);
> typedef int nbdkit_next_preconnect (nbdkit_backend *nxdata, int readonly);
> typedef int nbdkit_next_list_exports (nbdkit_backend *nxdata, int readonly,
> struct nbdkit_exports *exports);
> @@ -197,9 +195,8 @@ struct nbdkit_filter {
> nbdkit_backend *nxdata);
> const char *config_help;
> int (*thread_model) (void);
> - int (*get_ready) (nbdkit_next_get_ready *next, nbdkit_backend *nxdata,
> - int thread_model);
> - int (*after_fork) (nbdkit_next_after_fork *next, nbdkit_backend *nxdata);
> + int (*get_ready) (int thread_model);
> + int (*after_fork) (nbdkit_backend *backend);
> int (*preconnect) (nbdkit_next_preconnect *next, nbdkit_backend *nxdata,
> int readonly);
> int (*list_exports) (nbdkit_next_list_exports *next, nbdkit_backend *nxdata,
> diff --git a/server/filters.c b/server/filters.c
> index 31cf5c7e..136fa672 100644
> --- a/server/filters.c
> +++ b/server/filters.c
> @@ -171,33 +171,19 @@ filter_config_complete (struct backend *b)
> b->next->config_complete (b->next);
> }
>
> -static int
> -next_get_ready (struct backend *b)
> -{
> - b->get_ready (b);
> - return 0;
> -}
> -
> static void
> filter_get_ready (struct backend *b)
> {
> struct backend_filter *f = container_of (b, struct backend_filter, backend);
>
> + b->next->get_ready (b->next); /* exits on failure */
> +
> debug ("%s: get_ready thread_model=%d", b->name, thread_model);
>
> if (f->filter.get_ready) {
> - if (f->filter.get_ready (next_get_ready, b->next, thread_model) == -1)
> + if (f->filter.get_ready (thread_model) == -1)
> exit (EXIT_FAILURE);
> }
> - else
> - b->next->get_ready (b->next);
> -}
> -
> -static int
> -next_after_fork (struct backend *b)
> -{
> - b->after_fork (b);
> - return 0;
> }
>
> static void
> @@ -205,14 +191,14 @@ filter_after_fork (struct backend *b)
> {
> struct backend_filter *f = container_of (b, struct backend_filter, backend);
>
> + b->next->after_fork (b->next); /* exits on failure */
> +
> debug ("%s: after_fork", b->name);
>
> if (f->filter.after_fork) {
> - if (f->filter.after_fork (next_after_fork, b->next) == -1)
> + if (f->filter.after_fork (b->next) == -1)
> exit (EXIT_FAILURE);
> }
> - else
> - b->next->after_fork (b->next);
> }
>
[changes to filters omitted]
Yes, this one is pretty obvious in hindsight.
ACK.
Might as well push this one now.
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/
More information about the Libguestfs
mailing list