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 backend.c server: Ensure .finalize and .close are called as needed docs/nbdkit-filter.pod | 8 ++- server/internal.h | 14 +++-- server/backend.c | 105 ++++++++++++++++++++++++++------ server/connections.c | 5 +- server/filters.c | 35 +++-------- server/plugins.c | 10 +-- filters/retry/retry.c | 26 +++++--- tests/test-retry-reopen-fail.sh | 105 ++++++++++++++++++++++++-------- 8 files changed, 211 insertions(+), 97 deletions(-) -- 2.21.0
Eric Blake
2019-Oct-07 14:50 UTC
[Libguestfs] [nbdkit PATCH 1/5] retry: Don't call into closed plugin
If reopen fails, calling into the plugin would end up passing NULL as the handle. For the sh plugin, this merely results in a truncated command line, but for other plugins it could result in a crash. The fix is to have the retry filter track whether reopen failed, and short-circuit any new transaction to begin with a retry when the previous transaction ended in that state. Add asserts to backend to ensure that the plugin's handle is non-NULL at all points of use, now that the retry filter keeps that contract. Fixes: f0f0ec49 Signed-off-by: Eric Blake <eblake@redhat.com> --- server/backend.c | 18 ++++++++++++++++++ filters/retry/retry.c | 26 +++++++++++++++++--------- 2 files changed, 35 insertions(+), 9 deletions(-) diff --git a/server/backend.c b/server/backend.c index ace61c29..641af5ff 100644 --- a/server/backend.c +++ b/server/backend.c @@ -265,6 +265,7 @@ backend_get_size (struct backend *b, struct connection *conn) debug ("%s: get_size", b->name); + assert (h->handle); if (h->exportsize == -1) h->exportsize = b->get_size (b, conn, h->handle); return h->exportsize; @@ -277,6 +278,7 @@ backend_can_write (struct backend *b, struct connection *conn) debug ("%s: can_write", b->name); + assert (h->handle); if (h->can_write == -1) h->can_write = b->can_write (b, conn, h->handle); return h->can_write; @@ -289,6 +291,7 @@ backend_can_flush (struct backend *b, struct connection *conn) debug ("%s: can_flush", b->name); + assert (h->handle); if (h->can_flush == -1) h->can_flush = b->can_flush (b, conn, h->handle); return h->can_flush; @@ -301,6 +304,7 @@ backend_is_rotational (struct backend *b, struct connection *conn) debug ("%s: is_rotational", b->name); + assert (h->handle); if (h->is_rotational == -1) h->is_rotational = b->is_rotational (b, conn, h->handle); return h->is_rotational; @@ -314,6 +318,7 @@ backend_can_trim (struct backend *b, struct connection *conn) debug ("%s: can_trim", b->name); + assert (h->handle); if (h->can_trim == -1) { r = backend_can_write (b, conn); if (r != 1) { @@ -333,6 +338,7 @@ backend_can_zero (struct backend *b, struct connection *conn) debug ("%s: can_zero", b->name); + assert (h->handle); if (h->can_zero == -1) { r = backend_can_write (b, conn); if (r != 1) { @@ -352,6 +358,7 @@ backend_can_fast_zero (struct backend *b, struct connection *conn) debug ("%s: can_fast_zero", b->name); + assert (h->handle); if (h->can_fast_zero == -1) { r = backend_can_zero (b, conn); if (r < NBDKIT_ZERO_EMULATE) { @@ -370,6 +377,7 @@ backend_can_extents (struct backend *b, struct connection *conn) debug ("%s: can_extents", b->name); + assert (h->handle); if (h->can_extents == -1) h->can_extents = b->can_extents (b, conn, h->handle); return h->can_extents; @@ -383,6 +391,7 @@ backend_can_fua (struct backend *b, struct connection *conn) debug ("%s: can_fua", b->name); + assert (h->handle); if (h->can_fua == -1) { r = backend_can_write (b, conn); if (r != 1) { @@ -399,6 +408,7 @@ backend_can_multi_conn (struct backend *b, struct connection *conn) { struct b_conn_handle *h = &conn->handles[b->i]; + assert (h->handle); debug ("%s: can_multi_conn", b->name); if (h->can_multi_conn == -1) @@ -413,6 +423,7 @@ backend_can_cache (struct backend *b, struct connection *conn) debug ("%s: can_cache", b->name); + assert (h->handle); if (h->can_cache == -1) h->can_cache = b->can_cache (b, conn, h->handle); return h->can_cache; @@ -426,6 +437,7 @@ backend_pread (struct backend *b, struct connection *conn, struct b_conn_handle *h = &conn->handles[b->i]; int r; + assert (h->handle); assert (backend_valid_range (b, conn, offset, count)); assert (flags == 0); debug ("%s: pread count=%" PRIu32 " offset=%" PRIu64, @@ -446,6 +458,7 @@ backend_pwrite (struct backend *b, struct connection *conn, bool fua = !!(flags & NBDKIT_FLAG_FUA); int r; + assert (h->handle); assert (h->can_write == 1); assert (backend_valid_range (b, conn, offset, count)); assert (!(flags & ~NBDKIT_FLAG_FUA)); @@ -467,6 +480,7 @@ backend_flush (struct backend *b, struct connection *conn, struct b_conn_handle *h = &conn->handles[b->i]; int r; + assert (h->handle); assert (h->can_flush == 1); assert (flags == 0); debug ("%s: flush", b->name); @@ -486,6 +500,7 @@ backend_trim (struct backend *b, struct connection *conn, bool fua = !!(flags & NBDKIT_FLAG_FUA); int r; + assert (h->handle); assert (h->can_write == 1); assert (h->can_trim == 1); assert (backend_valid_range (b, conn, offset, count)); @@ -511,6 +526,7 @@ backend_zero (struct backend *b, struct connection *conn, bool fast = !!(flags & NBDKIT_FLAG_FAST_ZERO); int r; + assert (h->handle); assert (h->can_write == 1); assert (h->can_zero > NBDKIT_ZERO_NONE); assert (backend_valid_range (b, conn, offset, count)); @@ -541,6 +557,7 @@ backend_extents (struct backend *b, struct connection *conn, struct b_conn_handle *h = &conn->handles[b->i]; int r; + assert (h->handle); assert (h->can_extents >= 0); assert (backend_valid_range (b, conn, offset, count)); assert (!(flags & ~NBDKIT_FLAG_REQ_ONE)); @@ -570,6 +587,7 @@ backend_cache (struct backend *b, struct connection *conn, struct b_conn_handle *h = &conn->handles[b->i]; int r; + assert (h->handle); assert (h->can_cache > NBDKIT_CACHE_NONE); assert (backend_valid_range (b, conn, offset, count)); assert (flags == 0); diff --git a/filters/retry/retry.c b/filters/retry/retry.c index 0f249936..11e5313b 100644 --- a/filters/retry/retry.c +++ b/filters/retry/retry.c @@ -107,6 +107,7 @@ retry_config (nbdkit_next_config *next, void *nxdata, struct retry_handle { int readonly; /* Save original readonly setting. */ unsigned reopens; + bool open; }; static void * @@ -125,6 +126,7 @@ retry_open (nbdkit_next_open *next, void *nxdata, int readonly) h->readonly = readonly; h->reopens = 0; + h->open = true; return h; } @@ -183,7 +185,8 @@ do_retry (struct retry_handle *h, /* We could do this but it would overwrite the more important * errno from the underlying data call. */ - /* *err = errno; */ + if (*err == 0) + *err = errno; return false; } @@ -198,8 +201,11 @@ do_retry (struct retry_handle *h, /* If the reopen fails we treat it the same way as a command * failing. */ + h->open = false; + *err = ESHUTDOWN; goto again; } + h->open = true; /* Retry the data command. */ return true; @@ -215,7 +221,7 @@ retry_pread (struct nbdkit_next_ops *next_ops, void *nxdata, int r; again: - if (! valid_range (next_ops, nxdata, count, offset, false, err)) + if (! (h->open && valid_range (next_ops, nxdata, count, offset, false, err))) r = -1; else r = next_ops->pread (nxdata, buf, count, offset, flags, err); @@ -240,7 +246,7 @@ retry_pwrite (struct nbdkit_next_ops *next_ops, void *nxdata, *err = EROFS; return -1; } - if (! valid_range (next_ops, nxdata, count, offset, true, err)) + if (! (h->open && valid_range (next_ops, nxdata, count, offset, true, err))) r = -1; else if (next_ops->can_write (nxdata) != 1) { *err = EROFS; @@ -274,7 +280,7 @@ retry_trim (struct nbdkit_next_ops *next_ops, void *nxdata, *err = EROFS; return -1; } - if (! valid_range (next_ops, nxdata, count, offset, true, err)) + if (! (h->open && valid_range (next_ops, nxdata, count, offset, true, err))) r = -1; else if (next_ops->can_trim (nxdata) != 1) { *err = EROFS; @@ -303,7 +309,9 @@ retry_flush (struct nbdkit_next_ops *next_ops, void *nxdata, int r; again: - if (next_ops->can_flush (nxdata) != 1) { + if (! h->open) + r = -1; + else if (next_ops->can_flush (nxdata) != 1) { *err = EIO; r = -1; } @@ -331,11 +339,11 @@ retry_zero (struct nbdkit_next_ops *next_ops, void *nxdata, return -1; } if (flags & NBDKIT_FLAG_FAST_ZERO && - next_ops->can_fast_zero (nxdata) != 1) { + (! h->open || next_ops->can_fast_zero (nxdata) != 1)) { *err = EOPNOTSUPP; return -1; } - if (! valid_range (next_ops, nxdata, count, offset, true, err)) + if (! (h->open && valid_range (next_ops, nxdata, count, offset, true, err))) r = -1; else if (next_ops->can_zero (nxdata) <= NBDKIT_ZERO_NONE) { *err = EROFS; @@ -367,7 +375,7 @@ retry_extents (struct nbdkit_next_ops *next_ops, void *nxdata, size_t i; again: - if (! valid_range (next_ops, nxdata, count, offset, false, err)) + if (! (h->open && valid_range (next_ops, nxdata, count, offset, false, err))) r = -1; else if (next_ops->can_extents (nxdata) != 1) { *err = EIO; @@ -412,7 +420,7 @@ retry_cache (struct nbdkit_next_ops *next_ops, void *nxdata, int r; again: - if (! valid_range (next_ops, nxdata, count, offset, false, err)) + if (! h->open && (valid_range (next_ops, nxdata, count, offset, false, err))) r = -1; else if (next_ops->can_cache (nxdata) <= NBDKIT_CACHE_NONE) { *err = EIO; -- 2.21.0
Eric Blake
2019-Oct-07 14:50 UTC
[Libguestfs] [nbdkit PATCH 2/5] tests: Refactor test-retry-reopen-fail.sh
Place much of the test in a shell function to make it easier to test multiple reopen failure scenarios. Use qemu-io over qemu-img convert, for more precise control over how many reads are attempted. Signed-off-by: Eric Blake <eblake@redhat.com> --- tests/test-retry-reopen-fail.sh | 64 ++++++++++++++++++++------------- 1 file changed, 40 insertions(+), 24 deletions(-) diff --git a/tests/test-retry-reopen-fail.sh b/tests/test-retry-reopen-fail.sh index c3c2a67b..1aec0509 100755 --- a/tests/test-retry-reopen-fail.sh +++ b/tests/test-retry-reopen-fail.sh @@ -37,21 +37,30 @@ source ./functions.sh set -e set -x -requires qemu-img --version +fail=0 -files="retry-reopen-fail.img - retry-reopen-fail-count retry-reopen-fail-open-count" +requires qemu-io --version + +files="retry-reopen-fail-count retry-reopen-fail-open-count" rm -f $files cleanup_fn rm -f $files -touch retry-reopen-fail-count retry-reopen-fail-open-count -start_t=$SECONDS +# do_test retries mintime expcount +do_test () +{ + retries=$1 + mintime=$2 + expcount=$3 -# Create a custom plugin which will test retrying. -nbdkit -v -U - \ - sh - \ - --filter=retry retry-delay=1 \ - --run 'qemu-img convert $nbd retry-reopen-fail.img' <<'EOF' + echo 0 > retry-reopen-fail-count + echo 0 > retry-reopen-fail-open-count + start_t=$SECONDS + + # Create a custom plugin which will test retrying. + nbdkit -v -U - \ + sh - \ + --filter=retry retry-delay=1 retries=$retries \ + --run 'qemu-io -r -f raw $nbd -c "r 0 512" -c "r 0 512"' <<'EOF' #!/usr/bin/env bash case "$1" in open) @@ -82,27 +91,34 @@ case "$1" in esac EOF + # Check that running time appears reasonable. + end_t=$SECONDS + if [ $((end_t - start_t)) -lt $mintime ]; then + echo "$0: test ran too quickly" + fail=1 + fi + + # Check the handle was opened as often as expected. + read open_count < retry-reopen-fail-open-count + if [ $open_count -ne $expcount ]; then + echo "$0: open-count ($open_count) != $expcount" + fail=1 + fi +} + # In this test we should see 3 failures: -# pread FAILS +# first pread FAILS # retry and wait 1 seconds # open FAILS # retry and wait 2 seconds # open succeeds -# pread FAILS +# first pread FAILS # retry and wait 4 seconds # open succeeds -# pread succeeds +# first pread succeeds +# second pread succeeds # The minimum time for the test should be 1+2+4 = 7 seconds. -end_t=$SECONDS -if [ $((end_t - start_t)) -lt 7 ]; then - echo "$0: test ran too quickly" - exit 1 -fi +do_test 5 7 4 -# Check the handle was opened 4 times. -read open_count < retry-reopen-fail-open-count -if [ $open_count -ne 4 ]; then - echo "$0: open-count ($open_count) != 4" - exit 1 -fi +exit $fail -- 2.21.0
Eric Blake
2019-Oct-07 14:50 UTC
[Libguestfs] [nbdkit PATCH 3/5] tests: Enhance retry test to cover failed reopen
Demonstrate that the previous patch fixed an issue with calling into the plugin with a NULL handle after reopen failed. Signed-off-by: Eric Blake <eblake@redhat.com> --- tests/test-retry-reopen-fail.sh | 49 +++++++++++++++++++++++++++++---- 1 file changed, 43 insertions(+), 6 deletions(-) diff --git a/tests/test-retry-reopen-fail.sh b/tests/test-retry-reopen-fail.sh index 1aec0509..e9ec6e42 100755 --- a/tests/test-retry-reopen-fail.sh +++ b/tests/test-retry-reopen-fail.sh @@ -41,27 +41,38 @@ fail=0 requires qemu-io --version -files="retry-reopen-fail-count retry-reopen-fail-open-count" +files="retry-reopen-fail-count retry-reopen-fail-open-count + retry-reopen-fail-status" rm -f $files cleanup_fn rm -f $files -# do_test retries mintime expcount +# do_test retries mintime expcount status do_test () { retries=$1 mintime=$2 expcount=$3 + status=$4 echo 0 > retry-reopen-fail-count echo 0 > retry-reopen-fail-open-count + : > retry-reopen-fail-status start_t=$SECONDS # Create a custom plugin which will test retrying. nbdkit -v -U - \ sh - \ --filter=retry retry-delay=1 retries=$retries \ - --run 'qemu-io -r -f raw $nbd -c "r 0 512" -c "r 0 512"' <<'EOF' + --run 'qemu-io -r -f raw $nbd -c "r 0 512" -c "r 0 512" + echo $? >> retry-reopen-fail-status' <<'EOF' #!/usr/bin/env bash +handle=$2 +check_handle () { + if [ x"$handle" != xhandle ]; then + echo 22 >> retry-reopen-fail-status + exit 22 + fi +} case "$1" in open) # Count how many times the connection is (re-)opened. @@ -72,8 +83,10 @@ case "$1" in echo "EIO open failed" >&2 exit 1 fi + echo "handle" ;; pread) + check_handle # Fail 2 times then succeed. read i < retry-reopen-fail-count ((i++)) @@ -86,7 +99,9 @@ case "$1" in fi ;; - get_size) echo 512 ;; + get_size) + check_handle + echo 512 ;; *) exit 2 ;; esac EOF @@ -104,9 +119,16 @@ EOF echo "$0: open-count ($open_count) != $expcount" fail=1 fi + + # Check the exit status of qemu-io + read qemu_status < retry-reopen-fail-status + if [ $qemu_status -ne $status ]; then + echo "$0: status ($qemu_status) != $status" + fail=1 + fi } -# In this test we should see 3 failures: +# In this first test we should see 3 failures: # first pread FAILS # retry and wait 1 seconds # open FAILS @@ -119,6 +141,21 @@ EOF # second pread succeeds # The minimum time for the test should be 1+2+4 = 7 seconds. -do_test 5 7 4 +do_test 5 7 4 0 + +# In this second test we should see the following: +# first pread FAILS +# retry and wait 1 seconds +# open FAILS, ending first pread in failure +# first pread FAILS +# second pread requires open +# open succeeds +# second pread FAILS +# retry and wait 1 seconds +# open succeeds +# second pread succeeds + +# The minimum time for the test should be 1+1 = 2 seconds. +do_test 1 2 3 1 exit $fail -- 2.21.0
Eric Blake
2019-Oct-07 14:50 UTC
[Libguestfs] [nbdkit PATCH 4/5] server: Move prepare/finalize/close recursion to backend.c
Drop backend_set_handle, and instead use the return value of open to control that. While open recursion still has to go through filters (because a filter may change the readonly parameter), the recursion for prepare/finalize/close fits better in backend.c. This will also help an upcoming patch better handle prepare/finalize during reopen. Signed-off-by: Eric Blake <eblake@redhat.com> --- server/internal.h | 5 +---- server/backend.c | 46 +++++++++++++++++++++++++++++++++++----------- server/filters.c | 35 ++++++++--------------------------- server/plugins.c | 10 ++-------- 4 files changed, 46 insertions(+), 50 deletions(-) diff --git a/server/internal.h b/server/internal.h index d4b57419..eb0e30c1 100644 --- a/server/internal.h +++ b/server/internal.h @@ -303,7 +303,7 @@ struct backend { void (*config) (struct backend *, const char *key, const char *value); void (*config_complete) (struct backend *); const char *(*magic_config_key) (struct backend *); - int (*open) (struct backend *, struct connection *conn, int readonly); + void *(*open) (struct backend *, struct connection *conn, int readonly); int (*prepare) (struct backend *, struct connection *conn, void *handle, int readonly); int (*finalize) (struct backend *, struct connection *conn, void *handle); @@ -361,9 +361,6 @@ 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 void backend_set_handle (struct backend *b, struct connection *conn, - void *handle) - __attribute__((__nonnull__ (1, 2, 3))); extern bool backend_valid_range (struct backend *b, struct connection *conn, uint64_t offset, uint32_t count) __attribute__((__nonnull__ (1, 2))); diff --git a/server/backend.c b/server/backend.c index 641af5ff..702c9b96 100644 --- a/server/backend.c +++ b/server/backend.c @@ -172,7 +172,6 @@ int backend_open (struct backend *b, struct connection *conn, int readonly) { struct b_conn_handle *h = &conn->handles[b->i]; - int r; debug ("%s: open readonly=%d", b->name, readonly); @@ -180,18 +179,22 @@ backend_open (struct backend *b, struct connection *conn, int readonly) assert (h->can_write == -1); if (readonly) h->can_write = 0; - r = b->open (b, conn, readonly); - if (r == 0) { - assert (h->handle != NULL); - if (b->i) /* A filter must not succeed unless its backend did also */ - assert (conn->handles[b->i - 1].handle); - } - else { - assert (h->handle == NULL); + + /* Most filters will call next_open first, resulting in + * inner-to-outer ordering. + */ + h->handle = b->open (b, conn, readonly); + debug ("%s: open returned handle %p", b->name, h->handle); + + if (h->handle == NULL) { if (b->i) /* Do not strand backend if this layer failed */ backend_close (b->next, conn); + return -1; } - return r; + + if (b->i) /* A filter must not succeed unless its backend did also */ + assert (conn->handles[b->i - 1].handle); + return 0; } int @@ -199,6 +202,14 @@ backend_prepare (struct backend *b, struct connection *conn) { struct b_conn_handle *h = &conn->handles[b->i]; + assert (h->handle); + + /* Call these in order starting from the filter closest to the + * plugin, similar to typical .open order. + */ + if (b->i && backend_prepare (b->next, conn) == -1) + return -1; + debug ("%s: prepare readonly=%d", b->name, h->can_write == 0); return b->prepare (b, conn, h->handle, h->can_write == 0); @@ -209,9 +220,19 @@ backend_finalize (struct backend *b, struct connection *conn) { struct b_conn_handle *h = &conn->handles[b->i]; + /* Call these in reverse order to .prepare above, starting from the + * filter furthest away from the plugin, and matching .close order. + */ + + assert (h->handle); debug ("%s: finalize", b->name); - return b->finalize (b, conn, h->handle); + if (b->finalize (b, conn, h->handle) == -1) + return -1; + + if (b->i) + return backend_finalize (b->next, conn); + return 0; } void @@ -219,10 +240,13 @@ backend_close (struct backend *b, struct connection *conn) { struct b_conn_handle *h = &conn->handles[b->i]; + /* outer-to-inner order, opposite .open */ debug ("%s: close", b->name); b->close (b, conn, h->handle); reset_b_conn_handle (h); + if (b->i) + backend_close (b->next, conn); } void diff --git a/server/filters.c b/server/filters.c index 0635f0bb..1ac4a5f4 100644 --- a/server/filters.c +++ b/server/filters.c @@ -198,28 +198,21 @@ next_open (void *nxdata, int readonly) return backend_open (b_conn->b, b_conn->conn, readonly); } -static int +static void * filter_open (struct backend *b, struct connection *conn, int readonly) { struct backend_filter *f = container_of (b, struct backend_filter, backend); struct b_conn nxdata = { .b = b->next, .conn = conn }; - void *handle; /* Most filters will call next_open first, resulting in * inner-to-outer ordering. */ - if (f->filter.open) { - handle = f->filter.open (next_open, &nxdata, readonly); - if (handle == NULL) - return -1; - backend_set_handle (b, conn, handle); - } - else { - if (backend_open (b->next, conn, readonly) == -1) - return -1; - backend_set_handle (b, conn, NBDKIT_HANDLE_NOT_NEEDED); - } - return 0; + if (f->filter.open) + return f->filter.open (next_open, &nxdata, readonly); + else if (backend_open (b->next, conn, readonly) == -1) + return NULL; + else + return NBDKIT_HANDLE_NOT_NEEDED; } static void @@ -227,10 +220,8 @@ filter_close (struct backend *b, struct connection *conn, void *handle) { struct backend_filter *f = container_of (b, struct backend_filter, backend); - /* outer-to-inner order, opposite .open */ if (handle && f->filter.close) f->filter.close (handle); - backend_close (b->next, conn); } /* The next_functions structure contains pointers to backend @@ -411,12 +402,6 @@ filter_prepare (struct backend *b, struct connection *conn, void *handle, struct backend_filter *f = container_of (b, struct backend_filter, backend); struct b_conn nxdata = { .b = b->next, .conn = conn }; - /* Call these in order starting from the filter closest to the - * plugin, similar to typical .open order. - */ - if (backend_prepare (b->next, conn) == -1) - return -1; - if (f->filter.prepare && f->filter.prepare (&next_ops, &nxdata, handle, readonly) == -1) return -1; @@ -430,14 +415,10 @@ filter_finalize (struct backend *b, struct connection *conn, void *handle) struct backend_filter *f = container_of (b, struct backend_filter, backend); struct b_conn nxdata = { .b = b->next, .conn = conn }; - /* Call these in reverse order to .prepare above, starting from the - * filter furthest away from the plugin, and matching .close order. - */ if (f->filter.finalize && f->filter.finalize (&next_ops, &nxdata, handle) == -1) return -1; - - return backend_finalize (b->next, conn); + return 0; } static int64_t diff --git a/server/plugins.c b/server/plugins.c index f309ac22..87daaf26 100644 --- a/server/plugins.c +++ b/server/plugins.c @@ -233,20 +233,14 @@ plugin_magic_config_key (struct backend *b) return p->plugin.magic_config_key; } -static int +static void * plugin_open (struct backend *b, struct connection *conn, int readonly) { struct backend_plugin *p = container_of (b, struct backend_plugin, backend); - void *handle; assert (p->plugin.open != NULL); - handle = p->plugin.open (readonly); - if (!handle) - return -1; - - backend_set_handle (b, conn, handle); - return 0; + return p->plugin.open (readonly); } /* We don't expose .prepare and .finalize to plugins since they aren't -- 2.21.0
Eric Blake
2019-Oct-07 14:50 UTC
[Libguestfs] [nbdkit PATCH 5/5] server: Ensure .finalize and .close are called as needed
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@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
Richard W.M. Jones
2019-Oct-07 21:12 UTC
Re: [Libguestfs] [nbdkit PATCH 5/5] server: Ensure .finalize and .close are called as needed
Somewhat more complicated that I could have imagined when I added the filter. Thanks for fixing it! ACK series Rich. -- Richard Jones, Virtualization Group, Red Hat people.redhat.com/~rjones Read my programming and virtualization blog: rwmj.wordpress.com Fedora Windows cross-compiler. Compile Windows programs, test, and build Windows installers. Over 100 libraries supported. fedoraproject.org/wiki/MinGW
Apparently Analagous Threads
- [PATCH nbdkit v2 0/3] server: Remove explicit connection parameter.
- [PATCH nbdkit 0/3] server: Remove explicit connection parameter.
- [nbdkit PATCH 0/5] Another round of retry fixes
- [PATCH nbdkit 1/3] server: Rename global backend pointer to "top".
- [PATCH nbdkit v2 0/4] Add new retry filter.