Libguestfs - Jan 2023 - [nbdkit PATCH 0/2] retry: add support for retrying .open

In https://bugzilla.redhat.com/show_bug.cgi?id=1841820, it was pointed
out that the retry filter not retrying .open means that an ssh
connection (such as in a vmx+ssh v2v conversion) fails when the ssh
connection itself cannot be retried.  A year ago, this was an inherent
limitation of our retry implementation; but in the meantime, my work
to allow filters to open independent backends has made it feasible to
implement.

Eric Blake (2):
  retry: Add in retry support during .open
  retry: Test previous patch

 tests/Makefile.am        |  2 +
 server/backend.c         |  7 ++-
 filters/retry/retry.c    | 98 ++++++++++++++++++++++++----------------
 tests/test-retry-open.sh | 85 ++++++++++++++++++++++++++++++++++
 4 files changed, 150 insertions(+), 42 deletions(-)
 create mode 100755 tests/test-retry-open.sh

-- 
2.39.1

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