Rich pointed out that making thread_model dynamic even for plugins makes some sense, so here is the code for doing it for 'sh'. I'm less confident on how to do it for OCaml and Rust (not to mention that those allow the plugin to already compile in their model, rather than the language binding glue forcing a model). The other languages (lua, perl, python, ruby) still need to be adjusted to use the v2 API, although they could support this one even without implementing v2. Eric Blake (2): plugins: Add .thread_model callback sh: Implement .thread_model callback docs/nbdkit-plugin.pod | 33 ++++++++++++++++++--- plugins/sh/nbdkit-sh-plugin.pod | 15 ++++++++++ include/nbdkit-plugin.h | 2 ++ plugins/sh/sh.c | 51 +++++++++++++++++++++++++++++++-- server/plugins.c | 12 +++++++- tests/test-eflags.sh | 49 ++++++++++++++++++++----------- 6 files changed, 139 insertions(+), 23 deletions(-) -- 2.20.1
Eric Blake
2019-May-20 12:30 UTC
[Libguestfs] [nbdkit PATCH 1/2] plugins: Add .thread_model callback
Plugins cannot get rid of #define THREAD_MODEL (doing so would break
API/ABI compatibility); however, we CAN add an optional callback for
allowing a runtime reduction of the thread model. This can be
especially useful for language bindings, as in the next patch, where
the C glue code for the language has to cater to the maximum
parallelism possible for that language, but where an individual plugin
may still want to be stricter.
Signed-off-by: Eric Blake <eblake@redhat.com>
---
docs/nbdkit-plugin.pod | 33 +++++++++++++++++++++++++++++----
include/nbdkit-plugin.h | 2 ++
server/plugins.c | 12 +++++++++++-
3 files changed, 42 insertions(+), 5 deletions(-)
diff --git a/docs/nbdkit-plugin.pod b/docs/nbdkit-plugin.pod
index 7f83234..9a20663 100644
--- a/docs/nbdkit-plugin.pod
+++ b/docs/nbdkit-plugin.pod
@@ -133,6 +133,12 @@ has been passed to the plugin.
Both are called after loading the plugin but before any connections
are accepted.
+=item C<.thread_model>
+
+C<.thread_model> is called once after all configuration information
+has been passed to the plugin, and before any connections are
+accepted.
+
=item C<.open>
A new client has connected.
@@ -466,6 +472,19 @@ what already appears in C<.description>.
If the plugin doesn't take any config parameters you should probably
omit this.
+=head2 C<.thread_model>
+
+ int thread_model (void)
+
+This optional callback is called after all the configuration has been
+passed to the plugin. It can be used to force a stricter thread model
+based on configuration, compared to C<THREAD_MODEL>. See
L</THREADS>
+below for details. Attempts to request a looser (more parallel) model
+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<.open>
void *open (int readonly);
@@ -898,9 +917,14 @@ error message, and C<nbdkit_set_error> to record an
appropriate error
=head1 THREADS
-Each nbdkit plugin must declare its thread safety model by defining
-the C<THREAD_MODEL> macro. (This macro is used by
-C<NBDKIT_REGISTER_PLUGIN>).
+Each nbdkit plugin must declare its maximum thread safety model by
+defining the C<THREAD_MODEL> macro. (This macro is used by
+C<NBDKIT_REGISTER_PLUGIN>). Additionally, a plugin may implement the
+C<.thread_model> callback, called right after C<.config_complete>
to
+make a runtime decision on which thread model to use. The nbdkit
+server chooses the most restrictive model between the plugin's
+C<THREAD_MODEL>, the C<.thread_model> if present, and any
restrictions
+requested by filters.
The possible settings for C<THREAD_MODEL> are defined below.
@@ -935,7 +959,8 @@ parallel. However only one request will happen per handle
at a time
=item C<#define THREAD_MODEL NBDKIT_THREAD_MODEL_PARALLEL>
Multiple handles can be open and multiple data requests can happen in
-parallel (even on the same handle).
+parallel (even on the same handle). The server may reorder replies,
+answering a later request before an earlier one.
All the libraries you use must be thread-safe and reentrant. You may
also need to provide mutexes for fields in your connection handle.
diff --git a/include/nbdkit-plugin.h b/include/nbdkit-plugin.h
index e9b1808..632df86 100644
--- a/include/nbdkit-plugin.h
+++ b/include/nbdkit-plugin.h
@@ -130,6 +130,8 @@ struct nbdkit_plugin {
struct nbdkit_extents *extents);
int (*can_cache) (void *handle);
int (*cache) (void *handle, uint32_t count, uint64_t offset, uint32_t flags);
+
+ int (*thread_model) (void);
};
extern void nbdkit_set_error (int err);
diff --git a/server/plugins.c b/server/plugins.c
index f293e6a..85736bb 100644
--- a/server/plugins.c
+++ b/server/plugins.c
@@ -87,8 +87,18 @@ static int
plugin_thread_model (struct backend *b)
{
struct backend_plugin *p = container_of (b, struct backend_plugin, backend);
+ int thread_model = p->plugin._thread_model;
+ int r;
- return p->plugin._thread_model;
+ if (p->plugin.thread_model) {
+ r = p->plugin.thread_model ();
+ if (r == -1)
+ exit (EXIT_FAILURE);
+ if (r < thread_model)
+ thread_model = r;
+ }
+
+ return thread_model;
}
static const char *
--
2.20.1
Eric Blake
2019-May-20 12:30 UTC
[Libguestfs] [nbdkit PATCH 2/2] sh: Implement .thread_model callback
Wire up .thread_model to the sh plugin, with a change to test-eflags
to demonstrate the effect of a script that requests the tighter
SERIALIZE_CONNECTIONS. While at it, fix two swapped tests messed up
in afbcd070.
Signed-off-by: Eric Blake <eblake@redhat.com>
---
plugins/sh/nbdkit-sh-plugin.pod | 15 ++++++++++
plugins/sh/sh.c | 51 +++++++++++++++++++++++++++++++--
tests/test-eflags.sh | 49 ++++++++++++++++++++-----------
3 files changed, 97 insertions(+), 18 deletions(-)
diff --git a/plugins/sh/nbdkit-sh-plugin.pod b/plugins/sh/nbdkit-sh-plugin.pod
index a5285ed..7ba8830 100644
--- a/plugins/sh/nbdkit-sh-plugin.pod
+++ b/plugins/sh/nbdkit-sh-plugin.pod
@@ -180,6 +180,21 @@ ignored.
/path/to/script config_complete
+=item C<thread_model>
+
+ /path/to/script thread_model
+
+On success this should print the desired thread model of the script,
+one of C<"serialize_connections">,
C<"serialize_all_requests">,
+C<"serialize_requests">, or C<"parallel">.
+
+This method is I<not> required; if omitted, then the plugin will be
+executed under the default sh thread model (currently
+C<"serialize_all_requests">, which implies this callback only
makes a
+difference with an output of C<"serialize_connections">). If
an error
+occurs, the script should output an error message and exit with status
+C<1>; unrecognized output is ignored.
+
=item C<open>
/path/to/script open <readonly>
diff --git a/plugins/sh/sh.c b/plugins/sh/sh.c
index 862be21..aeb0191 100644
--- a/plugins/sh/sh.c
+++ b/plugins/sh/sh.c
@@ -257,6 +257,54 @@ sh_config_complete (void)
}
}
+#define THREAD_MODEL NBDKIT_THREAD_MODEL_SERIALIZE_ALL_REQUESTS
+
+static int
+sh_thread_model (void)
+{
+ const char *args[] = { script, "thread_model", NULL };
+ CLEANUP_FREE char *s = NULL;
+ size_t slen;
+ int r;
+
+ switch (call_read (&s, &slen, args)) {
+ case OK:
+ if (slen > 0 && s[slen-1] == '\n')
+ s[slen-1] = '\0';
+ if (strcasecmp (s, "parallel") == 0)
+ r = NBDKIT_THREAD_MODEL_PARALLEL;
+ else if (strcasecmp (s, "serialize_requests") == 0 ||
+ strcasecmp (s, "serialize-requests") == 0)
+ r = NBDKIT_THREAD_MODEL_SERIALIZE_REQUESTS;
+ else if (strcasecmp (s, "serialize_all_requests") == 0 ||
+ strcasecmp (s, "serialize-all-requests") == 0)
+ r = NBDKIT_THREAD_MODEL_SERIALIZE_ALL_REQUESTS;
+ else if (strcasecmp (s, "serialize_connections") == 0 ||
+ strcasecmp (s, "serialize-connections") == 0)
+ r = NBDKIT_THREAD_MODEL_SERIALIZE_CONNECTIONS;
+ else {
+ nbdkit_debug ("%s: ignoring unrecognized thread model: %s",
+ script, s);
+ r = THREAD_MODEL;
+ }
+ return r;
+
+ case MISSING:
+ return THREAD_MODEL;
+
+ case ERROR:
+ return -1;
+
+ case RET_FALSE:
+ nbdkit_error ("%s: %s method returned unexpected code (3/false)",
+ script, "thread_model");
+ errno = EIO;
+ return -1;
+
+ default: abort ();
+ }
+}
+
static void *
sh_open (int readonly)
{
@@ -865,8 +913,6 @@ sh_cache (void *handle, uint32_t count, uint64_t offset,
uint32_t flags)
"script=<FILENAME> (required) The shell script to run.\n"
\
"[other arguments may be used by the plugin that you load]"
-#define THREAD_MODEL NBDKIT_THREAD_MODEL_SERIALIZE_ALL_REQUESTS
-
static struct nbdkit_plugin plugin = {
.name = "sh",
.version = PACKAGE_VERSION,
@@ -878,6 +924,7 @@ static struct nbdkit_plugin plugin = {
.config = sh_config,
.config_complete = sh_config_complete,
.config_help = sh_config_help,
+ .thread_model = sh_thread_model,
.open = sh_open,
.close = sh_close,
diff --git a/tests/test-eflags.sh b/tests/test-eflags.sh
index 8158f75..80816ff 100755
--- a/tests/test-eflags.sh
+++ b/tests/test-eflags.sh
@@ -277,22 +277,6 @@ EOF
# -r
# can_multi_conn=true
-late_args="serialize=connections" do_nbdkit -r --filter=noparallel
<<'EOF'
-case "$1" in
- get_size) echo 1M ;;
- can_multi_conn) exit 0 ;;
- *) exit 2 ;;
-esac
-EOF
-
-[ $eflags -eq $(( HAS_FLAGS|READ_ONLY|SEND_DF )) ] ||
- fail "expected HAS_FLAGS|READ_ONLY|SEND_DF"
-
-#----------------------------------------------------------------------
-# -r
-# --filter=noparallel serialize=connections
-# can_multi_conn=true
-
do_nbdkit -r <<'EOF'
case "$1" in
get_size) echo 1M ;;
@@ -304,6 +288,39 @@ EOF
[ $eflags -eq $(( HAS_FLAGS|READ_ONLY|SEND_DF|CAN_MULTI_CONN )) ] ||
fail "expected HAS_FLAGS|READ_ONLY|SEND_DF|CAN_MULTI_CONN"
+#----------------------------------------------------------------------
+# -r
+# --filter=noparallel serialize=connections
+# can_multi_conn=true
+
+late_args="serialize=connections" do_nbdkit -r --filter=noparallel
<<'EOF'
+case "$1" in
+ get_size) echo 1M ;;
+ can_multi_conn) exit 0 ;;
+ *) exit 2 ;;
+esac
+EOF
+
+[ $eflags -eq $(( HAS_FLAGS|READ_ONLY|SEND_DF )) ] ||
+ fail "expected HAS_FLAGS|READ_ONLY|SEND_DF"
+
+#----------------------------------------------------------------------
+# -r
+# thread_model=serialize_connections
+# can_multi_conn=true
+
+do_nbdkit -r <<'EOF'
+case "$1" in
+ get_size) echo 1M ;;
+ can_multi_conn) exit 0 ;;
+ thread_model) echo "serialize_connections" ;;
+ *) exit 2 ;;
+esac
+EOF
+
+[ $eflags -eq $(( HAS_FLAGS|READ_ONLY|SEND_DF )) ] ||
+ fail "expected HAS_FLAGS|READ_ONLY|SEND_DF"
+
#----------------------------------------------------------------------
# -r
# can_cache=native
--
2.20.1
Richard W.M. Jones
2019-May-20 13:36 UTC
Re: [Libguestfs] [nbdkit PATCH 1/2] plugins: Add .thread_model callback
On Mon, May 20, 2019 at 07:30:31AM -0500, Eric Blake wrote:> +=head2 C<.thread_model> > + > + int thread_model (void) > + > +This optional callback is called after all the configuration has been > +passed to the plugin. It can be used to force a stricter thread model > +based on configuration, compared to C<THREAD_MODEL>. See L</THREADS> > +below for details. Attempts to request a looser (more parallel) model > +are silently ignored. > + > +If there is an error, C<.thread_model> should call C<nbdkit_error> > +with an error message and return C<-1>.Two comments: (1) Do we have an opportunity to change the way the thread model is specified to allow future expansion. This might involve returning something other than the simple int, or could be that we specify different constants to be returned by this call. (Actually while making that point I think I've talked myself out of it. But probably we should do this in the V3 API, so a note can be added to that effect at the end of the TODO file.) (2) Shouldn't it be an error if the thread model returns a more parallel thread model than the constant? Anyway patch series generally looks fine to me. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com Fedora Windows cross-compiler. Compile Windows programs, test, and build Windows installers. Over 100 libraries supported. http://fedoraproject.org/wiki/MinGW