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. --- docs/nbdkit-filter.pod | 20 +++++++++++++++----- docs/nbdkit-plugin.pod | 27 ++++++++++++++++++++++++++- include/nbdkit-filter.h | 2 ++ include/nbdkit-plugin.h | 2 ++ server/filters.c | 24 ++++++++++++++++++++++++ server/internal.h | 1 + server/main.c | 3 +++ server/plugins.c | 16 ++++++++++++++++ tests/test-layers-filter.c | 11 ++++++++++- tests/test-layers-plugin.c | 8 ++++++++ tests/test-layers.c | 12 ++++++++++++ 11 files changed, 119 insertions(+), 7 deletions(-) diff --git a/docs/nbdkit-filter.pod b/docs/nbdkit-filter.pod index 07ede47..4f10494 100644 --- a/docs/nbdkit-filter.pod +++ b/docs/nbdkit-filter.pod @@ -126,8 +126,8 @@ which is required. =head1 NEXT PLUGIN -F<nbdkit-filter.h> defines three function types -(C<nbdkit_next_config>, C<nbdkit_next_config_complete>, +F<nbdkit-filter.h> defines four function types (C<nbdkit_next_config>, +C<nbdkit_next_config_complete>, C<nbdkit_ready_to_serve>, 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 an opaque pointer C<nxdata> which must be passed along when calling @@ -135,9 +135,10 @@ these functions. =head2 Next config, open and close -The filter’s C<.config>, C<.config_complete> and C<.open> methods may -only call the next C<.config>, C<.config_complete> and C<.open> method -in the chain (optionally for C<.config>). +The filter’s C<.config>, C<.config_complete>, C<.ready_to_serve> and +C<.open> methods may only call the next C<.config>, +C<.config_complete>, C<.ready_to_serve> and C<.open> method in the +chain (optionally for C<.config>). The filter’s C<.close> method is called when an old connection closed, and this has no C<next> parameter because it cannot be @@ -284,6 +285,15 @@ filter doesn't slow down other filters or plugins. If there is an error, C<.thread_model> should call C<nbdkit_error> with an error message and return C<-1>. +=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>. + =head2 C<.open> void * (*open) (nbdkit_next_open *next, void *nxdata, diff --git a/docs/nbdkit-plugin.pod b/docs/nbdkit-plugin.pod index e34ffd1..3ae1303 100644 --- a/docs/nbdkit-plugin.pod +++ 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. + =item C<.open> A new client has connected. @@ -408,7 +415,8 @@ Any other bare parameters give errors. This optional callback is called after all the configuration has been passed to the plugin. It is a good place to do checks, for example -that the user has passed the required parameters to the plugin. +that the user has passed the required parameters to the plugin. It is +also a good place to do general start-up work. If there is an error, C<.config_complete> should call C<nbdkit_error> with an error message and return C<-1>. @@ -437,6 +445,23 @@ are silently ignored. If there is an error, C<.thread_model> should call C<nbdkit_error> with an error message and return C<-1>. +=head2 C<.ready_to_serve> + + int ready_to_serve (void); + +This optional 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. However because nbdkit may have already +forked into the background and so it is difficult to communicate +errors back to the user, this is B<not> a good place to do general +start-up work (use C<.config_complete> instead). + +If there is an error, C<.ready_to_serve> should call C<nbdkit_error> +with an error message and return C<-1>. + =head2 C<.open> void *open (int readonly); diff --git a/include/nbdkit-filter.h b/include/nbdkit-filter.h index c1930c1..7b3a64f 100644 --- a/include/nbdkit-filter.h +++ b/include/nbdkit-filter.h @@ -64,6 +64,7 @@ extern struct nbdkit_extent nbdkit_get_extent (const struct nbdkit_extents *, typedef int nbdkit_next_config (void *nxdata, const char *key, const char *value); typedef int nbdkit_next_config_complete (void *nxdata); +typedef int nbdkit_next_ready_to_serve (void *nxdata); typedef int nbdkit_next_open (void *nxdata, int readonly); @@ -129,6 +130,7 @@ struct nbdkit_filter { int (*config_complete) (nbdkit_next_config_complete *next, void *nxdata); const char *config_help; int (*thread_model) (void); + int (*ready_to_serve) (nbdkit_next_ready_to_serve *next, void *nxdata); void * (*open) (nbdkit_next_open *next, void *nxdata, int readonly); diff --git a/include/nbdkit-plugin.h b/include/nbdkit-plugin.h index 45ae705..746d9a1 100644 --- a/include/nbdkit-plugin.h +++ b/include/nbdkit-plugin.h @@ -134,6 +134,8 @@ struct nbdkit_plugin { int (*thread_model) (void); int (*can_fast_zero) (void *handle); + + int (*ready_to_serve) (void); }; extern void nbdkit_set_error (int err); diff --git a/server/filters.c b/server/filters.c index 1ac4a5f..ed00d11 100644 --- a/server/filters.c +++ b/server/filters.c @@ -190,6 +190,29 @@ plugin_magic_config_key (struct backend *b) return b->next->magic_config_key (b->next); } +static int +next_ready_to_serve (void *nxdata) +{ + struct backend *b = nxdata; + b->ready_to_serve (b); + return 0; +} + +static void +filter_ready_to_serve (struct backend *b) +{ + struct backend_filter *f = container_of (b, struct backend_filter, backend); + + debug ("%s: ready_to_serve", b->name); + + if (f->filter.ready_to_serve) { + if (f->filter.ready_to_serve (next_ready_to_serve, b->next) == -1) + exit (EXIT_FAILURE); + } + else + b->next->ready_to_serve (b->next); +} + static int next_open (void *nxdata, int readonly) { @@ -668,6 +691,7 @@ static struct backend filter_functions = { .config = filter_config, .config_complete = filter_config_complete, .magic_config_key = plugin_magic_config_key, + .ready_to_serve = filter_ready_to_serve, .open = filter_open, .prepare = filter_prepare, .finalize = filter_finalize, diff --git a/server/internal.h b/server/internal.h index 167da59..4c9419d 100644 --- a/server/internal.h +++ b/server/internal.h @@ -312,6 +312,7 @@ struct backend { void (*config) (struct backend *, const char *key, const char *value); void (*config_complete) (struct backend *); const char *(*magic_config_key) (struct backend *); + void (*ready_to_serve) (struct backend *); void *(*open) (struct backend *, struct connection *conn, int readonly); int (*prepare) (struct backend *, struct connection *conn, void *handle, int readonly); diff --git a/server/main.c b/server/main.c index 5623149..935d422 100644 --- a/server/main.c +++ b/server/main.c @@ -857,6 +857,7 @@ start_serving (void) for (i = 0; i < nr_socks; ++i) socks[i] = FIRST_SOCKET_ACTIVATION_FD + i; change_user (); + backend->ready_to_serve (backend); write_pidfile (); accept_incoming_connections (socks, nr_socks); free_listening_sockets (socks, nr_socks); /* also closes them */ @@ -866,6 +867,7 @@ start_serving (void) /* Handling a single connection on stdin/stdout. */ if (listen_stdin) { change_user (); + backend->ready_to_serve (backend); write_pidfile (); threadlocal_new_server_thread (); if (handle_single_connection (0, 1) == -1) @@ -882,6 +884,7 @@ start_serving (void) run_command (); change_user (); fork_into_background (); + backend->ready_to_serve (backend); write_pidfile (); accept_incoming_connections (socks, nr_socks); free_listening_sockets (socks, nr_socks); diff --git a/server/plugins.c b/server/plugins.c index 87daaf2..bb0269d 100644 --- a/server/plugins.c +++ b/server/plugins.c @@ -158,6 +158,7 @@ plugin_dump_fields (struct backend *b) HAS (config); HAS (config_complete); HAS (config_help); + HAS (ready_to_serve); HAS (open); HAS (close); HAS (get_size); @@ -233,6 +234,20 @@ plugin_magic_config_key (struct backend *b) return p->plugin.magic_config_key; } +static void +plugin_ready_to_serve (struct backend *b) +{ + struct backend_plugin *p = container_of (b, struct backend_plugin, backend); + + debug ("%s: ready_to_serve", b->name); + + if (!p->plugin.ready_to_serve) + return; + + if (p->plugin.ready_to_serve () == -1) + exit (EXIT_FAILURE); +} + static void * plugin_open (struct backend *b, struct connection *conn, int readonly) { @@ -656,6 +671,7 @@ static struct backend plugin_functions = { .config = plugin_config, .config_complete = plugin_config_complete, .magic_config_key = plugin_magic_config_key, + .ready_to_serve = plugin_ready_to_serve, .open = plugin_open, .prepare = plugin_prepare, .finalize = plugin_finalize, diff --git a/tests/test-layers-filter.c b/tests/test-layers-filter.c index d590cf9..315fff3 100644 --- a/tests/test-layers-filter.c +++ b/tests/test-layers-filter.c @@ -65,7 +65,7 @@ test_layers_filter_config (nbdkit_next_config *next, void *nxdata, static int test_layers_filter_config_complete (nbdkit_next_config_complete *next, - void *nxdata) + void *nxdata) { DEBUG_FUNCTION; return next (nxdata); @@ -74,6 +74,14 @@ test_layers_filter_config_complete (nbdkit_next_config_complete *next, #define test_layers_filter_config_help \ "test_layers_" layer "_config_help" +static int +test_layers_filter_ready_to_serve (nbdkit_next_ready_to_serve *next, + void *nxdata) +{ + DEBUG_FUNCTION; + return next (nxdata); +} + static void * test_layers_filter_open (nbdkit_next_open *next, void *nxdata, int readonly) { @@ -267,6 +275,7 @@ static struct nbdkit_filter filter = { .config = test_layers_filter_config, .config_complete = test_layers_filter_config_complete, .config_help = test_layers_filter_config_help, + .ready_to_serve = test_layers_filter_ready_to_serve, .open = test_layers_filter_open, .close = test_layers_filter_close, .prepare = test_layers_filter_prepare, diff --git a/tests/test-layers-plugin.c b/tests/test-layers-plugin.c index e9ffd3b..62c0066 100644 --- a/tests/test-layers-plugin.c +++ b/tests/test-layers-plugin.c @@ -72,6 +72,13 @@ test_layers_plugin_config_complete (void) #define test_layers_plugin_config_help "test_layers_plugin_config_help" +static int +test_layers_plugin_ready_to_serve (void) +{ + DEBUG_FUNCTION; + return 0; +} + static void * test_layers_plugin_open (int readonly) { @@ -224,6 +231,7 @@ static struct nbdkit_plugin plugin = { .config = test_layers_plugin_config, .config_complete = test_layers_plugin_config_complete, .config_help = test_layers_plugin_config_help, + .ready_to_serve = test_layers_plugin_ready_to_serve, .open = test_layers_plugin_open, .close = test_layers_plugin_close, .get_size = test_layers_plugin_get_size, diff --git a/tests/test-layers.c b/tests/test-layers.c index eac7f15..c12af47 100644 --- a/tests/test-layers.c +++ b/tests/test-layers.c @@ -282,6 +282,18 @@ main (int argc, char *argv[]) "test_layers_plugin_config_complete", NULL); + /* ready_to_serve methods called in order. */ + log_verify_seen_in_order + ("testlayersfilter3: ready_to_serve", + "filter3: test_layers_filter_ready_to_serve", + "testlayersfilter2: ready_to_serve", + "filter2: test_layers_filter_ready_to_serve", + "testlayersfilter1: ready_to_serve", + "filter1: test_layers_filter_ready_to_serve", + "testlayersplugin: ready_to_serve", + "test_layers_plugin_ready_to_serve", + NULL); + /* open methods called in outer-to-inner order, but thanks to next * pointer, complete in inner-to-outer order. */ log_verify_seen_in_order -- 2.23.0

On Fri, Oct 11, 2019 at 07:59:30AM -0500, Eric Blake wrote: 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. As you say, .unload is the counterpart. (There are other problems with .unload running too late, but they are independent of this patch.) 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/

This finally solves a long-standing problem that the plugin did not follow the peculiar rules that VDDK imposes on multithreaded programming (see "Multithreading Considerations" in the VDDK manual). We add a background thread to the plugin which is responsible for issuing the following VDDK calls: VixDiskLib_InitEx VixDiskLib_Exit VixDiskLib_ConnectEx VixDiskLib_Open VixDiskLib_Close VixDiskLib_Disconnect (Other calls are still made directly by the plugin.) This involves some complicated synchronization but is otherwise uninteresting. This allows us to relax the thread model from SERIALIZE_ALL_REQUESTS to SERIALIZE_REQUESTS. Note that VDDK does not allow parallel requests to be made on one handle, so we cannot relax the model any further. --- plugins/vddk/Makefile.am | 2 + plugins/vddk/octhread.c | 399 +++++++++++++++++++++++++++++++++++++++ plugins/vddk/vddk.c | 190 ++++++------------- plugins/vddk/vddk.h | 89 +++++++++ tests/test-vddk-real.sh | 2 +- 5 files changed, 547 insertions(+), 135 deletions(-) diff --git a/plugins/vddk/Makefile.am b/plugins/vddk/Makefile.am index b806a7d..aff1288 100644 --- a/plugins/vddk/Makefile.am +++ b/plugins/vddk/Makefile.am @@ -42,6 +42,8 @@ plugin_LTLIBRARIES = nbdkit-vddk-plugin.la nbdkit_vddk_plugin_la_SOURCES = \ vddk.c \ + octhread.c \ + vddk.h \ vddk-structs.h \ vddk-stubs.h \ $(top_srcdir)/include/nbdkit-plugin.h \ diff --git a/plugins/vddk/octhread.c b/plugins/vddk/octhread.c new file mode 100644 index 0000000..9da3fb2 --- /dev/null +++ b/plugins/vddk/octhread.c @@ -0,0 +1,399 @@ +/* nbdkit + * Copyright (C) 2013-2019 Red Hat Inc. + * + * Redistribution and use in source and binary forms, with or without + * modification, are permitted provided that the following conditions are + * met: + * + * * Redistributions of source code must retain the above copyright + * notice, this list of conditions and the following disclaimer. + * + * * Redistributions in binary form must reproduce the above copyright + * notice, this list of conditions and the following disclaimer in the + * documentation and/or other materials provided with the distribution. + * + * * Neither the name of Red Hat nor the names of its contributors may be + * used to endorse or promote products derived from this software without + * specific prior written permission. + * + * THIS SOFTWARE IS PROVIDED BY RED HAT AND CONTRIBUTORS ''AS IS'' AND + * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, + * THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A + * PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL RED HAT OR + * CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, + * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT + * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF + * USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND + * ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, + * OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT + * OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF + * SUCH DAMAGE. + */ + +/* This is the Open/Close Thread (octhread). + * + * Because of VDDK's complicated multi-threading requirements (see + * "Multithreading Considerations" in the VDDK manual) we have to + * issue various VDDK calls from a single thread. In this file we + * create the extra thread which always runs while the VDDK plugin is + * loaded. + * + * The main VDDK plugin calls out to this thread synchronously + * whenever it needs to make one of the following calls: + * + * VixDiskLib_InitEx + * VixDiskLib_Exit + * VixDiskLib_ConnectEx + * VixDiskLib_Open + * VixDiskLib_Close + * VixDiskLib_Disconnect + * + * (Any other calls are made directly by the plugin.) + * + * To make one of the above calls, the plugin thread fills in struct + * request with the request type and signals on the request_cond. The + * plugin thread then waits on the return_cond until that is signalled + * which indicates that the request was carried out (or there was an + * error). The return value can be read out of the request struct. + * All this logic is wrapped in the octhread_do_* functions below. + */ + +#include <config.h> + +#include <stdio.h> +#include <stdlib.h> +#include <stdbool.h> +#include <string.h> +#include <errno.h> + +#include <pthread.h> + +#define NBDKIT_API_VERSION 2 +#include <nbdkit-plugin.h> + +#include "cleanup.h" + +#include "vddk.h" + +static pthread_t thread; + +/* Enable debugging of message passing: -D vddk.messagepassing=1 */ +int vddk_debug_messagepassing; + +/* Because the entire purpose of this is to serialize certain + * operations, the request struct can only be used by one calling + * thread. The calling thread must hold request_lock during the + * entire request. + */ +static pthread_mutex_t request_lock = PTHREAD_MUTEX_INITIALIZER; +static struct { + /* Which function to call. */ + enum { NONE = 0, + INITEX = 1, EXIT, CONNECTEX, OPEN, CLOSE, DISCONNECT, + STOP } request_type; + + /* Parameters. */ + VixDiskLibConnectParams *params; + VixDiskLibConnection *connection_ret; + VixDiskLibConnection connection; + VixDiskLibHandle *handle_ret; + VixDiskLibHandle handle; + int readonly; + + /* Return code. */ + bool request_done; + VixError ret; +} request; + +/* Request and return conditions. */ +static pthread_cond_t request_cond = PTHREAD_COND_INITIALIZER; +static pthread_mutex_t request_cond_lock = PTHREAD_MUTEX_INITIALIZER; +static pthread_cond_t return_cond = PTHREAD_COND_INITIALIZER; +static pthread_mutex_t return_cond_lock = PTHREAD_MUTEX_INITIALIZER; + +static inline void +wake_up_octhread (void) +{ + ACQUIRE_LOCK_FOR_CURRENT_SCOPE (&request_cond_lock); + pthread_cond_signal (&request_cond); +} + +static inline void +wait_for_octhread (void) +{ + ACQUIRE_LOCK_FOR_CURRENT_SCOPE (&return_cond_lock); + while (! request.request_done) + pthread_cond_wait (&return_cond, &return_cond_lock); + request.request_done = false; +} + +static VixError +do_request (void) +{ + if (vddk_debug_messagepassing) + nbdkit_debug ("waking up octhread with request %d", request.request_type); + wake_up_octhread (); + if (vddk_debug_messagepassing) + nbdkit_debug ("waiting for octhread"); + wait_for_octhread (); + + return request.ret; +} + +static void *octhread_run (void *); + +/*----------------------------------------------------------------------*/ +/* Plugins call these functions from the normal nbdkit thread context. */ + +/* Start the octhread. Called by vddk_ready_to_serve. */ +void +start_octhread (void) +{ + int err; + + err = pthread_create (&thread, NULL, octhread_run, NULL); + if (err != 0) { + errno = err; + nbdkit_error ("octhread: pthread_create: %m"); + exit (EXIT_FAILURE); + } +} + +/* Terminate the octhread. Called from vddk_unload. */ +void +stop_octhread (void) +{ + ACQUIRE_LOCK_FOR_CURRENT_SCOPE (&request_lock); + + request.request_type = STOP; + do_request (); + + /* We must wait for the thread to exit to avoid races during + * shutdown, eg. if this plugin was unloaded while the thread was + * still exiting. + */ + pthread_join (thread, NULL); +} + +/* Request VixDiskLib_InitEx and wait for it to finish. */ +VixError +octhread_do_VixDiskLib_InitEx (void) +{ + ACQUIRE_LOCK_FOR_CURRENT_SCOPE (&request_lock); + + request.request_type = INITEX; + return do_request (); +} + +/* Request VixDiskLib_Exit and wait for it to finish. */ +VixError +octhread_do_VixDiskLib_Exit (void) +{ + ACQUIRE_LOCK_FOR_CURRENT_SCOPE (&request_lock); + + request.request_type = EXIT; + return do_request (); +} + +/* Request VixDiskLib_ConnectEx and wait for it to finish. */ +VixError +octhread_do_VixDiskLib_ConnectEx (const VixDiskLibConnectParams *params, + int readonly, + VixDiskLibConnection *connection) +{ + ACQUIRE_LOCK_FOR_CURRENT_SCOPE (&request_lock); + + request.request_type = CONNECTEX; + request.params = (VixDiskLibConnectParams *) params; + request.readonly = readonly; + request.connection_ret = connection; + return do_request (); +} + +/* Request VixDiskLib_Open and wait for it to finish. */ +VixError +octhread_do_VixDiskLib_Open (const VixDiskLibConnection connection, + int readonly, + VixDiskLibHandle *handle) +{ + ACQUIRE_LOCK_FOR_CURRENT_SCOPE (&request_lock); + + request.request_type = OPEN; + request.connection = connection; + request.readonly = readonly; + request.handle_ret = handle; + return do_request (); +} + +/* Request VixDiskLib_Close and wait for it to finish. */ +VixError +octhread_do_VixDiskLib_Close (VixDiskLibHandle handle) +{ + ACQUIRE_LOCK_FOR_CURRENT_SCOPE (&request_lock); + + request.request_type = CLOSE; + request.handle = handle; + return do_request (); +} + +/* Request VixDiskLib_Disconnect and wait for it to finish. */ +VixError +octhread_do_VixDiskLib_Disconnect (VixDiskLibConnection connection) +{ + ACQUIRE_LOCK_FOR_CURRENT_SCOPE (&request_lock); + + request.request_type = DISCONNECT; + request.connection = connection; + return do_request (); +} + +/*----------------------------------------------------------------------*/ +/* The octhread itself. */ + +static void +trim (char *str) +{ + size_t len = strlen (str); + + if (len > 0 && str[len-1] == '\n') + str[len-1] = '\0'; +} + +/* Turn log messages from the library into nbdkit_debug. */ +static void +debug_function (const char *fs, va_list args) +{ + CLEANUP_FREE char *str = NULL; + + if (vasprintf (&str, fs, args) == -1) { + nbdkit_debug ("lost debug message: %s", fs); + return; + } + + trim (str); + + nbdkit_debug ("%s", str); +} + +/* Turn error messages from the library into nbdkit_error. */ +static void +error_function (const char *fs, va_list args) +{ + CLEANUP_FREE char *str = NULL; + + if (vasprintf (&str, fs, args) == -1) { + nbdkit_error ("lost error message: %s", fs); + return; + } + + trim (str); + + nbdkit_error ("%s", str); +} + +static void * +octhread_run (void *arg) +{ + int err; + uint32_t flags; + bool stop = false; + + nbdkit_debug ("octhread started"); + + while (!stop) { + /* Wait for an incoming request. */ + { + ACQUIRE_LOCK_FOR_CURRENT_SCOPE (&request_cond_lock); + while (request.request_type == NONE) + pthread_cond_wait (&request_cond, &request_cond_lock); + } + + if (vddk_debug_messagepassing) + nbdkit_debug ("octhread: request %d", request.request_type); + + switch (request.request_type) { + case INITEX: + DEBUG_CALL ("VixDiskLib_InitEx", + "%d, %d, &debug_fn, &error_fn, &error_fn, %s, %s", + VDDK_MAJOR, VDDK_MINOR, + libdir ? : VDDK_LIBDIR, config ? : "NULL"); + err = VixDiskLib_InitEx (VDDK_MAJOR, VDDK_MINOR, + &debug_function, /* log function */ + &error_function, /* warn function */ + &error_function, /* panic function */ + libdir ? : VDDK_LIBDIR, config); + break; + + case EXIT: + DEBUG_CALL ("VixDiskLib_Exit", ""); + VixDiskLib_Exit (); + err = VIX_OK; + break; + + case CONNECTEX: + DEBUG_CALL ("VixDiskLib_ConnectEx", + "params, %d, %s, %s, &connection", + request.readonly, + snapshot_moref ? : "NULL", + transport_modes ? : "NULL"); + err = VixDiskLib_ConnectEx (request.params, + request.readonly, + snapshot_moref, + transport_modes, + request.connection_ret); + break; + + case OPEN: + flags = 0; + if (request.readonly) + flags |= VIXDISKLIB_FLAG_OPEN_READ_ONLY; + if (single_link) + flags |= VIXDISKLIB_FLAG_OPEN_SINGLE_LINK; + if (unbuffered) + flags |= VIXDISKLIB_FLAG_OPEN_UNBUFFERED; + DEBUG_CALL ("VixDiskLib_Open", + "connection, %s, %d, &handle", filename, flags); + err = VixDiskLib_Open (request.connection, filename, flags, + request.handle_ret); + break; + + case CLOSE: + DEBUG_CALL ("VixDiskLib_Close", "handle"); + err = VixDiskLib_Close (request.handle); + break; + + case DISCONNECT: + DEBUG_CALL ("VixDiskLib_Disconnect", "connection"); + err = VixDiskLib_Disconnect (request.connection); + break; + + case STOP: + stop = true; + err = VIX_OK; + break; + + case NONE: + default: + abort (); + } + + request.request_type = NONE; + + /* Set up return field and signal to plugin thread that we have + * finished. + */ + if (vddk_debug_messagepassing) + nbdkit_debug ("octhread: return %d", err); + request.ret = err; + request.request_done = true; + + { + ACQUIRE_LOCK_FOR_CURRENT_SCOPE (&return_cond_lock); + pthread_cond_signal (&return_cond); + } + } /* for (;;) */ + + nbdkit_debug ("octhread exiting"); + + pthread_exit (NULL); +} diff --git a/plugins/vddk/vddk.c b/plugins/vddk/vddk.c index 5d3764d..375fbfa 100644 --- a/plugins/vddk/vddk.c +++ b/plugins/vddk/vddk.c @@ -42,7 +42,6 @@ #include <dlfcn.h> #define NBDKIT_API_VERSION 2 - #include <nbdkit-plugin.h> #include "cleanup.h" @@ -50,7 +49,7 @@ #include "minmax.h" #include "rounding.h" -#include "vddk-structs.h" +#include "vddk.h" /* Enable extra disk info debugging with: -D vddk.diskinfo=1 */ int vddk_debug_diskinfo; @@ -58,90 +57,34 @@ int vddk_debug_diskinfo; /* Enable debugging of extents code with: -D vddk.extents=1 */ int vddk_debug_extents; -/* For each VDDK API define a static global variable. These globals - * are initialized when the plugin is loaded (by vddk_load). +/* For each VDDK API define a global variable. These globals are + * initialized when the plugin is loaded (by vddk_load). */ -#define STUB(fn,ret,args) static ret (*fn) args -#define OPTIONAL_STUB(fn,ret,args) static ret (*fn) args +#define STUB(fn,ret,args) ret (*fn) args +#define OPTIONAL_STUB(fn,ret,args) ret (*fn) args #include "vddk-stubs.h" #undef STUB #undef OPTIONAL_STUB -/* Parameters passed to InitEx. */ -#define VDDK_MAJOR 5 -#define VDDK_MINOR 1 - static void *dl = NULL; /* dlopen handle */ -static int init_called = 0; /* was InitEx called */ - -static char *config = NULL; /* config */ -static const char *cookie = NULL; /* cookie */ -static const char *filename = NULL; /* file */ -static char *libdir = NULL; /* libdir */ -static uint16_t nfc_host_port = 0; /* nfchostport */ -static char *password = NULL; /* password */ -static uint16_t port = 0; /* port */ -static const char *server_name = NULL; /* server */ -static bool single_link = false; /* single-link */ -static const char *snapshot_moref = NULL; /* snapshot */ -static const char *thumb_print = NULL; /* thumbprint */ -static const char *transport_modes = NULL; /* transports */ -static bool unbuffered = false; /* unbuffered */ -static const char *username = NULL; /* user */ -static const char *vmx_spec = NULL; /* vm */ -static bool is_remote = false; - -#define VDDK_ERROR(err, fs, ...) \ - do { \ - char *vddk_err_msg; \ - vddk_err_msg = VixDiskLib_GetErrorText ((err), NULL); \ - nbdkit_error (fs ": %s", ##__VA_ARGS__, vddk_err_msg); \ - VixDiskLib_FreeErrorText (vddk_err_msg); \ - } while (0) - -#define DEBUG_CALL(fn, fs, ...) \ - nbdkit_debug ("VDDK call: %s (" fs ")", fn, ##__VA_ARGS__) - -static void -trim (char *str) -{ - size_t len = strlen (str); - - if (len > 0 && str[len-1] == '\n') - str[len-1] = '\0'; -} - -/* Turn log messages from the library into nbdkit_debug. */ -static void -debug_function (const char *fs, va_list args) -{ - CLEANUP_FREE char *str = NULL; - - if (vasprintf (&str, fs, args) == -1) { - nbdkit_debug ("lost debug message: %s", fs); - return; - } - - trim (str); - - nbdkit_debug ("%s", str); -} - -/* Turn error messages from the library into nbdkit_error. */ -static void -error_function (const char *fs, va_list args) -{ - CLEANUP_FREE char *str = NULL; - - if (vasprintf (&str, fs, args) == -1) { - nbdkit_error ("lost error message: %s", fs); - return; - } - - trim (str); - - nbdkit_error ("%s", str); -} +static bool octhread_running = false; /* octhread is running */ + +char *config = NULL; /* config */ +const char *cookie = NULL; /* cookie */ +const char *filename = NULL; /* file */ +char *libdir = NULL; /* libdir */ +uint16_t nfc_host_port = 0; /* nfchostport */ +char *password = NULL; /* password */ +uint16_t port = 0; /* port */ +const char *server_name = NULL; /* server */ +bool single_link = false; /* single-link */ +const char *snapshot_moref = NULL; /* snapshot */ +const char *thumb_print = NULL; /* thumbprint */ +const char *transport_modes = NULL; /* transports */ +bool unbuffered = false; /* unbuffered */ +const char *username = NULL; /* user */ +const char *vmx_spec = NULL; /* vm */ +bool is_remote = false; /* is a remote connection? */ /* Load and unload the plugin. */ static void @@ -194,9 +137,9 @@ vddk_load (void) static void vddk_unload (void) { - if (init_called) { - DEBUG_CALL ("VixDiskLib_Exit", ""); - VixDiskLib_Exit (); + if (octhread_running) { + octhread_do_VixDiskLib_Exit (); + stop_octhread (); } if (dl) dlclose (dl); @@ -292,8 +235,6 @@ vddk_config (const char *key, const char *value) static int vddk_config_complete (void) { - VixError err; - if (filename == NULL) { nbdkit_error ("you must supply the file=<FILENAME> parameter " "after the plugin name on the command line"); @@ -330,29 +271,31 @@ vddk_config_complete (void) #undef missing } + return 0; +} + +#define vddk_config_help \ + "file=<FILENAME> (required) The filename (eg. VMDK file) to serve.\n" \ + "Many optional parameters are supported, see nbdkit-vddk-plugin(3)." + +static int +vddk_ready_to_serve (void) +{ + VixError err; + + start_octhread (); + /* Initialize VDDK library. */ - DEBUG_CALL ("VixDiskLib_InitEx", - "%d, %d, &debug_fn, &error_fn, &error_fn, %s, %s", - VDDK_MAJOR, VDDK_MINOR, - libdir ? : VDDK_LIBDIR, config ? : "NULL"); - err = VixDiskLib_InitEx (VDDK_MAJOR, VDDK_MINOR, - &debug_function, /* log function */ - &error_function, /* warn function */ - &error_function, /* panic function */ - libdir ? : VDDK_LIBDIR, config); + err = octhread_do_VixDiskLib_InitEx (); if (err != VIX_OK) { VDDK_ERROR (err, "VixDiskLib_InitEx"); - exit (EXIT_FAILURE); + return -1; } - init_called = 1; + octhread_running = true; return 0; } -#define vddk_config_help \ - "file=<FILENAME> (required) The filename (eg. VMDK file) to serve.\n" \ - "Many optional parameters are supported, see nbdkit-vddk-plugin(3)." - static void vddk_dump_plugin (void) { @@ -376,11 +319,11 @@ vddk_dump_plugin (void) #endif } -/* XXX To really do threading correctly in accordance with the VDDK - * documentation, we must do all open/close calls from a single - * thread. This is a huge pain. +/* See "Multithreading Considerations" in the VDDK manual. VDDK does + * not allow parallel requests on the same handle, so we cannot use + * the parallel thread model. */ -#define THREAD_MODEL NBDKIT_THREAD_MODEL_SERIALIZE_ALL_REQUESTS +#define THREAD_MODEL NBDKIT_THREAD_MODEL_SERIALIZE_REQUESTS /* The per-connection handle. */ struct vddk_handle { @@ -424,7 +367,6 @@ vddk_open (int readonly) { struct vddk_handle *h; VixError err; - uint32_t flags; h = malloc (sizeof *h); if (h == NULL) { @@ -462,33 +404,15 @@ vddk_open (int readonly) * Advanced Transport modes, but I could not make it work with * either ESXi or vCenter servers. */ - - DEBUG_CALL ("VixDiskLib_ConnectEx", - "h->params, %d, %s, %s, &connection", - readonly, - snapshot_moref ? : "NULL", - transport_modes ? : "NULL"); - err = VixDiskLib_ConnectEx (h->params, - readonly, - snapshot_moref, - transport_modes, - &h->connection); + err = octhread_do_VixDiskLib_ConnectEx (h->params, readonly, + &h->connection); if (err != VIX_OK) { VDDK_ERROR (err, "VixDiskLib_ConnectEx"); goto err1; } - flags = 0; - if (readonly) - flags |= VIXDISKLIB_FLAG_OPEN_READ_ONLY; - if (single_link) - flags |= VIXDISKLIB_FLAG_OPEN_SINGLE_LINK; - if (unbuffered) - flags |= VIXDISKLIB_FLAG_OPEN_UNBUFFERED; - - DEBUG_CALL ("VixDiskLib_Open", - "connection, %s, %d, &handle", filename, flags); - err = VixDiskLib_Open (h->connection, filename, flags, &h->handle); + err = octhread_do_VixDiskLib_Open (h->connection, readonly, + &h->handle); if (err != VIX_OK) { VDDK_ERROR (err, "VixDiskLib_Open: %s", filename); goto err2; @@ -500,8 +424,7 @@ vddk_open (int readonly) return h; err2: - DEBUG_CALL ("VixDiskLib_Disconnect", "connection"); - VixDiskLib_Disconnect (h->connection); + octhread_do_VixDiskLib_Disconnect (h->connection); err1: free_connect_params (h->params); err0: @@ -515,10 +438,8 @@ vddk_close (void *handle) { struct vddk_handle *h = handle; - DEBUG_CALL ("VixDiskLib_Close", "handle"); - VixDiskLib_Close (h->handle); - DEBUG_CALL ("VixDiskLib_Disconnect", "connection"); - VixDiskLib_Disconnect (h->connection); + octhread_do_VixDiskLib_Close (h->handle); + octhread_do_VixDiskLib_Disconnect (h->connection); free_connect_params (h->params); free (h); } @@ -833,10 +754,11 @@ static struct nbdkit_plugin plugin = { .version = PACKAGE_VERSION, .load = vddk_load, .unload = vddk_unload, + .dump_plugin = vddk_dump_plugin, .config = vddk_config, .config_complete = vddk_config_complete, .config_help = vddk_config_help, - .dump_plugin = vddk_dump_plugin, + .ready_to_serve = vddk_ready_to_serve, .open = vddk_open, .close = vddk_close, .get_size = vddk_get_size, diff --git a/plugins/vddk/vddk.h b/plugins/vddk/vddk.h new file mode 100644 index 0000000..7d012eb --- /dev/null +++ b/plugins/vddk/vddk.h @@ -0,0 +1,89 @@ +/* nbdkit + * Copyright (C) 2013-2019 Red Hat Inc. + * + * Redistribution and use in source and binary forms, with or without + * modification, are permitted provided that the following conditions are + * met: + * + * * Redistributions of source code must retain the above copyright + * notice, this list of conditions and the following disclaimer. + * + * * Redistributions in binary form must reproduce the above copyright + * notice, this list of conditions and the following disclaimer in the + * documentation and/or other materials provided with the distribution. + * + * * Neither the name of Red Hat nor the names of its contributors may be + * used to endorse or promote products derived from this software without + * specific prior written permission. + * + * THIS SOFTWARE IS PROVIDED BY RED HAT AND CONTRIBUTORS ''AS IS'' AND + * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, + * THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A + * PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL RED HAT OR + * CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, + * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT + * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF + * USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND + * ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, + * OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT + * OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF + * SUCH DAMAGE. + */ + +#ifndef NBDKIT_VDDK_H +#define NBDKIT_VDDK_H + +#include "vddk-structs.h" + +/* For each VDDK API define an extern global variable. These globals + * are initialized when the plugin is loaded (by vddk_load). + */ +#define STUB(fn,ret,args) extern ret (*fn) args +#define OPTIONAL_STUB(fn,ret,args) extern ret (*fn) args +#include "vddk-stubs.h" +#undef STUB +#undef OPTIONAL_STUB + +extern char *config; +extern const char *cookie; +extern const char *filename; +extern char *libdir; +extern uint16_t nfc_host_port; +extern char *password; +extern uint16_t port; +extern const char *server_name; +extern bool single_link; +extern const char *snapshot_moref; +extern const char *thumb_print; +extern const char *transport_modes; +extern bool unbuffered; +extern const char *username; +extern const char *vmx_spec; +extern bool is_remote; + +#define VDDK_ERROR(err, fs, ...) \ + do { \ + char *vddk_err_msg; \ + vddk_err_msg = VixDiskLib_GetErrorText ((err), NULL); \ + nbdkit_error (fs ": %s", ##__VA_ARGS__, vddk_err_msg); \ + VixDiskLib_FreeErrorText (vddk_err_msg); \ + } while (0) + +#define DEBUG_CALL(fn, fs, ...) \ + nbdkit_debug ("VDDK call: %s (" fs ")", fn, ##__VA_ARGS__) + +/* Parameters passed to InitEx. */ +#define VDDK_MAJOR 5 +#define VDDK_MINOR 1 + +/* Functions in octhread.c */ +extern void start_octhread (void); +extern void stop_octhread (void); +extern VixError octhread_do_VixDiskLib_InitEx (void); +extern VixError octhread_do_VixDiskLib_Exit (void); +extern VixError octhread_do_VixDiskLib_ConnectEx (const VixDiskLibConnectParams *params, int readonly, VixDiskLibConnection *connection); +extern VixError octhread_do_VixDiskLib_Open (const VixDiskLibConnection connection, int readonly, VixDiskLibHandle *handle); +extern VixError octhread_do_VixDiskLib_Close (VixDiskLibHandle handle); +extern VixError octhread_do_VixDiskLib_Disconnect (VixDiskLibConnection connection); + +#endif /* NBDKIT_VDDK_H */ diff --git a/tests/test-vddk-real.sh b/tests/test-vddk-real.sh index 60f634c..6b31808 100755 --- a/tests/test-vddk-real.sh +++ b/tests/test-vddk-real.sh @@ -57,7 +57,7 @@ qemu-img create -f vmdk test-vddk-real.vmdk 100M export old_ld_library_path="$LD_LIBRARY_PATH" export LD_LIBRARY_PATH="$vddkdir/$lib:$LD_LIBRARY_PATH" -nbdkit -f -v -U - \ +nbdkit -f -v -U - -D vddk.messagepassing=1 \ --filter=readahead \ vddk libdir="$vddkdir" file=test-vddk-real.vmdk \ --run ' -- 2.23.0