Now that a filter can open a backend as many times as it wants, there's no longer a technical reason we can't retry .open. However, adding retry logic here does mean we have to weaken an assert in the server backend code, since prepare can now be reached more than once. Test coverage will be added in a separate patch, so that it becomes easy to swap patch order and see that the test fails without this patch. See also https://bugzilla.redhat.com/show_bug.cgi?id=1841820 --- server/backend.c | 7 +++- filters/retry/retry.c | 98 +++++++++++++++++++++++++------------------ 2 files changed, 63 insertions(+), 42 deletions(-) diff --git a/server/backend.c b/server/backend.c index 3bbba601..45889ea9 100644 --- a/server/backend.c +++ b/server/backend.c @@ -1,5 +1,5 @@ /* nbdkit - * Copyright (C) 2013-2021 Red Hat Inc. + * Copyright (C) 2013-2023 Red Hat Inc. * * Redistribution and use in source and binary forms, with or without * modification, are permitted provided that the following conditions are @@ -312,7 +312,10 @@ backend_prepare (struct context *c) struct backend *b = c->b; assert (c->handle); - assert ((c->state & (HANDLE_OPEN | HANDLE_CONNECTED)) == HANDLE_OPEN); + assert (c->state & HANDLE_OPEN); + + if (c->state & HANDLE_CONNECTED) + return 0; /* Call these in order starting from the filter closest to the * plugin, similar to typical .open order. But remember that diff --git a/filters/retry/retry.c b/filters/retry/retry.c index fb1d0526..7abf74c4 100644 --- a/filters/retry/retry.c +++ b/filters/retry/retry.c @@ -1,5 +1,5 @@ /* nbdkit - * Copyright (C) 2019-2021 Red Hat Inc. + * Copyright (C) 2019-2023 Red Hat Inc. * * Redistribution and use in source and binary forms, with or without * modification, are permitted provided that the following conditions are @@ -113,45 +113,6 @@ struct retry_handle { bool open; }; -static void * -retry_open (nbdkit_next_open *next, nbdkit_context *nxdata, - int readonly, const char *exportname, int is_tls) -{ - struct retry_handle *h; - - if (next (nxdata, readonly, exportname) == -1) - return NULL; - - h = malloc (sizeof *h); - if (h == NULL) { - nbdkit_error ("malloc: %m"); - return NULL; - } - - h->readonly = readonly; - h->exportname = strdup (exportname); - h->context = nxdata; - if (h->exportname == NULL) { - nbdkit_error ("strdup: %m"); - free (h); - return NULL; - } - h->reopens = 0; - h->open = true; - - return h; -} - -static void -retry_close (void *handle) -{ - struct retry_handle *h = handle; - - nbdkit_debug ("reopens needed: %u", h->reopens); - free (h->exportname); - free (h); -} - /* This function encapsulates the common retry logic used across all * data commands. If it returns true then the data command will retry * the operation. ‘struct retry_data’ is stack data saved between @@ -247,6 +208,63 @@ do_retry (struct retry_handle *h, struct retry_data *data, return true; } +static void * +retry_open (nbdkit_next_open *next, nbdkit_context *nxdata, + int readonly, const char *exportname, int is_tls) +{ + struct retry_handle *h; + struct retry_data data = {0}; + + h = malloc (sizeof *h); + if (h == NULL) { + nbdkit_error ("malloc: %m"); + return NULL; + } + + h->readonly = readonly; + h->exportname = strdup (exportname); + h->context = nxdata; + if (h->exportname == NULL) { + nbdkit_error ("strdup: %m"); + free (h); + return NULL; + } + h->reopens = 0; + + if (next (nxdata, readonly, exportname) != -1) + h->open = true; + else { + /* Careful - our .open must not return a handle unless do_retry() + * works, as the caller's next action will be calling .get_size + * and similar probe functions which we do not bother to wire up + * into retry logic because they only need to be used right after + * connecting. + */ + nbdkit_next *next_handle = NULL; + int err = ESHUTDOWN; + + while (! h->open && do_retry (h, &data, &next_handle, "open", &err)) + ; + + if (! h->open) { + free (h->exportname); + free (h); + return NULL; + } + } + return h; +} + +static void +retry_close (void *handle) +{ + struct retry_handle *h = handle; + + nbdkit_debug ("reopens needed: %u", h->reopens); + free (h->exportname); + free (h); +} + static int retry_pread (nbdkit_next *next, void *handle, void *buf, uint32_t count, uint64_t offset, -- 2.39.1

Separate commit to allow rearranging patches to demonstrate that open can now be retried. --- tests/Makefile.am | 2 + tests/test-retry-open.sh | 85 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 87 insertions(+) create mode 100755 tests/test-retry-open.sh diff --git a/tests/Makefile.am b/tests/Makefile.am index e5396b5f..713ccca7 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -1807,6 +1807,7 @@ TESTS += \ test-retry-size.sh \ test-retry-reopen-fail.sh \ test-retry-zero-flags.sh \ + test-retry-open.sh \ $(NULL) EXTRA_DIST += \ test-retry.sh \ @@ -1815,6 +1816,7 @@ EXTRA_DIST += \ test-retry-size.sh \ test-retry-reopen-fail.sh \ test-retry-zero-flags.sh \ + test-retry-open.sh \ $(NULL) # retry-request filter test. diff --git a/tests/test-retry-open.sh b/tests/test-retry-open.sh new file mode 100755 index 00000000..468a03e1 --- /dev/null +++ b/tests/test-retry-open.sh @@ -0,0 +1,85 @@ +#!/usr/bin/env bash +# nbdkit +# Copyright (C) 2018-2023 Red Hat Inc. +# +# Redistribution and use in source and binary forms, with or without +# modification, are permitted provided that the following conditions are +# met: +# +# * Redistributions of source code must retain the above copyright +# notice, this list of conditions and the following disclaimer. +# +# * Redistributions in binary form must reproduce the above copyright +# notice, this list of conditions and the following disclaimer in the +# documentation and/or other materials provided with the distribution. +# +# * Neither the name of Red Hat nor the names of its contributors may be +# used to endorse or promote products derived from this software without +# specific prior written permission. +# +# THIS SOFTWARE IS PROVIDED BY RED HAT AND CONTRIBUTORS ''AS IS'' AND +# ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, +# THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A +# PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL RED HAT OR +# CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, +# SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT +# LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF +# USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND +# ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, +# OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT +# OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF +# SUCH DAMAGE. + +source ./functions.sh +set -e +set -x + +requires_plugin sh +requires qemu-io --version + +files="retry-open-count" +rm -f $files +cleanup_fn rm -f $files + +echo 0 > retry-open-count +start_t=$SECONDS + +# Create a custom plugin which will test retrying open. +nbdkit -v -U - \ + sh - \ + --filter=retry retry-delay=1 \ + --run 'qemu-io -f raw -c "r -P0 0 512" $nbd || :' <<'EOF' +#!/usr/bin/env bash +case "$1" in + open) + # Count how many times the connection is (re-)opened. + read i < retry-open-count + echo $((i+1)) > retry-open-count + if test $i -lt 1; then + echo EIO >&2; exit 1 + fi + ;; + get_size) echo 512 ;; + pread) dd if=/dev/zero count=$3 iflag=count_bytes ;; + *) exit 2 ;; +esac +EOF + +# In this test we should see 1 failure: +# first open FAILS +# retry and wait 1 second +# second open PASSES + +# The minimum time for the test should be 1 second. +end_t=$SECONDS +if [ $((end_t - start_t)) -lt 1 ]; then + echo "$0: test ran too quickly" + exit 1 +fi + +# Check the handle was opened 2 times (first open + one reopen). +read open_count < retry-open-count +if [ $open_count -ne 2 ]; then + echo "$0: open-count ($open_count) != 2" + exit 1 +fi -- 2.39.1