search for: status_pip

The previous incomplete patch was here: https://www.redhat.com/archives/libguestfs/2019-September/msg00049.html based on earlier discussion here: https://www.redhat.com/archives/libguestfs/2019-September/msg00047.html In v2: - The previous patch was incomplete. This version completes it by adding tests and extending nbdkit-sh-plugin. - nbdkit_export_name now returns NULL for error,

Patch itself is not controversial. However I do wonder if we want to change all these constructs so that instead of using #ifdef we use something like: if (HAVE_PIPE2) { // normal path } else { // fallback } (It wouldn't actually work as written above because HAVE_PIPE2 is not always defined, but you get the idea.) This would allow us to test that the fallback paths still

...ections.c +++ b/server/connections.c @@ -277,25 +277,25 @@ new_connection (int sockin, int sockout, int nworkers) * non-atomicity okay. */ assert (thread_model <= NBDKIT_THREAD_MODEL_SERIALIZE_ALL_REQUESTS); - lock_request (NULL); + lock_request (); if (pipe (conn->status_pipe)) { perror ("pipe"); - unlock_request (NULL); + unlock_request (); goto error2; } if (set_nonblock (set_cloexec (conn->status_pipe[0])) == -1) { perror ("fcntl"); close (conn->status_pipe[1]); - unlock_request (NULL); +...

We noticed while writing various libnbd tests that when the delay filter is in use, there are scenarios where we had to resort to SIGKILL to get rid of nbdkit, because it was non-responsive to SIGINT. I'm still trying to figure out the best way to add testsuite coverage of this, but already proved to myself that it works from the command line, under two scenarios that both used to cause long

...-- 5 files changed, 27 insertions(+), 11 deletions(-) diff --git a/server/connections.c b/server/connections.c index c55d381..95d8296 100644 --- a/server/connections.c +++ b/server/connections.c @@ -97,7 +97,7 @@ connection_set_status (struct connection *conn, int value) assert (conn->status_pipe[1] >= 0); if (write (conn->status_pipe[1], &c, 1) != 1 && errno != EAGAIN) - nbdkit_debug ("failed to notify pipe-to-self: %m"); + debug ("failed to notify pipe-to-self: %m"); } conn->status = value; } @@ -176,7 +176,7 @@ _...

Since the v1 series (0/4, at [1]), I've applied patches 1 and 2, rewritten patch 3 [Forbid NUL in export and context names] into patch 4 here, patch 4 there turned into patch 6 here, and everything else here is new. [1]https://www.redhat.com/archives/libguestfs/2019-September/msg00180.html I don't know if there is a handy reusable function for checking whether a string contains valid

...> continue on with the transaction. > > +++ b/server/public.c > +#else > + struct timespec ts; > + struct connection *conn = threadlocal_get_conn (); > + struct pollfd fds[2] = { > + [0].fd = quit_fd, > + [0].events = POLLIN, > + [1].fd = conn ? conn->status_pipe[0] : -1, > + [1].events = POLLIN, In testing this, the code is responsive to a multi-threaded connection detecting client death on any other thread, but not responsive to a single-threaded connection detecting client death on the lone thread. But even with a multi-threaded connection, if ev...

...ockout) /* Finalize (for filters), called just before close. */ lock_request (); - r = backend_finalize (backend); + r = backend_finalize (top); unlock_request (); if (r == -1) goto done; @@ -251,12 +251,12 @@ new_connection (int sockin, int sockout, int nworkers) conn->status_pipe[0] = conn->status_pipe[1] = -1; - conn->handles = calloc (backend->i + 1, sizeof *conn->handles); + conn->handles = calloc (top->i + 1, sizeof *conn->handles); if (conn->handles == NULL) { perror ("malloc"); goto error; } - conn->nr_handles...

...ror ("nbdkit_nanosleep not yet ported to systems without ppoll"); + errno = ENOSYS; + return -1; +#else + struct timespec ts; + struct connection *conn = threadlocal_get_conn (); + struct pollfd fds[2] = { + [0].fd = quit_fd, + [0].events = POLLIN, + [1].fd = conn ? conn->status_pipe[0] : -1, + [1].events = POLLIN, + }; + sigset_t all; + + if (sec >= INT_MAX - nsec / 1000000000) { + nbdkit_error ("sleep request is too long"); + errno = EINVAL; + return -1; + } + ts.tv_sec = sec + nsec / 1000000000; + ts.tv_nsec = nsec % 1000000000; + + /* Block...

When I added .can_FOO caching in 1.16, I missed the case that the sh plugin itself was calling .can_flush twice in some situations (in order to default .can_fua). Then right after, I regressed it to call .can_zero twice (in order to default .can_fast_zero). I also missed that .thread_model could use better caching, because at the time, I did not add testsuite coverage. Fix that now. Eric Blake

...,9 @@ free_connection (struct connection *conn) * callback should always be called. */ if (!quit) { - lock_request (conn); - backend_close (backend, conn); - unlock_request (conn); + lock_request (); + backend_close (backend); + unlock_request (); } if (conn->status_pipe[0] >= 0) { @@ -375,9 +378,9 @@ free_connection (struct connection *conn) * that this send will be followed by related data. */ static int -raw_send_socket (struct connection *conn, const void *vbuf, size_t len, - int flags) +raw_send_socket (const void *vbuf, size_t len, in...

v1 was here: https://www.redhat.com/archives/libguestfs/2020-February/msg00081.html v2 replaces struct connection *conn = GET_CONN; with GET_CONN; which sets conn implicitly and asserts that it is non-NULL. If we actually want to test if conn is non-NULL or behave differently, then you must use threadlocal_get_conn() instead, and some existing uses do that. Rich.

The third patch is a large but mechanical change which gets rid of passing around struct connection * entirely within the server, preferring instead to reference the connection through thread-local storage. I hope this is a gateway to simplifying other parts of the code. Rich.

It's easy to use the sh script to demonstrate that nbdkit is inefficiently calling into .get_size, .can_fua, and friends more than necessary. We've also commented on the list in the past that it would be nice to ensure that when filters call into next_ops, they are not violating constraints (as we've have to fix several bugs in the past where we did not have such checking to protect