search for: backend_close

..._CONNECTED); + if (b->finalize (b, conn, h->handle) == -1) { + h->state |= HANDLE_FAILED; + return -1; + } + } + else + assert (! (h->state & HANDLE_CONNECTED)); + if (b->i) return backend_finalize (b->next, conn); return 0; @@ -243,7 +259,12 @@ backend_close (struct backend *b, struct connection *conn) /* outer-to-inner order, opposite .open */ debug ("%s: close", b->name); - b->close (b, conn, h->handle); + if (h->handle) { + assert (h->state & HANDLE_OPEN); + b->close (b, conn, h->handle); + } + els...

...a/server/internal.h +++ b/server/internal.h @@ -334,9 +334,11 @@ extern int backend_open (struct backend *b, struct connection *conn, __attribute__((__nonnull__ (1, 2))); extern int backend_prepare (struct backend *b, struct connection *conn) __attribute__((__nonnull__ (1, 2))); +extern void backend_close (struct backend *b, struct connection *conn) + __attribute__((__nonnull__ (1, 2))); extern void backend_set_handle (struct backend *b, struct connection *conn, void *handle) - __attribute__((__nonnull__ (1, 2 /* not 3 */))); + __attribute__((__nonnull__ (1, 2, 3...

The first one is the nastiest - it is an assertion failure caused by a spec-compliant client and introduced by our security fix that was released in 1.14.1. Eric Blake (4): server: Fix regression for NBD_OPT_INFO before NBD_OPT_GO server: Fix back-to-back SET_META_CONTEXT server: Forbid NUL in export and context names server: Fix OPT_GO on different export than SET_META_CONTEXT

...Suppress the debug message unless the callback is actually being called. --- server/backend.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/backend.c b/server/backend.c index 51b56a48..108f4a25 100644 --- a/server/backend.c +++ b/server/backend.c @@ -241,10 +241,10 @@ backend_close (struct backend *b) struct handle *h = get_handle (conn, b->i); /* outer-to-inner order, opposite .open */ - controlpath_debug ("%s: close", b->name); if (h->handle) { assert (h->state & HANDLE_OPEN); + controlpath_debug ("%s: close", b->...

I think this is my last round of patches for issues I identified with the retry filter. With this in place, it should be safe to interject another filter in between retry and the plugin. Eric Blake (5): retry: Don't call into closed plugin tests: Refactor test-retry-reopen-fail.sh tests: Enhance retry test to cover failed reopen server: Move prepare/finalize/close recursion to

.../* Wrappers for all callbacks in a filter's struct nbdkit_next_ops. */ > > +int > +backend_reopen (struct backend *b, struct connection *conn, int readonly) > +{ > + debug ("%s: reopen", b->name); I'd also debug the value of readonly=%d here. > + > + backend_close (b, conn); > + return backend_open (b, conn, readonly); > +} Your followup patch about only calling backend_close if h->handle is non-NULL is correct. Also, this gets us into weird territory. Previously, we could claim that if handles[0]->handle is non-NULL, then when the connection...

...ct connection *conn, int readonly) if (b->i) /* A filter must not succeed unless its backend did also */ assert (conn->handles[b->i - 1].handle); } - else + else { assert (h->handle == NULL); + if (b->i) /* Do not strand backend if this layer failed */ + backend_close (b->next, conn); + } return r; } diff --git a/server/connections.c b/server/connections.c index 27cf202b..df5e09af 100644 --- a/server/connections.c +++ b/server/connections.c @@ -360,7 +360,7 @@ free_connection (struct connection *conn) * thread will be in the process of unloading it....

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.

v1 was here: https://www.redhat.com/archives/libguestfs/2019-September/msg00199.html v2: - Adds a fairly simple yet comprehensive test using sh plugin. - Rebase and retest. Patch 1 is a misc patch not really related to the series. Rich.

...int readonly) - __attribute__((__nonnull__ (1, 2))); -extern int backend_prepare (struct backend *b, struct connection *conn) - __attribute__((__nonnull__ (1, 2))); -extern int backend_finalize (struct backend *b, struct connection *conn) - __attribute__((__nonnull__ (1, 2))); -extern void backend_close (struct backend *b, struct connection *conn) - __attribute__((__nonnull__ (1, 2))); -extern bool backend_valid_range (struct backend *b, struct connection *conn, +extern int backend_open (struct backend *b, int readonly) + __attribute__((__nonnull__ (1))); +extern int backend_prepare (struct back...

...one I just thought of: > +int > +backend_reopen (struct backend *b, struct connection *conn, int readonly) > +{ > + struct b_conn_handle *h = &conn->handles[b->i]; > + > + debug ("%s: reopen", b->name); > + > + if (h->handle != NULL) > + backend_close (b, conn); > + return backend_open (b, conn, readonly); Do we want to grab a resource lock at this point? Should we be trying hard to prevent further client calls from going to the retry filter while the retry filter is reopening the real plugin? Or do those locks belong in the retry filter...

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.

...b) { GET_CONN; - struct b_conn_handle *h = &conn->handles[b->i]; + struct handle *h = get_handle (conn, b->i); /* Call these in reverse order to .prepare above, starting from the * filter furthest away from the plugin, and matching .close order. @@ -238,7 +238,7 @@ void backend_close (struct backend *b) { GET_CONN; - struct b_conn_handle *h = &conn->handles[b->i]; + struct handle *h = get_handle (conn, b->i); /* outer-to-inner order, opposite .open */ controlpath_debug ("%s: close", b->name); @@ -249,7 +249,7 @@ backend_close (struct back...

...l (backend) <= + assert (top->thread_model (top) <= NBDKIT_THREAD_MODEL_SERIALIZE_ALL_REQUESTS); lock_request (NULL); if (pipe (conn->status_pipe)) { @@ -354,7 +354,7 @@ free_connection (struct connection *conn) */ if (!quit) { lock_request (); - backend_close (backend); + backend_close (top); unlock_request (); } diff --git a/server/locks.c b/server/locks.c index f005710d..6211648d 100644 --- a/server/locks.c +++ b/server/locks.c @@ -69,7 +69,7 @@ name_of_thread_model (int model) void lock_init_thread_model (void) { - thread_model = bac...

I'm still working on another set of patches to have reopen call .finalize/.prepare (so that another filter can safely appear between retry and the plugin), but for tonight, these are the patches I think are ready to go. Eric Blake (4): retry: Handle can_fua and can_fast_zero changes tests: Test retry with different fua/fast-zero flags server: Close backends if a filter's .open fails

...read.so.0 #4 0x00007fabc05cc063 in clone () from /lib64/libc.so.6 Thread 2 (Thread 0x7fabbfcf8700 (LWP 3955793)): #0 0x00007fabc069eab7 in __pthread_clockjoin_ex () from /lib64/libpthread.so.0 #1 0x00007fabc090af2b in nbdplug_close_handle (h=0x5584020e09b0) at nbd.c:538 #2 0x00005583f90caee0 in backend_close (b=<optimized out>) at backend.c:247 #3 0x00005583f90cdbf1 in free_connection (conn=0x5584020df890) at connections.c:359 #4 handle_single_connection (sockin=<optimized out>, sockout=<optimized out>) at connections.c:230 #5 0x00005583f90d63e8 in start_thread (datav=0x5584020bf1b...

...234f2fccc8157b7228b546 and commit a6b88b195a959b17524d1c8353fd425d4891dc5f. --- server/backend.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/server/backend.c b/server/backend.c index 64dbf7d..6c102f9 100644 --- a/server/backend.c +++ b/server/backend.c @@ -209,8 +209,7 @@ backend_close (struct backend *b, struct connection *conn) debug ("%s: close", b->name); b->close (b, conn); - memset (h, -1, sizeof *h); - h->handle = NULL; + reset_b_conn_handle (h); } void -- 2.23.0

...tion(-) Makes sense, but .finalize should get the same treatment since it likewise gets skipped when .prepare fails. > > diff --git a/server/backend.c b/server/backend.c > index 51b56a48..108f4a25 100644 > --- a/server/backend.c > +++ b/server/backend.c > @@ -241,10 +241,10 @@ backend_close (struct backend *b) > struct handle *h = get_handle (conn, b->i); > > /* outer-to-inner order, opposite .open */ > - controlpath_debug ("%s: close", b->name); > > if (h->handle) { > assert (h->state & HANDLE_OPEN); > + co...

...@ -233,6 +233,15 @@ backend_valid_range (struct backend *b, struct connection *conn, /* Wrappers for all callbacks in a filter's struct nbdkit_next_ops. */ +int +backend_reopen (struct backend *b, struct connection *conn, int readonly) +{ + debug ("%s: reopen", b->name); + + backend_close (b, conn); + return backend_open (b, conn, readonly); +} + int64_t backend_get_size (struct backend *b, struct connection *conn) { diff --git a/server/filters.c b/server/filters.c index 1091c2d..78e32bc 100644 --- a/server/filters.c +++ b/server/filters.c @@ -237,6 +237,13 @@ filter_close (stru...

...ppers for all callbacks in a filter's struct nbdkit_next_ops. */ +int +backend_reopen (struct backend *b, struct connection *conn, int readonly) +{ + struct b_conn_handle *h = &conn->handles[b->i]; + + debug ("%s: reopen", b->name); + + if (h->handle != NULL) + backend_close (b, conn); + return backend_open (b, conn, readonly); +} + int64_t backend_get_size (struct backend *b, struct connection *conn) { diff --git a/server/filters.c b/server/filters.c index 1091c2d..78e32bc 100644 --- a/server/filters.c +++ b/server/filters.c @@ -237,6 +237,13 @@ filter_close (stru...