search for: backend_reopen

...); diff --git a/server/backend.c b/server/backend.c index 6c102f9..69a07d7 100644 --- a/server/backend.c +++ b/server/backend.c @@ -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/serv...

...); diff --git a/server/backend.c b/server/backend.c index 6c102f9..52b1734 100644 --- a/server/backend.c +++ b/server/backend.c @@ -233,6 +233,18 @@ 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) +{ + 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...

...at 12:34:17PM +0100, Richard W.M. Jones wrote: > diff --git a/server/backend.c b/server/backend.c > index 8a434bd..b8c5742 100644 > --- a/server/backend.c > +++ b/server/backend.c > @@ -224,9 +224,17 @@ backend_valid_range (struct backend *b, struct connection *conn, > 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); > > b->close (b, conn); > + > + /* This forces .open to recalculate h->can_write, w...

...+++ > server/backend.c | 12 ++++++++++++ > server/filters.c | 8 ++++++++ > server/internal.h | 4 ++++ > 4 files changed, 30 insertions(+) > I made comments on v2 that still apply here (crossed emails again). Another 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,...

...our version match check. No further header version bumps needed :) > +++ b/server/backend.c > @@ -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); 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...

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.

On 2/12/20 7:40 AM, Richard W.M. Jones wrote: > This was previously used as ‘nxdata’ and stored a tuple of ’b->next’ > and the real filter handle. However after recent changes we don't > need it. We can use ‘b->next’ as nxdata, and the handle is passed to > us by the calling functions. > > Inspired by Eric Blakes observations in this email: Blake's >

...eed to save it in the handle > >+ * because of the lifetime issue. > >+ */ > >+ if (conn->exportname == NULL) { > > Can't we assert(!conn->exportname) at this point? After all, we > only ever call .open at most once per connection. I don't think so - backend_reopen will call plugin_open a second time. As a test I added assert (conn->exportname == NULL) before this line and it crashed in tests/test-retry.sh. > >+++ b/server/protocol-handshake-newstyle.c > >@@ -200,11 +200,29 @@ conn_recv_full (void *buf, size_t len, const char *fmt, ...) > &...

This is a retry filter implementation as outlined here: https://www.redhat.com/archives/libguestfs/2019-September/msg00167.html It is only lightly tested. One way to test it is to try an SSH copy (see the commit message for patch 2/2), and in the middle of the copy kill the per-connection sshd on the remote machine. You will see that the copy recovers after a few seconds. Add the nbdkit -v

...would be ABI compatible, and although it would require > recompilation, we already state that filter recompilation is par for > the course (since only plugins promise API compatibility). Yes my original version had stuff like: static struct nbdkit_next_ops next_ops = { .reopen = (void *) backend_reopen, .get_size = (void *) backend_get_size, but that wasn't very safe, and exporting struct backend, even opaquely, to the public header didn't sound like a good idea either. (Are C structs always treated the same by name? That could cause a problem for a filter which used "struct back...

...are (struct backend *b) __attribute__((__nonnull__ (1))); extern int backend_finalize (struct backend *b) @@ -414,8 +421,9 @@ extern bool backend_valid_range (struct backend *b, uint64_t offset, uint32_t count) __attribute__((__nonnull__ (1))); -extern int backend_reopen (struct backend *b, int readonly) - __attribute__((__nonnull__ (1))); +extern int backend_reopen (struct backend *b, + int readonly, const char *exportname) + __attribute__((__nonnull__ (1, 3))); extern int64_t backend_get_size (struct backend *b) __attribute__((__no...

...These are only needed for type safety (nxdata is void - * pointer, backend_* functions expect a struct backend * parameter). - * nxdata is a pointer to the next backend in the linked list. - */ - -static int -next_reopen (void *nxdata, int readonly) -{ - struct backend *b_next = nxdata; - return backend_reopen (b_next, readonly); -} - -static int64_t -next_get_size (void *nxdata) -{ - struct backend *b_next = nxdata; - return backend_get_size (b_next); -} - -static int -next_can_write (void *nxdata) -{ - struct backend *b_next = nxdata; - return backend_can_write (b_next); -} - -static int -next_can_...

...tions. These are only needed for type safety (nxdata is void + * pointer, backend_* functions expect a struct backend * parameter). + * nxdata is a pointer to the next backend in the linked list. */ static int next_reopen (void *nxdata, int readonly) { - struct b_h *b_h = nxdata; - return backend_reopen (b_h->b, readonly); + struct backend *b_next = nxdata; + return backend_reopen (b_next, readonly); } static int64_t next_get_size (void *nxdata) { - struct b_h *b_h = nxdata; - return backend_get_size (b_h->b); + struct backend *b_next = nxdata; + return backend_get_size (b_next);...

v2 was here: https://www.redhat.com/archives/libguestfs/2019-September/msg00221.html I think this is more like "the one". It handles reopen failing correctly, and there is a second test for that. I also ran my sshd tests locally and it worked in all scenarios I could think up (except of course sshd not being available at the start, but we want that to fail). Rich.

...& HANDLE_OPEN); + b->close (b, conn, h->handle); + } + else + assert (! (h->state & HANDLE_OPEN)); reset_b_conn_handle (h); if (b->i) backend_close (b->next, conn); @@ -273,13 +294,21 @@ backend_valid_range (struct backend *b, struct connection *conn, int backend_reopen (struct backend *b, struct connection *conn, int readonly) { - struct b_conn_handle *h = &conn->handles[b->i]; - debug ("%s: reopen readonly=%d", b->name, readonly); - if (h->handle != NULL) + if (backend_finalize (b, conn) == -1) + return -1; + backend_close (...

...his call and almost certainly will be * freed on return of this function, so backends must save the * exportname if they need to refer to it later. diff --git a/server/backend.c b/server/backend.c index 427a0cec..3630163b 100644 --- a/server/backend.c +++ b/server/backend.c @@ -360,6 +360,28 @@ backend_reopen (struct backend *b, int readonly, const char *exportname) return 0; } +const char * +backend_export_description (struct backend *b) +{ + GET_CONN; + struct handle *h = get_handle (conn, b->i); + const char *s; + + controlpath_debug ("%s: export_description", b->name); + +...

...))); +extern void backend_close (struct backend *b) + __attribute__((__nonnull__ (1))); +extern bool backend_valid_range (struct backend *b, uint64_t offset, uint32_t count) - __attribute__((__nonnull__ (1, 2))); + __attribute__((__nonnull__ (1))); -extern int backend_reopen (struct backend *b, struct connection *conn, - int readonly) - __attribute__((__nonnull__ (1, 2))); -extern int64_t backend_get_size (struct backend *b, struct connection *conn) - __attribute__((__nonnull__ (1, 2))); -extern int backend_can_write (struct backend *b, stru...

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 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