On Fri, Aug 02, 2019 at 05:33:46PM -0500, Eric Blake wrote: > On platforms that lack atomic CLOEXEC, it's simpler to just always > force serialization (no client thread will be executing when nbdkit > itself is creating a new file descriptor) than to audit which plugins > actually care about not getting any leaked fds. We have one final > function that we need to use for atomic CLOEXEC; the next patch will > actually put accept4() to use. > > Maybe this penalization will encourage Haiku to implement atomic > CLOEXEC sooner. > > Accordingly, the testsuite is tweaked to skip tests that require > parallel execution. > > Signed-off-by: Eric Blake <[email protected]> > --- > > This replaces my .fork_safe patch; I've rebased the rest of the series > on top of it, including fixing the sh default to serialize if there is > no explicit opt-in to parallel, and pushed. > > docs/nbdkit-plugin.pod | 19 +++++++++++++++---- > configure.ac | 1 + > common/utils/utils.c | 6 ++++-- > server/plugins.c | 9 +++++++++ > tests/test-parallel-file.sh | 3 +++ > tests/test-parallel-nbd.sh | 5 ++++- > 6 files changed, 36 insertions(+), 7 deletions(-) > > diff --git a/docs/nbdkit-plugin.pod b/docs/nbdkit-plugin.pod > index fe9ada87..9510253f 100644 > --- a/docs/nbdkit-plugin.pod > +++ b/docs/nbdkit-plugin.pod > @@ -942,8 +942,16 @@ 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. > +C<THREAD_MODEL>, the C<.thread_model> if present, any restrictions > +requested by filters, and any limitations imposed by the system (for > +example, a system without atomic C<FD_CLOEXEC> will serialize all > +requests, so as to avoid nbdkit leaking a new file descriptor from one > +thread into a child process created by another thread). > + > +In C<nbdkit --dump-plugin PLUGIN> output, the C<max_thread_model> line > +matches the C<THREAD_MODEL> macro, and the C<thread_model> line > +matches what the system finally settled on after applying all > +restrictions. > > The possible settings for C<THREAD_MODEL> are defined below. > > @@ -981,8 +989,11 @@ Multiple handles can be open and multiple data requests > can happen in > 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. > +All the libraries you use must be thread-safe and reentrant, and any > +code that creates a file descriptor should atomically set > +C<FD_CLOEXEC> if you do not want it accidentally leaked to another > +thread's child process. You may also need to provide mutexes for > +fields in your connection handle. > > =back > > diff --git a/configure.ac b/configure.ac > index cabec5c8..054ad64a 100644 > --- a/configure.ac > +++ b/configure.ac > @@ -193,6 +193,7 @@ AC_CHECK_HEADERS([\ > > dnl Check for functions in libc, all optional. > AC_CHECK_FUNCS([\ > + accept4 \ > fdatasync \ > get_current_dir_name \ > mkostemp \ > diff --git a/common/utils/utils.c b/common/utils/utils.c > index 9ac3443b..029b6685 100644 > --- a/common/utils/utils.c > +++ b/common/utils/utils.c > @@ -140,13 +140,15 @@ exit_status_to_nbd_error (int status, const char *cmd) > */ > int > set_cloexec (int fd) { > -#if defined SOCK_CLOEXEC && defined HAVE_MKOSTEMP && defined HAVE_PIPE2 > +#if (defined SOCK_CLOEXEC && defined HAVE_MKOSTEMP && defined HAVE_PIPE2 && \ > + defined HAVE_ACCEPT4) > nbdkit_error ("prefer creating fds with CLOEXEC atomically set"); > close (fd); > errno = EBADF; > return -1; > #else > -# if defined SOCK_CLOEXEC || defined HAVE_MKOSTEMP || defined HAVE_PIPE2 > +# if (defined SOCK_CLOEXEC || defined HAVE_MKOSTEMP || defined HAVE_PIPE2 || > \ > + !defined HAVE_ACCEPT4) > # error "Unexpected: your system has incomplete atomic CLOEXEC support" > # endif > int f; > diff --git a/server/plugins.c b/server/plugins.c > index 3bb20c93..be952504 100644 > --- a/server/plugins.c > +++ b/server/plugins.c > @@ -39,6 +39,7 @@ > #include <inttypes.h> > #include <assert.h> > #include <errno.h> > +#include <sys/socket.h> > > #include <dlfcn.h> > > @@ -90,6 +91,14 @@ plugin_thread_model (struct backend *b) > int thread_model = p->plugin._thread_model; > int r; > > +#if !(defined SOCK_CLOEXEC && defined HAVE_MKOSTEMP && defined HAVE_PIPE2 && > \ > + defined HAVE_ACCEPT4) > + if (thread_model > NBDKIT_THREAD_MODEL_SERIALIZE_ALL_REQUESTS) { > + nbdkit_debug ("system lacks atomic CLOEXEC, serializing to avoid fd > leaks"); > + thread_model = NBDKIT_THREAD_MODEL_SERIALIZE_ALL_REQUESTS; > + } > +#endif > + > if (p->plugin.thread_model) { > r = p->plugin.thread_model (); > if (r == -1) > diff --git a/tests/test-parallel-file.sh b/tests/test-parallel-file.sh > index 8335dc99..20276d48 100755 > --- a/tests/test-parallel-file.sh > +++ b/tests/test-parallel-file.sh > @@ -36,6 +36,9 @@ source ./functions.sh > requires test -f file-data > requires qemu-io --version > > +nbdkit --dump-plugin file | grep -q ^thread_model=parallel || > + { echo "nbdkit lacks support for parallel requests"; exit 77; } > + > cleanup_fn rm -f test-parallel-file.data test-parallel-file.out > > # Populate file, and sanity check that qemu-io can issue parallel requests > diff --git a/tests/test-parallel-nbd.sh b/tests/test-parallel-nbd.sh > index 36088fa1..4e546df4 100755 > --- a/tests/test-parallel-nbd.sh > +++ b/tests/test-parallel-nbd.sh > @@ -1,6 +1,6 @@ > #!/usr/bin/env bash > # nbdkit > -# Copyright (C) 2017-2018 Red Hat Inc. > +# Copyright (C) 2017-2019 Red Hat Inc. > # > # Redistribution and use in source and binary forms, with or without > # modification, are permitted provided that the following conditions are > @@ -36,6 +36,9 @@ source ./functions.sh > requires test -f file-data > requires qemu-io --version > > +nbdkit --dump-plugin nbd | grep -q ^thread_model=parallel || > + { echo "nbdkit lacks support for parallel requests"; exit 77; } > + > sock=`mktemp -u` > files="test-parallel-nbd.out $sock test-parallel-nbd.data > test-parallel-nbd.pid" > rm -f $files > -- > 2.20.1
ACK Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com libguestfs lets you edit virtual machines. Supports shell scripting, bindings from many languages. http://libguestfs.org _______________________________________________ Libguestfs mailing list [email protected] https://www.redhat.com/mailman/listinfo/libguestfs
