similar to: [PATCH nbdkit] locks: Cache the plugin thread model.

Mostly code motion. --- src/Makefile.am | 1 + src/connections.c | 14 +++---- src/internal.h | 14 ++++--- src/locks.c | 115 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ src/plugins.c | 77 +++++------------------------------- 5 files changed, 142 insertions(+), 79 deletions(-) diff --git a/src/Makefile.am b/src/Makefile.am index 12b9043..1f05eab 100644 ---

Mostly code motion. --- src/Makefile.am | 1 + src/connections.c | 14 +++---- src/internal.h | 14 ++++--- src/locks.c | 115 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ src/plugins.c | 77 +++++------------------------------- 5 files changed, 142 insertions(+), 79 deletions(-) diff --git a/src/Makefile.am b/src/Makefile.am index 12b9043..1f05eab 100644 ---

Low-level pthread locks should not fail except in extreme cases of programmer bugs; we're better off calling attention to such bugs rather than just assuming that they work and continuing on with possibly inconsistent state. Signed-off-by: Eric Blake <eblake@redhat.com> --- server/connections.c | 20 ++++++++++++-------- server/locks.c | 42

--- src/locks.c | 26 ++++++-------------------- 1 file changed, 6 insertions(+), 20 deletions(-) diff --git a/src/locks.c b/src/locks.c index bd8fd99..1724b5a 100644 --- a/src/locks.c +++ b/src/locks.c @@ -56,53 +56,39 @@ lock_init_thread_model (void) void lock_connection (void) { - if (thread_model <= NBDKIT_THREAD_MODEL_SERIALIZE_CONNECTIONS) { - debug ("acquire connection

On Haiku tests/test-socket-activation failed with: nbdkit: locks.c:96:lock_request: conn != NULL called from server/sockets.c: accept_connection in the fallback path which does: lock_request (); thread_data->sock = set_cloexec (accept (listen_sock, NULL, NULL)); unlock_request () Because there is no current connection in this thread this code fails. However it should be possible to

There are several race conditions where nbdkit exits (calling plugin_cleanup() which unloads the plugin), while the plugin is either in a callback or in plugin.close(). Avoid these by: (1) Taking a shared lock around all plugin callbacks. (2) Taking the same as an exclusive lock in plugin_cleanup. This delays plugin_cleanup until all callbacks have finished. (3) Removing a few unnecessary

Introduce the concept of a backend. Currently the only type of backend is a plugin, and there can only be one of them. Instead of calling functions like ‘plugin_pwrite’ you call the backend method ‘backend->pwrite (backend, ...)’. The change is largely mechanical. I was able to remove ‘assert (dl)’ statements throughout since we can now prove they will never be called. Note this does not

Introduce the concept of a backend. Currently the only type of backend is a plugin, and there can only be one of them. Instead of calling functions like ‘plugin_pwrite’ you call the backend method ‘backend->pwrite (backend, ...)’. The change is largely mechanical. I was able to remove ‘assert (dl)’ statements throughout since we can now prove they will never be called. Note this does not

Introduce the concept of a backend. Currently the only type of backend is a plugin, and there can only be one of them. Instead of calling functions like ‘plugin_pwrite’ you call the backend method ‘backend->pwrite (backend, ...)’. The change is largely mechanical. I was able to remove ‘assert (dl)’ statements throughout since we can now prove they will never be called. Note this does not

On 01/16/2018 08:11 AM, Richard W.M. Jones wrote: > Mostly code motion. > --- > src/Makefile.am | 1 + > src/connections.c | 14 +++---- > src/internal.h | 14 ++++--- > src/locks.c | 115 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ > src/plugins.c | 77 +++++------------------------------- > 5 files changed, 142 insertions(+), 79

Annotate some function parameters with attribute((nonnull)). Only do this for internal headers where we are sure that we will be using sufficiently recent GCC or Clang. For the public header files (ie. include/nbdkit-*.h) it may be that people building out of tree plugins are using old GCC which had problems, or even other compilers that don't support this extension at all. Libvirt has an

It's confusing to use the same terminology for a single backend as for the linked list of backends. In particular it's often not clear if we're calling the next backend or the whole chain of backends. --- server/internal.h | 14 ++++++++++-- server/connections.c | 20 ++++++++--------- server/locks.c | 2 +- server/main.c

Since commit 86fdb48c6a5362d66865493d9d2172166f99722e we have stored the connection object in thread-local storage. In this very large, but mostly mechanical change we stop passing the connection pointer around everywhere, and instead use the value stored in thread-local storage. This assumes a 1-1 mapping between the connection and the current thread which is true in *most* places.

The messages are not really useful to us, but they do bloat the debugging output of virt-v2v massively: nbdkit: python[1]: debug: acquire global request lock nbdkit: python[1]: debug: acquire per-connection request lock nbdkit: python[1]: debug: acquire unload prevention lock nbdkit: python[1]: debug: pwrite count=2097152 offset=4628414464 fua=0 nbdkit: python[1]: debug: release unload prevention

This fixes the long-standing crash on close when nbdkit exits. I did try first to fix threads so we're using a proper thread pool, but that's difficult to implement. So this does the minimal change needed to fix the crash instead. There are still two segfaults that happen during running the test suite. One is deliberately caused (tests/test-captive.sh). The other appears to be an

As shown in the previous patch, plugins may choose to use stdin or stdout during .config. But from .get_ready onwards, well-written plugins shouldn't be needing any further use of stdin/out. We already swapped stdin/out to /dev/null while daemonizing, but did not do do during -f or --run, which leads to some surprising inconsistency when trying to debug a plugin that works in the foreground

As shown in the previous patch, plugins may choose to use stdin or stdout during .config. But from .get_ready onwards, well-written plugins shouldn't be needing any further use of stdin/out. We already swapped stdin/out to /dev/null while daemonizing, but did not do do during -f or --run, which leads to some surprising inconsistency when trying to debug a plugin that works in the foreground

Somewhat invasive but mostly mechanical change to how plugins are called. This patch is in preparation for adding a second backend subtype for filters. Rich.

v1 -> v2: - Fixed everything mentioned in the review. Rich.

When accepting a connection on a TCP or Unix domain socket we recorded the peer address in both the thread_data struct and thread-local storage. But for no reason because it was never used anywhere. Since we were only allocating a ‘struct sockaddr’ (rather than a ‘struct sockaddr_storage’) it's likely that some peer addresses would have been truncated. Remove all this code, it had no