similar to: [nbdkit PATCH 0/4] thread-safety issues prior to parallel handling

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

There's enough here to need a review; some of it probably needs backporting to stable-1.12. This probably breaks tests on Haiku or other platforms that have not been as on-the-ball about atomic CLOEXEC; feel free to report issues that arise, and I'll help come up with workarounds (even if we end up leaving a rare fd leak on less-capable systems). Meanwhile, I'm still working on my

GCC is concerned that if we never go round the loop then fd will be uninitialized. By asserting that getaddrinfo set result != NULL we can avoid this. nbd.c: In function ‘nbd_open_handle’: nbd.c:974:5: error: ‘fd’ may be used uninitialized in this function [-Werror=maybe-uninitialized] 974 | close (fd); | ^~~~~~~~~~ nbd.c:954:7: note: ‘fd’ was declared here 954 | int fd;

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

This is a major rewrite compared to my v1 series, where I've tried a lot harder to ensure that we still accommodate building on Haiku (although I have not actually yet fired up a Haiku VM to try it for myself). I also managed to make the sh plugin fully parallel, on capable platforms. See also my question on patch 10 on whether I've picked the best naming convention. Eric Blake (17):

It is never a wise idea to pass stack-allocated storage to another thread during pthread_create() unless you can guarantee that the new thread will complete prior to the calling thread returning and ending the lifetime of that storage. We were violating this, with the result in a SEGV in the detached child thread during threadlocal_set_sockaddr() with parameters pointing into thread_data which

This fixes the race conditions for me, using the test described here: https://www.redhat.com/archives/libguestfs/2017-September/msg00226.html Rich.

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

On 11/16/2017 09:13 PM, Eric Blake wrote: > We can't guarantee what storage duration the caller's request > for a thread name has; and in fact, if the caller uses > plugin_name() for their thread name, then the moment .unload > is called, our threadlocal storage is pointing to la-la-land > and we get a nice SEGV while trying to print any debug message. > So copy the

Version 1 was here: https://www.redhat.com/archives/libguestfs/2019-April/msg00144.html Version 2 makes a couple of much larger changes: The OCaml patch changes the API of the pread method so it matches what other language bindings are already doing, ie. get the language plugin to return a newly allocated buffer, check it is long enough, copy out the data. The server patch implements a

We can't guarantee what storage duration the caller's request for a thread name has; and in fact, if the caller uses plugin_name() for their thread name, then the moment .unload is called, our threadlocal storage is pointing to la-la-land and we get a nice SEGV while trying to print any debug message. So copy the user's string instead. Signed-off-by: Eric Blake

Unlike the equivalent change on the client side which caused a dramatic performance improvement, there is no noticable difference from this patch in my testing. --- server/sockets.c | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/server/sockets.c b/server/sockets.c index 2c71970..b25405c 100644 --- a/server/sockets.c +++ b/server/sockets.c @@ -37,13 +37,15 @@

For GCC only, define unlikely() macro. Use it on error paths to move code out of the hot path. In the server only, use the debug() macro (don't call nbdkit_debug directly). This macro checks the verbose flag and moves the call to nbdkit_debug out of the hot path. --- server/connections.c | 11 ++++++----- server/internal.h | 17 ++++++++++++++++- server/plugins.c | 2 +-

Commit d02d9c9d used pthread_mutex to avoid interleaving output. However, the standard provides flockfile() for grouping related FILE* I/O that must not be interleaved; and that may be lighter-weight than rolling our own locking. Signed-off-by: Eric Blake <eblake@redhat.com> --- Pushing this one; as a related conversation on another mailing list reminded me about flockfile().

On 3/24/20 3:12 PM, Eric Blake wrote: >> (For non-mingw platforms) this breaks the source API promises rather >> seriously, so if I understand your proposal correctly I don't think >> this is a good idea.  It's possibly something we can consider for >> internal plugins, or for the V3 API. > > How does it break API to request that someone link against a

I'm not going to put this upstream because there's no benefit. However it is useful to record the patch on the mailing list. Rich.

v1 was here: https://www.redhat.com/archives/libguestfs/2019-January/msg00008.html In v2 I have provided two patches: The first patch extends attribute((nonnull)) to most internal functions, but not to the external API. The second patch uses a macro so that attribute((format)) is only used in the public API on GCC or Clang. At least in theory these headers could be used by a C compiler which

Fix some things I noticed while reviewing v1, and follow Rich's idea to add a new nbdkit_set_error() utility function with a binding for Python users to request a particular error (rather than being forced to live with whatever stale value is in errno after all the intermediate binding glue code). I could not easily find out how to register a C function callable from perl bindings, and have

On Thu, Nov 16, 2017 at 09:13:55PM -0600, Eric Blake wrote: > Since we construct our error/debug messages via multiple calls > to stdio primitives, we are at risk of multiple threads interleaving > their output if they try to output at once. Add a mutex to group > related outputs into an atomic chunk. > > Signed-off-by: Eric Blake <eblake@redhat.com> > --- >

If bind() fails we _always_ should close socket. I sent this patch while ago to djm but I still don't see this fix in openssh_cvs. diff -urN openssh-2.3.0p1.org/sshd.c openssh-2.3.0p1/sshd.c --- openssh-2.3.0p1.org/sshd.c Sat Jan 6 19:54:11 2001 +++ openssh-2.3.0p1/sshd.c Sat Jan 6 19:55:48 2001 @@ -782,10 +782,10 @@ debug("Bind to port %s on %s.", strport, ntop); /*