[Libguestfs] [nbdkit PATCH 4/5] api: Add .cleanup callback

The existing .unload callback is currently called inner-to-outer, which is not suitable for use in late cleanups where a filter wants to make one final call into the underlying plugin. We were careful to state that unload does not have a specific ordering, but switching that ordering may not be helpful. Late cleanup may also want one final chance to open a context into the plugin, which implies one final access to the backend pointer from filters (in contrast to .unload taking no parameters). So, I found it easier to introduce a new .cleanup callback, specifically stated to be the counterpart of .after_fork. This leaves .load/.unload as a pair for handling things at the dlopen barrier (affecting only the local filter or plugin, but no interaction with other modules), and .after_fork/.cleanup for handling things related to the first and last points at which it is safe to interact with the underlying plugin. The new callback has no return value, as by the time we are cleaning up, any failures are not going to change the fact we are shutting down anyway. --- docs/nbdkit-filter.pod | 22 +++++++++++++++++++--- docs/nbdkit-plugin.pod | 33 ++++++++++++++++++++++++++++++--- include/nbdkit-filter.h | 1 + include/nbdkit-plugin.h | 4 +++- server/internal.h | 2 ++ server/filters.c | 13 +++++++++++++ server/main.c | 1 + server/plugins.c | 13 +++++++++++++ tests/test-layers-filter.c | 8 ++++++++ tests/test-layers-plugin.c | 9 ++++++++- tests/test-layers.c | 12 ++++++++++++ 11 files changed, 110 insertions(+), 8 deletions(-) diff --git a/docs/nbdkit-filter.pod b/docs/nbdkit-filter.pod index d2cdde13..aaa7706d 100644 --- a/docs/nbdkit-filter.pod +++ b/docs/nbdkit-filter.pod @@ -147,7 +147,7 @@ initialization outside any connections. The value of C<backend> passed to C<.after_fork> also occurs without connections, but is shared with C<.preconnect>, C<.list_exports>, and C<.default_export>, and can also be obtained from the C<context> passed to C<.open>, and -has a lifetime that lasts to C<.unload> for use by +has a lifetime that lasts to C<.cleanup> for use by C<nbdkit_next_context_open>. In turn, the value of C<context> passed to C<.open> has a lifetime that lasts until the matching C<.close> for use by C<nbdkit_context_get_backend> and C<nbdkit_context_set_next>. @@ -213,7 +213,7 @@ Obtains the backend pointer from the C<context> parameter to C<.open>, matching the backend pointer available to C<.after_fork>, C<.preconnect>, C<.list_exports>, and C<.default_export>. This backend pointer has a stable lifetime from the time of C<.after_fork> -until C<.unload>. +until C<.cleanup>. nbdkit_next *nbdkit_next_context_open (nbdkit_backend *backend, int readonly, const char *exportname, @@ -443,12 +443,28 @@ an error message and return C<-1>. 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 +The C<backend> parameter is valid until C<.cleanup>, 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>. +=head2 C<.cleanup> + + int (cleanup) (nbdkit_backend *backend); + +This optional callback is reached once after all client connections +have been closed, but before the underlying plugin C<.cleanup> or any +C<.unload> callbacks. It can be used by the filter to gracefully +close any background threads created during C<.after_fork>, as well as +close any manual contexts into C<backend> previously opened with +C<nbdkit_next_context_open>. + +Note that it's not guaranteed that C<.cleanup> will always be called +(eg. the server might be killed or segfault), so you should try to +make the filter as robust as possible by not requiring cleanup. See +also L<nbdkit-plugin(3)/SHUTDOWN>. + =head2 C<.preconnect> int (*preconnect) (nbdkit_next_preconnect *next, nbdkit_backend *nxdata, diff --git a/docs/nbdkit-plugin.pod b/docs/nbdkit-plugin.pod index 89374f55..9a36c01d 100644 --- a/docs/nbdkit-plugin.pod +++ b/docs/nbdkit-plugin.pod @@ -176,6 +176,9 @@ the plugin: │ before nbdkit exits │ ┌─────────┴────────┐ + │ cleanup │ + └─────────┬────────┘ + ┌─────────┴────────┐ │ unload │ └──────────────────┘ @@ -281,9 +284,20 @@ The sequence C<.preconnect>, C<.open> ... C<.close> can be called repeatedly over the lifetime of the plugin, and can be called in parallel (depending on the thread model). +=item C<.cleanup> + +is called once after all connections have been closed, prior to +unloading the plugin from memory. This is only called if +C<.after_fork> succeeded earlier (even in cases where nbdkit did not +fork but is running in the foreground), which makes it a good place to +gracefully end any background threads. + =item C<.unload> -is called once just before the plugin is unloaded from memory. +is called once just before the plugin is unloaded from memory. This +is called even when nbdkit did not need to use C<.after_fork> (such as +when using C<--dump-plugin> to display documentation about the +plugin). =back @@ -644,6 +658,18 @@ above. If there is an error, C<.after_fork> should call C<nbdkit_error> with an error message and return C<-1>. +=head2 C<.cleanup> + + void cleanup (void); + +This optional callback is called after the server has closed all +connections and is preparing to unload. It is only reached in the +same cases that the C<.after_fork> callback was used, making it a good +place to clean up any background threads. However, it is not +guaranteed that this callback will be reached, so you should try to +make the plugin as robust as possible by not requiring cleanup. See +also L</SHUTDOWN> below. + =head2 C<.preconnect> int preconnect (int readonly); @@ -1290,8 +1316,9 @@ plugin having to call C<nbdkit_set_error>. When nbdkit receives certain signals it will shut down (see L<nbdkit(1)/SIGNALS>). The server will wait for any currently running -plugin callbacks to finish and also call the C<.unload> callback -before unloading the plugin. +plugin callbacks to finish, call C<.close> on those connections, then +call the C<.cleanup> and C<.unload> callbacks before unloading the +plugin. Note that it's not guaranteed this can always happen (eg. the server might be killed by C<SIGKILL> or segfault). diff --git a/include/nbdkit-filter.h b/include/nbdkit-filter.h index a91de88f..633d34e7 100644 --- a/include/nbdkit-filter.h +++ b/include/nbdkit-filter.h @@ -197,6 +197,7 @@ struct nbdkit_filter { int (*thread_model) (void); int (*get_ready) (int thread_model); int (*after_fork) (nbdkit_backend *backend); + void (*cleanup) (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/include/nbdkit-plugin.h b/include/nbdkit-plugin.h index db010a5a..611987c7 100644 --- a/include/nbdkit-plugin.h +++ b/include/nbdkit-plugin.h @@ -1,5 +1,5 @@ /* nbdkit - * Copyright (C) 2013-2020 Red Hat Inc. + * Copyright (C) 2013-2021 Red Hat Inc. * * Redistribution and use in source and binary forms, with or without * modification, are permitted provided that the following conditions are @@ -144,6 +144,8 @@ struct nbdkit_plugin { struct nbdkit_exports *exports); const char * (*default_export) (int readonly, int is_tls); const char * (*export_description) (void *handle); + + void (*cleanup) (void); }; NBDKIT_EXTERN_DECL (void, nbdkit_set_error, (int err)); diff --git a/server/internal.h b/server/internal.h index d2ddf271..94eb4a9c 100644 --- a/server/internal.h +++ b/server/internal.h @@ -359,6 +359,8 @@ struct backend { const char *(*magic_config_key) (struct backend *); void (*get_ready) (struct backend *); void (*after_fork) (struct backend *); + void (*cleanup) (struct backend *); + int (*preconnect) (struct backend *, int readonly); int (*list_exports) (struct backend *, int readonly, int is_tls, struct nbdkit_exports *exports); diff --git a/server/filters.c b/server/filters.c index 1df8e1c9..d0dd31e5 100644 --- a/server/filters.c +++ b/server/filters.c @@ -201,6 +201,18 @@ filter_after_fork (struct backend *b) } } +static void +filter_cleanup (struct backend *b) +{ + struct backend_filter *f = container_of (b, struct backend_filter, backend); + + debug ("%s: cleanup", b->name); + if (f->filter.cleanup) + f->filter.cleanup (b->next); + + b->next->cleanup (b->next); +} + static int filter_preconnect (struct backend *b, int readonly) { @@ -599,6 +611,7 @@ static struct backend filter_functions = { .magic_config_key = plugin_magic_config_key, .get_ready = filter_get_ready, .after_fork = filter_after_fork, + .cleanup = filter_cleanup, .preconnect = filter_preconnect, .list_exports = filter_list_exports, .default_export = filter_default_export, diff --git a/server/main.c b/server/main.c index 7200647a..74bdf040 100644 --- a/server/main.c +++ b/server/main.c @@ -735,6 +735,7 @@ main (int argc, char *argv[]) start_serving (); + top->cleanup (top); top->free (top); top = NULL; diff --git a/server/plugins.c b/server/plugins.c index 457c8c86..6576a846 100644 --- a/server/plugins.c +++ b/server/plugins.c @@ -164,6 +164,7 @@ plugin_dump_fields (struct backend *b) HAS (thread_model); HAS (get_ready); HAS (after_fork); + HAS (cleanup); HAS (preconnect); HAS (list_exports); HAS (default_export); @@ -273,6 +274,17 @@ plugin_after_fork (struct backend *b) exit (EXIT_FAILURE); } +static void +plugin_cleanup (struct backend *b) +{ + struct backend_plugin *p = container_of (b, struct backend_plugin, backend); + + debug ("%s: cleanup", b->name); + + if (p->plugin.cleanup) + p->plugin.cleanup (); +} + static int plugin_preconnect (struct backend *b, int readonly) { @@ -802,6 +814,7 @@ static struct backend plugin_functions = { .magic_config_key = plugin_magic_config_key, .get_ready = plugin_get_ready, .after_fork = plugin_after_fork, + .cleanup = plugin_cleanup, .preconnect = plugin_preconnect, .list_exports = plugin_list_exports, .default_export = plugin_default_export, diff --git a/tests/test-layers-filter.c b/tests/test-layers-filter.c index 565b5616..380e3cea 100644 --- a/tests/test-layers-filter.c +++ b/tests/test-layers-filter.c @@ -103,6 +103,13 @@ test_layers_filter_after_fork (nbdkit_backend *backend) return 0; } +static void +test_layers_filter_cleanup (nbdkit_backend *backend) +{ + assert (backend == saved_backend); + DEBUG_FUNCTION; +} + static int test_layers_filter_preconnect (nbdkit_next_preconnect *next, nbdkit_backend *nxdata, int readonly) @@ -423,6 +430,7 @@ static struct nbdkit_filter filter = { .thread_model = test_layers_filter_thread_model, .get_ready = test_layers_filter_get_ready, .after_fork = test_layers_filter_after_fork, + .cleanup = test_layers_filter_cleanup, .preconnect = test_layers_filter_preconnect, .list_exports = test_layers_filter_list_exports, .default_export = test_layers_filter_default_export, diff --git a/tests/test-layers-plugin.c b/tests/test-layers-plugin.c index 856e4031..1716b5b9 100644 --- a/tests/test-layers-plugin.c +++ b/tests/test-layers-plugin.c @@ -1,5 +1,5 @@ /* nbdkit - * Copyright (C) 2018-2020 Red Hat Inc. + * Copyright (C) 2018-2021 Red Hat Inc. * * Redistribution and use in source and binary forms, with or without * modification, are permitted provided that the following conditions are @@ -93,6 +93,12 @@ test_layers_plugin_after_fork (void) return 0; } +static void +test_layers_plugin_cleanup (void) +{ + DEBUG_FUNCTION; +} + static int test_layers_plugin_preconnect (int readonly) { @@ -277,6 +283,7 @@ static struct nbdkit_plugin plugin = { .thread_model = test_layers_plugin_thread_model, .get_ready = test_layers_plugin_get_ready, .after_fork = test_layers_plugin_after_fork, + .cleanup = test_layers_plugin_cleanup, .preconnect = test_layers_plugin_preconnect, .list_exports = test_layers_plugin_list_exports, .default_export = test_layers_plugin_default_export, diff --git a/tests/test-layers.c b/tests/test-layers.c index 09edcc0a..13c82289 100644 --- a/tests/test-layers.c +++ b/tests/test-layers.c @@ -609,6 +609,18 @@ main (int argc, char *argv[]) "test_layers_plugin_close", NULL); + /* cleanup methods called in outer-to-inner order. */ + log_verify_seen_in_order + ("testlayersfilter3: cleanup", + "filter3: test_layers_filter_cleanup", + "testlayersfilter2: cleanup", + "filter2: test_layers_filter_cleanup", + "testlayersfilter1: cleanup", + "filter1: test_layers_filter_cleanup", + "testlayersplugin: cleanup", + "test_layers_plugin_cleanup", + NULL); + /* unload methods should be run in any order. */ log_verify_seen ("test_layers_plugin_unload"); log_verify_seen ("filter1: test_layers_filter_unload"); -- 2.31.1