Libguestfs - Oct 2019 - [nbdkit PATCH 0/5] More retry fixes

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

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

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

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

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

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

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 http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
Fedora Windows cross-compiler. Compile Windows programs, test, and
build Windows installers. Over 100 libraries supported.
http://fedoraproject.org/wiki/MinGW