[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [Libguestfs] [nbdkit PATCH 1/5] filters: Tighter rules on .get_ready/.after_fork



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/


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]