After a retry, if the second connection has fewer permissions than the first, but we blindly call next_ops->FOO, we end up triggering an assertion failure in backend.c. This is particularly noticeable when the force_readonly flag is in effect, as that makes it much easier for there to be fewer permissions than before. Add testsuite coverage of pwrite to demonstrate. Signed-off-by: Eric Blake <eblake(a)redhat.com&gt; --- filters/retry/retry.c | 58 ++++++++++++++++++++++++++++++++++++++----- 1 file changed, 52 insertions(+), 6 deletions(-) diff --git a/filters/retry/retry.c b/filters/retry/retry.c index 59e7d4b8..00dbd091 100644 --- a/filters/retry/retry.c +++ b/filters/retry/retry.c @@ -106,6 +106,7 @@ retry_config (nbdkit_next_config *next, void *nxdata, struct retry_handle { int readonly; /* Save original readonly setting. */ + unsigned reopens; }; static void * @@ -123,6 +124,7 @@ retry_open (nbdkit_next_open *next, void *nxdata, int readonly) } h->readonly = readonly; + h->reopens = 0; return h; } @@ -132,6 +134,7 @@ retry_close (void *handle) { struct retry_handle *h = handle; + nbdkit_debug ("reopens needed: %u", h->reopens); free (h); } @@ -179,6 +182,7 @@ do_retry (struct retry_handle *h, data->delay *= 2; /* Reopen the connection. */ + h->reopens++; if (next_ops->reopen (nxdata, h->readonly || force_readonly) == -1) { /* If the reopen fails we treat it the same way as a command * failing. @@ -218,7 +222,16 @@ retry_pwrite (struct nbdkit_next_ops *next_ops, void *nxdata, int r; again: - r = next_ops->pwrite (nxdata, buf, count, offset, flags, err); + if (h->reopens && force_readonly) { + *err = EROFS; + return -1; + } + if (next_ops->can_write (nxdata) != 1) { + *err = EROFS; + r = -1; + } + else + r = next_ops->pwrite (nxdata, buf, count, offset, flags, err); if (r == -1 && do_retry (h, &data, next_ops, nxdata, err)) goto again; return r; @@ -236,7 +249,16 @@ retry_trim (struct nbdkit_next_ops *next_ops, void *nxdata, int r; again: - r = next_ops->trim (nxdata, count, offset, flags, err); + if (h->reopens && force_readonly) { + *err = EROFS; + return -1; + } + if (next_ops->can_trim (nxdata) != 1) { + *err = EROFS; + r = -1; + } + else + r = next_ops->trim (nxdata, count, offset, flags, err); if (r == -1 && do_retry (h, &data, next_ops, nxdata, err)) goto again; return r; @@ -253,7 +275,12 @@ retry_flush (struct nbdkit_next_ops *next_ops, void *nxdata, int r; again: - r = next_ops->flush (nxdata, flags, err); + if (next_ops->can_flush (nxdata) != 1) { + *err = EIO; + r = -1; + } + else + r = next_ops->flush (nxdata, flags, err); if (r == -1 && do_retry (h, &data, next_ops, nxdata, err)) goto again; return r; @@ -271,7 +298,16 @@ retry_zero (struct nbdkit_next_ops *next_ops, void *nxdata, int r; again: - r = next_ops->zero (nxdata, count, offset, flags, err); + if (h->reopens && force_readonly) { + *err = EROFS; + return -1; + } + if (next_ops->can_zero (nxdata) < NBDKIT_ZERO_EMULATE) { + *err = EROFS; + r = -1; + } + else + r = next_ops->zero (nxdata, count, offset, flags, err); if (r == -1 && do_retry (h, &data, next_ops, nxdata, err)) goto again; return r; @@ -289,7 +325,12 @@ retry_extents (struct nbdkit_next_ops *next_ops, void *nxdata, int r; again: - r = next_ops->extents (nxdata, count, offset, flags, extents, err); + if (next_ops->can_extents (nxdata) != 1) { + *err = EIO; + r = -1; + } + else + r = next_ops->extents (nxdata, count, offset, flags, extents, err); if (r == -1 && do_retry (h, &data, next_ops, nxdata, err)) goto again; return r; @@ -307,7 +348,12 @@ retry_cache (struct nbdkit_next_ops *next_ops, void *nxdata, int r; again: - r = next_ops->cache (nxdata, count, offset, flags, err); + if (next_ops->can_cache (nxdata) != 1) { + *err = EIO; + r = -1; + } + else + r = next_ops->cache (nxdata, count, offset, flags, err); if (r == -1 && do_retry (h, &data, next_ops, nxdata, err)) goto again; return r; -- 2.21.0

Add testsuite coverage for the previous patch. When the retry filter forces a readonly reconnection, all subsequent attempts to read should instantly fail without trying further reconnects. Signed-off-by: Eric Blake <eblake(a)redhat.com&gt; --- tests/Makefile.am | 2 + tests/test-retry-readonly.sh | 96 ++++++++++++++++++++++++++++++++++++ 2 files changed, 98 insertions(+) create mode 100755 tests/test-retry-readonly.sh diff --git a/tests/Makefile.am b/tests/Makefile.am index 325ca84d..4cf0325e 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -120,6 +120,7 @@ EXTRA_DIST = \ test.rb \ test-readahead-copy.sh \ test-retry.sh \ + test-retry-readonly.sh \ test-retry-reopen-fail.sh \ test-shutdown.sh \ test-ssh.sh \ @@ -1057,6 +1058,7 @@ test_readahead_LDADD = libtest.la $(LIBGUESTFS_LIBS) # retry filter test. TESTS += \ test-retry.sh \ + test-retry-readonly.sh \ test-retry-reopen-fail.sh \ $(NULL) diff --git a/tests/test-retry-readonly.sh b/tests/test-retry-readonly.sh new file mode 100755 index 00000000..710c2e93 --- /dev/null +++ b/tests/test-retry-readonly.sh @@ -0,0 +1,96 @@ +#!/usr/bin/env bash +# nbdkit +# Copyright (C) 2018-2019 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 qemu-io --version + +files="retry-readonly-count retry-readonly-open-count" +rm -f $files +cleanup_fn rm -f $files + +touch retry-readonly-count retry-readonly-open-count +start_t=$SECONDS + +# Create a custom plugin which will test retrying. +nbdkit -v -U - \ + sh - \ + --filter=retry retry-delay=1 retry-readonly=yes \ + --run 'qemu-io -f raw -c "w 0 512" -c "w 0 512" $nbd || :' <<'EOF' +#!/usr/bin/env bash +case "$1" in + open) + # Count how many times the connection is (re-)opened. + read i < retry-readonly-open-count + echo $((i+1)) > retry-readonly-open-count + ;; + can_write) ;; + pwrite) + # Fail 3 times then succeed - however, force-readonly means we will + # never return here after the first failure. + read i < retry-readonly-count + ((i++)) + echo $i > retry-readonly-count + if [ $i -le 3 ]; then + echo "EIO pwrite failed" >&2 + exit 1 + else + : drop data + fi + ;; + + get_size) echo 512 ;; + *) exit 2 ;; +esac +EOF + +# In this test we should see 3 failures: +# first pwrite FAILS +# retry and wait 1 seconds +# first pwrite FAILS immediately now that connection is readonly +# second pwrite FAILS immediately + +# 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-readonly-open-count +if [ $open_count -ne 2 ]; then + echo "$0: open-count ($open_count) != 2" + exit 1 +fi -- 2.21.0

Add coverage for the previous patch. Each retry of .extents must be into a fresh extents object, to avoid mismatch between provided and expected offsets. Signed-off-by: Eric Blake <eblake(a)redhat.com&gt; --- tests/Makefile.am | 2 + tests/test-retry-extents.sh | 114 ++++++++++++++++++++++++++++++++++++ 2 files changed, 116 insertions(+) create mode 100755 tests/test-retry-extents.sh diff --git a/tests/Makefile.am b/tests/Makefile.am index 4cf0325e..60cba6c5 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -120,6 +120,7 @@ EXTRA_DIST = \ test.rb \ test-readahead-copy.sh \ test-retry.sh \ + test-retry-extents.sh \ test-retry-readonly.sh \ test-retry-reopen-fail.sh \ test-shutdown.sh \ @@ -1059,6 +1060,7 @@ test_readahead_LDADD = libtest.la $(LIBGUESTFS_LIBS) TESTS += \ test-retry.sh \ test-retry-readonly.sh \ + test-retry-extents.sh \ test-retry-reopen-fail.sh \ $(NULL) diff --git a/tests/test-retry-extents.sh b/tests/test-retry-extents.sh new file mode 100755 index 00000000..ffcef311 --- /dev/null +++ b/tests/test-retry-extents.sh @@ -0,0 +1,114 @@ +#!/usr/bin/env bash +# nbdkit +# Copyright (C) 2018-2019 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 nbdsh --base-allocation -c 'quit()' + +files="retry-extents-count retry-extents-open-count" +rm -f $files +cleanup_fn rm -f $files + +touch retry-extents-count retry-extents-open-count +start_t=$SECONDS + +# Create a custom plugin which will test retrying. +nbdkit -v -U - \ + sh - \ + --filter=retry retry-delay=1 \ + --run 'nbdsh --base-allocation --uri $uri -c " +entries = [] +def f (metacontext, offset, e, err): + global entries + assert err.value == 0 + assert metacontext == nbd.CONTEXT_BASE_ALLOCATION + entries = e +h.block_status (1024, 0, f) +assert entries == [ 512, 0, + 512, 3] + "' <<'EOF' +#!/usr/bin/env bash +case "$1" in + open) + # Count how many times the connection is (re-)opened. + read i < retry-extents-open-count + echo $((i+1)) > retry-extents-open-count + ;; + extents) + # Fail in three different ways then succeed. + read i < retry-extents-count + ((i++)) + echo $i > retry-extents-count + case $i in + 1) echo "EIO pread failed" >&2; exit 1;; + 2) echo "garbage" + exit 0;; + 3) echo "0 512 0" + echo "garbage" + exit 0;; + *) echo "0 512 0" + echo "512 512 3" + exit 0;; + esac + ;; + + can_extents) exit 0 ;; + get_size) echo 1024 ;; + *) exit 2 ;; +esac +EOF + +# In this test we should see 3 failures: +# extents FAILS +# retry and wait 1 seconds +# extents FAILS +# retry and wait 2 seconds +# extents FAILS +# retry and wait 4 seconds +# extents 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 + +# Check the handle was opened 4 times (first open + one reopen for +# each retry). +read open_count < retry-extents-open-count +if [ $open_count -ne 4 ]; then + echo "$0: open-count ($open_count) != 4" + exit 1 +fi -- 2.21.0