9:50 a.m.
The retry filter was originally written to be the closest filter to
the plugin, with no other filters in between; as such, the reopen
command did not have to worry about recursion or about .prepare or
.finalize. But it is not that much harder to properly track
everything needed to allow other filters to be retried, as long as we
are careful to never call .finalize unless .prepare succeeded, never
call .close unless .open succeeded, and never use a connection after
the failure of .finalize (none of our current filters fail that).
Signed-off-by: Eric Blake <eblake(a)redhat.com>
---
docs/nbdkit-filter.pod | 8 ++++++--
server/internal.h | 9 +++++++++
server/backend.c | 45 ++++++++++++++++++++++++++++++++++--------
server/connections.c | 5 +----
4 files changed, 53 insertions(+), 14 deletions(-)
diff --git a/docs/nbdkit-filter.pod b/docs/nbdkit-filter.pod
index 3c98d89a..07ede47a 100644
--- a/docs/nbdkit-filter.pod
+++ b/docs/nbdkit-filter.pod
@@ -358,10 +358,14 @@ methods are called starting with the filter closest to the plugin
and
proceeding outwards (matching the order of C<.open> if all filters
call C<next> before doing anything locally). Finalize methods are
called in the reverse order of prepare methods, with the outermost
-filter first (and matching the order of C<.close>).
+filter first (and matching the order of C<.close>), and only if the
+prepare method succeeded.
If there is an error, both callbacks should call C<nbdkit_error> with
-an error message and return C<-1>.
+an error message and return C<-1>. An error in C<.prepare> is
+reported to the client, but leaves the connection open (a client may
+try again with a different export name, for example); while an error
+in C<.finalize> forces the client to disconnect.
=head2 C<.get_size>
diff --git a/server/internal.h b/server/internal.h
index eb0e30c1..167da59a 100644
--- a/server/internal.h
+++ b/server/internal.h
@@ -153,9 +153,17 @@ typedef int (*connection_send_function) (struct connection *,
typedef void (*connection_close_function) (struct connection *)
__attribute__((__nonnull__ (1)));
+enum {
+ HANDLE_OPEN = 1, /* Set if .open passed, so .close is needed */
+ HANDLE_CONNECTED = 2, /* Set if .prepare passed, so .finalize is needed */
+ HANDLE_FAILED = 4, /* Set if .finalize failed */
+};
+
struct b_conn_handle {
void *handle;
+ unsigned char state; /* Bitmask of HANDLE_* values */
+
uint64_t exportsize;
int can_write;
int can_flush;
@@ -173,6 +181,7 @@ static inline void
reset_b_conn_handle (struct b_conn_handle *h)
{
h->handle = NULL;
+ h->state = 0;
h->exportsize = -1;
h->can_write = -1;
h->can_flush = -1;
diff --git a/server/backend.c b/server/backend.c
index 702c9b96..bdc5bbfd 100644
--- a/server/backend.c
+++ b/server/backend.c
@@ -176,6 +176,7 @@ backend_open (struct backend *b, struct connection *conn, int
readonly)
debug ("%s: open readonly=%d", b->name, readonly);
assert (h->handle == NULL);
+ assert ((h->state & HANDLE_OPEN) == 0);
assert (h->can_write == -1);
if (readonly)
h->can_write = 0;
@@ -192,6 +193,7 @@ backend_open (struct backend *b, struct connection *conn, int
readonly)
return -1;
}
+ h->state |= HANDLE_OPEN;
if (b->i) /* A filter must not succeed unless its backend did also */
assert (conn->handles[b->i - 1].handle);
return 0;
@@ -203,6 +205,7 @@ backend_prepare (struct backend *b, struct connection *conn)
struct b_conn_handle *h = &conn->handles[b->i];
assert (h->handle);
+ assert ((h->state & (HANDLE_OPEN | HANDLE_CONNECTED)) == HANDLE_OPEN);
/* Call these in order starting from the filter closest to the
* plugin, similar to typical .open order.
@@ -212,7 +215,10 @@ backend_prepare (struct backend *b, struct connection *conn)
debug ("%s: prepare readonly=%d", b->name, h->can_write == 0);
- return b->prepare (b, conn, h->handle, h->can_write == 0);
+ if (b->prepare (b, conn, h->handle, h->can_write == 0) == -1)
+ return -1;
+ h->state |= HANDLE_CONNECTED;
+ return 0;
}
int
@@ -224,12 +230,22 @@ backend_finalize (struct backend *b, struct connection *conn)
* filter furthest away from the plugin, and matching .close order.
*/
- assert (h->handle);
debug ("%s: finalize", b->name);
- if (b->finalize (b, conn, h->handle) == -1)
+ /* Once finalize fails, we can do nothing further on this connection */
+ if (h->state & HANDLE_FAILED)
return -1;
+ if (h->handle) {
+ assert (h->state & HANDLE_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);
+ }
+ 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 (b, conn);
+ if (backend_open (b, conn, readonly) == -1) {
backend_close (b, conn);
- return backend_open (b, conn, readonly);
+ return -1;
+ }
+ if (backend_prepare (b, conn) == -1) {
+ backend_finalize (b, conn);
+ backend_close (b, conn);
+ return -1;
+ }
+ return 0;
}
int64_t
diff --git a/server/connections.c b/server/connections.c
index c6fa232f..c55d3816 100644
--- a/server/connections.c
+++ b/server/connections.c
@@ -216,10 +216,7 @@ _handle_single_connection (int sockin, int sockout)
/* Finalize (for filters), called just before close. */
lock_request (conn);
- if (backend)
- r = backend_finalize (backend, conn);
- else
- r = 0;
+ r = backend_finalize (backend, conn);
unlock_request (conn);
if (r == -1)
goto done;
--
2.21.0