Eric Blake
2020-Mar-17 03:36 UTC
[Libguestfs] [nbdkit PATCH 0/4] Fix testsuite hang with nbd-stadalone
Either patch 1 or patch 2 in isolation is sufficient to fix the problem that Rich forwarded on from an archlinux tester (name so I can credit them?). But both patches should be applied, as well as backported to appropriate stable branches, to maximize cross-version interoperability of nbdkit vs. plugins. Patch 3 will let us detect future similar bugs much faster. I want patch 4 to ensure that we guarantee our API in the future, but it isn't finished yet. Then I still need to follow up with my threat to remove nbd-standalone for 1.20 (relying on libnbd is a lot easier now than it was when we first added optional libnbd support in 1.14). Eric Blake (4): server: Normalize plugin can_* values nbd: Normalize return values of can_* tests: Don't let test-parallel-* hang on nbdkit bug RFC tests: Add test to cover unusual .can_flush return tests/Makefile.am | 21 +++++++ server/plugins.c | 24 ++++--- plugins/nbd/nbd-standalone.c | 12 ++-- tests/test-flush.sh | 85 +++++++++++++++++++++++++ tests/test-parallel-file.sh | 19 +++--- tests/test-parallel-nbd.sh | 13 ++-- tests/test-parallel-sh.sh | 19 +++--- tests/test-flush-plugin.c | 119 +++++++++++++++++++++++++++++++++++ 8 files changed, 275 insertions(+), 37 deletions(-) create mode 100755 tests/test-flush.sh create mode 100644 tests/test-flush-plugin.c -- 2.25.1
Eric Blake
2020-Mar-17 03:36 UTC
[Libguestfs] [nbdkit PATCH 1/4] server: Normalize plugin can_* values
The documentation for .can_write and friends mentioned merely that the
callback must return a boolean value, or -1 on failure. Historically,
we have treated any non-zero value other than -1 as true, and some
plugins have sloppily relied on this behavior, including the
standalone nbd plugin prior to the use of libnbd in v1.14. But in
1.15.1, we added an assert in our backend code if an enabled feature
bit is not exactly 1. To avoid tripping the assertion, we have to
normalize the plugin values before storing our internal value.
Fixes: c306fa93ab
Signed-off-by: Eric Blake <eblake@redhat.com>
---
server/plugins.c | 24 ++++++++++++++++--------
1 file changed, 16 insertions(+), 8 deletions(-)
diff --git a/server/plugins.c b/server/plugins.c
index 78ed6723..4a4fcdee 100644
--- a/server/plugins.c
+++ b/server/plugins.c
@@ -1,5 +1,5 @@
/* nbdkit
- * Copyright (C) 2013-2019 Red Hat Inc.
+ * Copyright (C) 2013-2020 Red Hat Inc.
*
* Redistribution and use in source and binary forms, with or without
* modification, are permitted provided that the following conditions are
@@ -308,13 +308,21 @@ plugin_get_size (struct backend *b, void *handle)
return p->plugin.get_size (handle);
}
+static int normalize_bool (int value)
+{
+ if (value == -1 || value == 0)
+ return value;
+ /* Normalize all other non-zero values to true */
+ return 1;
+}
+
static int
plugin_can_write (struct backend *b, void *handle)
{
struct backend_plugin *p = container_of (b, struct backend_plugin, backend);
if (p->plugin.can_write)
- return p->plugin.can_write (handle);
+ return normalize_bool (p->plugin.can_write (handle));
else
return p->plugin.pwrite || p->plugin._pwrite_v1;
}
@@ -325,7 +333,7 @@ plugin_can_flush (struct backend *b, void *handle)
struct backend_plugin *p = container_of (b, struct backend_plugin, backend);
if (p->plugin.can_flush)
- return p->plugin.can_flush (handle);
+ return normalize_bool (p->plugin.can_flush (handle));
else
return p->plugin.flush || p->plugin._flush_v1;
}
@@ -336,7 +344,7 @@ plugin_is_rotational (struct backend *b, void *handle)
struct backend_plugin *p = container_of (b, struct backend_plugin, backend);
if (p->plugin.is_rotational)
- return p->plugin.is_rotational (handle);
+ return normalize_bool (p->plugin.is_rotational (handle));
else
return 0; /* assume false */
}
@@ -347,7 +355,7 @@ plugin_can_trim (struct backend *b, void *handle)
struct backend_plugin *p = container_of (b, struct backend_plugin, backend);
if (p->plugin.can_trim)
- return p->plugin.can_trim (handle);
+ return normalize_bool (p->plugin.can_trim (handle));
else
return p->plugin.trim || p->plugin._trim_v1;
}
@@ -380,7 +388,7 @@ plugin_can_fast_zero (struct backend *b, void *handle)
int r;
if (p->plugin.can_fast_zero)
- return p->plugin.can_fast_zero (handle);
+ return normalize_bool (p->plugin.can_fast_zero (handle));
/* Advertise support for fast zeroes if no .zero or .can_zero is
* false: in those cases, we fail fast instead of using .pwrite.
* This also works when v1 plugin has only ._zero_v1.
@@ -399,7 +407,7 @@ plugin_can_extents (struct backend *b, void *handle)
struct backend_plugin *p = container_of (b, struct backend_plugin, backend);
if (p->plugin.can_extents)
- return p->plugin.can_extents (handle);
+ return normalize_bool (p->plugin.can_extents (handle));
else
return p->plugin.extents != NULL;
}
@@ -430,7 +438,7 @@ plugin_can_multi_conn (struct backend *b, void *handle)
struct backend_plugin *p = container_of (b, struct backend_plugin, backend);
if (p->plugin.can_multi_conn)
- return p->plugin.can_multi_conn (handle);
+ return normalize_bool (p->plugin.can_multi_conn (handle));
else
return 0; /* assume false */
}
--
2.25.1
Eric Blake
2020-Mar-17 03:36 UTC
[Libguestfs] [nbdkit PATCH 2/4] nbd: Normalize return values of can_*
Although nbdkit documents that any positive value should be treated as
success to the .can_* callbacks, we had a window of releases where
anything other than 1 could trigger an assertion failure, fixed in the
previous patch. Update what we return to avoid tripping the bug in
broken nbdkit.
Our return value has been latent since 0e8e8eb11d (v1.1.17), but the
assertion failures did not occur until c306fa93ab and neighboring
commits (v1.15.1). As v1.13.6 was the point where we started
preferring builds against libnbd, where we always returned 1 on
success, the problem was not detected until now; but it is still in
the wild for any user mixing old plugins with new libnbd.
Signed-off-by: Eric Blake <eblake@redhat.com>
---
plugins/nbd/nbd-standalone.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/plugins/nbd/nbd-standalone.c b/plugins/nbd/nbd-standalone.c
index cbf9dae7..4230ecd9 100644
--- a/plugins/nbd/nbd-standalone.c
+++ b/plugins/nbd/nbd-standalone.c
@@ -1,5 +1,5 @@
/* nbdkit
- * Copyright (C) 2017-2019 Red Hat Inc.
+ * Copyright (C) 2017-2020 Red Hat Inc.
*
* Redistribution and use in source and binary forms, with or without
* modification, are permitted provided that the following conditions are
@@ -1175,7 +1175,7 @@ nbd_can_flush (void *handle)
{
struct handle *h = handle;
- return h->flags & NBD_FLAG_SEND_FLUSH;
+ return !!(h->flags & NBD_FLAG_SEND_FLUSH);
}
static int
@@ -1183,7 +1183,7 @@ nbd_is_rotational (void *handle)
{
struct handle *h = handle;
- return h->flags & NBD_FLAG_ROTATIONAL;
+ return !!(h->flags & NBD_FLAG_ROTATIONAL);
}
static int
@@ -1191,7 +1191,7 @@ nbd_can_trim (void *handle)
{
struct handle *h = handle;
- return h->flags & NBD_FLAG_SEND_TRIM;
+ return !!(h->flags & NBD_FLAG_SEND_TRIM);
}
static int
@@ -1199,7 +1199,7 @@ nbd_can_zero (void *handle)
{
struct handle *h = handle;
- return h->flags & NBD_FLAG_SEND_WRITE_ZEROES;
+ return !!(h->flags & NBD_FLAG_SEND_WRITE_ZEROES);
}
static int
@@ -1215,7 +1215,7 @@ nbd_can_multi_conn (void *handle)
{
struct handle *h = handle;
- return h->flags & NBD_FLAG_CAN_MULTI_CONN;
+ return !!(h->flags & NBD_FLAG_CAN_MULTI_CONN);
}
static int
--
2.25.1
Eric Blake
2020-Mar-17 03:36 UTC
[Libguestfs] [nbdkit PATCH 3/4] tests: Don't let test-parallel-* hang on nbdkit bug
If nbdkit has a bug (such as the nbd-standalone bug fixed in the
previous commit), qemu-io ends up waiting forever rather than
realizing that if the server disappears unexpectedly then qemu-io
should quit. So add timeouts so the testsuite will flag the problem
instead of hang (tested by reordering this commit before the
previous).
It's trickier than I expected: from the command line, 'timeout 10s
qemu-io ...' works just fine, but from a script, it fails. Why?
Because timeout defaults to creating a new process group when run
non-interactively, but qemu-io is generally an interactive program
that expects to be able to modify the terminal settings when stdin is
a terminal, even when it will not be reading from stdin because of the
use of -c. When stdin is inherited but no longer the controlling
terminal because of timeout's new process group, qemu-io fails to make
progress. As solutions, we can either use 'timeout --foreground', or
redirect stdin to something that is not a terminal; this patch uses
the latter.
Signed-off-by: Eric Blake <eblake@redhat.com>
---
tests/test-parallel-file.sh | 19 ++++++++++---------
tests/test-parallel-nbd.sh | 13 ++++++++-----
tests/test-parallel-sh.sh | 19 ++++++++++---------
3 files changed, 28 insertions(+), 23 deletions(-)
diff --git a/tests/test-parallel-file.sh b/tests/test-parallel-file.sh
index 20276d48..136c2db5 100755
--- a/tests/test-parallel-file.sh
+++ b/tests/test-parallel-file.sh
@@ -1,6 +1,6 @@
#!/usr/bin/env bash
# nbdkit
-# Copyright (C) 2017-2019 Red Hat Inc.
+# Copyright (C) 2017-2020 Red Hat Inc.
#
# Redistribution and use in source and binary forms, with or without
# modification, are permitted provided that the following conditions are
@@ -35,6 +35,7 @@ source ./functions.sh
# Check file-data was created by Makefile and qemu-io exists.
requires test -f file-data
requires qemu-io --version
+requires timeout --version
nbdkit --dump-plugin file | grep -q ^thread_model=parallel ||
{ echo "nbdkit lacks support for parallel requests"; exit 77; }
@@ -43,8 +44,8 @@ cleanup_fn rm -f test-parallel-file.data
test-parallel-file.out
# Populate file, and sanity check that qemu-io can issue parallel requests
printf '%1024s' . > test-parallel-file.data
-qemu-io -f raw -c "aio_write -P 1 0 512" -c "aio_write -P 2 512
512" \
- -c aio_flush test-parallel-file.data ||
+timeout 10s </dev/null qemu-io -f raw -c "aio_write -P 1 0 512" \
+ -c "aio_write -P 2 512 512" -c aio_flush
test-parallel-file.data ||
{ echo "'qemu-io' can't drive parallel requests";
exit 77; }
# Set up the file plugin to delay both reads and writes (for a good chance
@@ -55,8 +56,8 @@ qemu-io -f raw -c "aio_write -P 1 0 512" -c
"aio_write -P 2 512 512" \
# With --threads=1, the write should complete first because it was issued first
nbdkit -v -t 1 -U - --filter=delay file test-parallel-file.data \
- wdelay=2 rdelay=1 --run 'qemu-io -f raw -c "aio_write -P 2 512
512" \
- -c "aio_read -P 1 0 512" -c aio_flush
$nbd' |
+ wdelay=2 rdelay=1 --run 'timeout 10s </dev/null qemu-io -f raw \
+ -c "aio_write -P 2 512 512" -c "aio_read -P 1 0 512" -c
aio_flush $nbd' |
tee test-parallel-file.out
if test "$(grep '512/512' test-parallel-file.out)" != \
"wrote 512/512 bytes at offset 512
@@ -66,8 +67,8 @@ fi
# With default --threads, the faster read should complete first
nbdkit -v -U - --filter=delay file test-parallel-file.data \
- wdelay=2 rdelay=1 --run 'qemu-io -f raw -c "aio_write -P 2 512
512" \
- -c "aio_read -P 1 0 512" -c aio_flush
$nbd' |
+ wdelay=2 rdelay=1 --run 'timeout 10s </dev/null qemu-io -f raw \
+ -c "aio_write -P 2 512 512" -c "aio_read -P 1 0 512" -c
aio_flush $nbd' |
tee test-parallel-file.out
if test "$(grep '512/512' test-parallel-file.out)" != \
"read 512/512 bytes at offset 0
@@ -77,8 +78,8 @@ fi
# With --filter=noparallel, the write should complete first because it was
issued first
nbdkit -v -U - --filter=noparallel --filter=delay file test-parallel-file.data
\
- wdelay=2 rdelay=1 --run 'qemu-io -f raw -c "aio_write -P 2 512
512" \
- -c "aio_read -P 1 0 512" -c aio_flush
$nbd' |
+ wdelay=2 rdelay=1 --run 'timeout 10s </dev/null qemu-io -f raw \
+ -c "aio_write -P 2 512 512" -c "aio_read -P 1 0 512" -c
aio_flush $nbd' |
tee test-parallel-file.out
if test "$(grep '512/512' test-parallel-file.out)" != \
"wrote 512/512 bytes at offset 512
diff --git a/tests/test-parallel-nbd.sh b/tests/test-parallel-nbd.sh
index 4e546df4..1fdf7df4 100755
--- a/tests/test-parallel-nbd.sh
+++ b/tests/test-parallel-nbd.sh
@@ -1,6 +1,6 @@
#!/usr/bin/env bash
# nbdkit
-# Copyright (C) 2017-2019 Red Hat Inc.
+# Copyright (C) 2017-2020 Red Hat Inc.
#
# Redistribution and use in source and binary forms, with or without
# modification, are permitted provided that the following conditions are
@@ -35,6 +35,7 @@ source ./functions.sh
# Check file-data was created by Makefile and qemu-io exists.
requires test -f file-data
requires qemu-io --version
+requires timeout --version
nbdkit --dump-plugin nbd | grep -q ^thread_model=parallel ||
{ echo "nbdkit lacks support for parallel requests"; exit 77; }
@@ -46,8 +47,8 @@ cleanup_fn rm -f $files
# Populate file, and sanity check that qemu-io can issue parallel requests
printf '%1024s' . > test-parallel-nbd.data
-qemu-io -f raw -c "aio_write -P 1 0 512" -c "aio_write -P 2 512
512" \
- -c aio_flush test-parallel-nbd.data ||
+timeout 10s </dev/null qemu-io -f raw -c "aio_write -P 1 0 512" \
+ -c "aio_write -P 2 512 512" -c aio_flush
test-parallel-nbd.data ||
{ echo "'qemu-io' can't drive parallel requests";
exit 77; }
# Set up the file plugin to delay both reads and writes (for a good chance
@@ -63,7 +64,8 @@ start_nbdkit -P test-parallel-nbd.pid \
# With --threads=1, the write should complete first because it was issued first
nbdkit -v -t 1 -U - nbd socket=$sock --run '
- qemu-io -f raw -c "aio_write -P 2 512 512" -c "aio_read -P 1 0
512" \
+ timeout 10s </dev/null qemu-io -f raw \
+ -c "aio_write -P 2 512 512" -c "aio_read -P 1 0 512" \
-c aio_flush $nbd' | tee test-parallel-nbd.out
if test "$(grep '512/512' test-parallel-nbd.out)" != \
"wrote 512/512 bytes at offset 512
@@ -73,7 +75,8 @@ fi
# With default --threads, the faster read should complete first
nbdkit -v -U - nbd socket=$sock --run '
- qemu-io -f raw -c "aio_write -P 2 512 512" -c "aio_read -P 1 0
512" \
+ timeout 10s </dev/null qemu-io -f raw \
+ -c "aio_write -P 2 512 512" -c "aio_read -P 1 0 512" \
-c aio_flush $nbd' | tee test-parallel-nbd.out
if test "$(grep '512/512' test-parallel-nbd.out)" != \
"read 512/512 bytes at offset 0
diff --git a/tests/test-parallel-sh.sh b/tests/test-parallel-sh.sh
index 15d8875d..4cc93c33 100755
--- a/tests/test-parallel-sh.sh
+++ b/tests/test-parallel-sh.sh
@@ -1,6 +1,6 @@
#!/usr/bin/env bash
# nbdkit
-# Copyright (C) 2017-2019 Red Hat Inc.
+# Copyright (C) 2017-2020 Red Hat Inc.
#
# Redistribution and use in source and binary forms, with or without
# modification, are permitted provided that the following conditions are
@@ -35,6 +35,7 @@ source ./functions.sh
# Check file-data was created by Makefile and qemu-io exists.
requires test -f file-data
requires qemu-io --version
+requires timeout --version
nbdkit --dump-plugin sh | grep -q ^thread_model=parallel ||
{ echo "nbdkit lacks support for parallel requests"; exit 77; }
@@ -43,8 +44,8 @@ cleanup_fn rm -f test-parallel-sh.data test-parallel-sh.out
test-parallel-sh.scr
# Populate file, and sanity check that qemu-io can issue parallel requests
printf '%1024s' . > test-parallel-sh.data
-qemu-io -f raw -c "aio_write -P 1 0 512" -c "aio_write -P 2 512
512" \
- -c aio_flush test-parallel-sh.data ||
+timeout 10s </dev/null qemu-io -f raw -c "aio_write -P 1 0 512" \
+ -c "aio_write -P 2 512 512" -c aio_flush
test-parallel-sh.data ||
{ echo "'qemu-io' can't drive parallel requests";
exit 77; }
# Set up the sh plugin to delay both reads and writes (for a good
@@ -96,8 +97,8 @@ chmod +x test-parallel-sh.script
# With --threads=1, the write should complete first because it was issued first
nbdkit -v -t 1 -U - --filter=delay sh test-parallel-sh.script \
- wdelay=2 rdelay=1 --run 'qemu-io -f raw -c "aio_write -P 2 512
512" \
- -c "aio_read -P 1 0 512" -c aio_flush
$nbd' |
+ wdelay=2 rdelay=1 --run 'timeout 10s </dev/null qemu-io -f raw \
+ -c "aio_write -P 2 512 512" -c "aio_read -P 1 0 512" -c
aio_flush $nbd' |
tee test-parallel-sh.out
if test "$(grep '512/512' test-parallel-sh.out)" != \
"wrote 512/512 bytes at offset 512
@@ -107,8 +108,8 @@ fi
# With default --threads, the faster read should complete first
nbdkit -v -U - --filter=delay sh test-parallel-sh.script \
- wdelay=2 rdelay=1 --run 'qemu-io -f raw -c "aio_write -P 2 512
512" \
- -c "aio_read -P 1 0 512" -c aio_flush
$nbd' |
+ wdelay=2 rdelay=1 --run 'timeout 10s </dev/null qemu-io -f raw \
+ -c "aio_write -P 2 512 512" -c "aio_read -P 1 0 512" -c
aio_flush $nbd' |
tee test-parallel-sh.out
if test "$(grep '512/512' test-parallel-sh.out)" != \
"read 512/512 bytes at offset 0
@@ -120,8 +121,8 @@ fi
# issued first. Also test that the log filter doesn't leak an fd
nbdkit -v -U - --filter=noparallel --filter=log --filter=delay \
sh test-parallel-sh.script logfile=/dev/null \
- wdelay=2 rdelay=1 --run 'qemu-io -f raw -c "aio_write -P 2 512
512" \
- -c "aio_read -P 1 0 512" -c aio_flush
$nbd' |
+ wdelay=2 rdelay=1 --run 'timeout 10s </dev/null qemu-io -f raw \
+ -c "aio_write -P 2 512 512" -c "aio_read -P 1 0 512" -c
aio_flush $nbd' |
tee test-parallel-sh.out
if test "$(grep '512/512' test-parallel-sh.out)" != \
"wrote 512/512 bytes at offset 512
--
2.25.1
Eric Blake
2020-Mar-17 03:36 UTC
[Libguestfs] [nbdkit PATCH 4/4] RFC tests: Add test to cover unusual .can_flush return
We want some testsuite coverage of handling non-1 .can_flush values.
The only in-tree plugin with this property is nbd-standalone, but it
doesn't get frequently tested, not to mention that the next patch will
change it to work with older nbdkit.
Signed-off-by: Eric Blake <eblake@redhat.com>
---
work in progress: currently compiles but fails during test; posting it
now to show my thoughts before I go to bed
tests/Makefile.am | 21 +++++++
tests/test-flush.sh | 85 +++++++++++++++++++++++++++
tests/test-flush-plugin.c | 119 ++++++++++++++++++++++++++++++++++++++
3 files changed, 225 insertions(+)
create mode 100755 tests/test-flush.sh
create mode 100644 tests/test-flush-plugin.c
diff --git a/tests/Makefile.am b/tests/Makefile.am
index 65dd148d..7e0024d3 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -116,6 +116,7 @@ EXTRA_DIST = \
test-extentlist.sh \
test-file-extents.sh \
test-floppy.sh \
+ test-flush.sh \
test-foreground.sh \
test-fua.sh \
test-full.sh \
@@ -267,6 +268,7 @@ TESTS += \
test-foreground.sh \
test-debug-flags.sh \
test-long-name.sh \
+ test-flush.sh \
test-swap.sh \
test-shutdown.sh \
$(NULL)
@@ -282,6 +284,25 @@ test_socket_activation_CPPFLAGS = \
$(NULL)
test_socket_activation_CFLAGS = $(WARNINGS_CFLAGS)
+# check_LTLIBRARIES won't build a shared library (see automake manual).
+# So we have to do this and add a dependency.
+noinst_LTLIBRARIES += \
+ test-flush-plugin.la \
+ $(NULL)
+test-flush.sh: test-flush-plugin.la
+
+test_flush_plugin_la_SOURCES = \
+ test-flush-plugin.c \
+ $(top_srcdir)/include/nbdkit-plugin.h \
+ $(NULL)
+test_flush_plugin_la_CPPFLAGS = -I$(top_srcdir)/include
+test_flush_plugin_la_CFLAGS = $(WARNINGS_CFLAGS)
+# For use of the -rpath option, see:
+# https://lists.gnu.org/archive/html/libtool/2007-07/msg00067.html
+test_flush_plugin_la_LDFLAGS = \
+ -module -avoid-version -shared -rpath /nowhere \
+ $(NULL)
+
# check_LTLIBRARIES won't build a shared library (see automake manual).
# So we have to do this and add a dependency.
noinst_LTLIBRARIES += \
diff --git a/tests/test-flush.sh b/tests/test-flush.sh
new file mode 100755
index 00000000..54801b3a
--- /dev/null
+++ b/tests/test-flush.sh
@@ -0,0 +1,85 @@
+#!/usr/bin/env bash
+# nbdkit
+# Copyright (C) 2019-2020 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 -x
+
+requires nbdsh --version
+
+plugin=.libs/test-flush-plugin.so
+requires test -f $plugin
+
+# Test what happens when a plugin fails .can_flush
+nbdsh -c '
+try:
+ h.connect_command (["nbdkit", "-s", "-v",
"'$plugin'", "level=-1"])
+except nbd.Error as ex:
+ exit (0)
+# If we got here, things are broken
+exit (1)
+'
+
+# A read-only connection never triggers .can_flush
+nbdsh -c '
+h.connect_command (["nbdkit", "-s", "-r",
"-v", "'$plugin'", "level=-1"])
+assert h.is_read_only () == 1
+assert h.can_flush () == 0
+assert h.can_fua () == 0
+'
+
+# Disable flush and FUA
+nbdsh -c '
+h.connect_command (["nbdkit", "-s", "-v",
"'$plugin'", "0"])
+assert h.is_read_only () == 0
+assert h.can_flush () == 0
+assert h.can_fua () == 0
+'
+
+# Normal flush, emulated FUA
+nbdsh -c '
+h.connect_command (["nbdkit", "-s", "-v",
"'$plugin'", "1"])
+assert h.is_read_only () == 0
+assert h.can_flush () == 1
+assert h.can_fua () == 1
+# try direct flush; check that output has "handling flush"
+# try write with FUA; check that output has "handling flush"
+'
+
+# Unusual return value for .can_flush, native FUA
+nbdsh -c '
+h.connect_command (["nbdkit", "-s", "-v",
"'$plugin'", "2"])
+assert h.is_read_only () == 0
+assert h.can_flush () == 1
+assert h.can_fua () == 1
+# try direct flush; check that output has "handling flush"
+# try write with FUA; check that output has "handling native FUA"
+'
diff --git a/tests/test-flush-plugin.c b/tests/test-flush-plugin.c
new file mode 100644
index 00000000..5553c607
--- /dev/null
+++ b/tests/test-flush-plugin.c
@@ -0,0 +1,119 @@
+/* nbdkit
+ * Copyright (C) 2013-2020 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.
+ */
+
+#include <config.h>
+
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+
+#define NBDKIT_API_VERSION 2
+
+#include <nbdkit-plugin.h>
+
+/* level abuses our knowledge of internal nbdkit values:
+ * -1: force error during connect
+ * 0: no flush, no FUA
+ * 1: flush works, FUA is emulated
+ * 2: flush works, FUA is native
+ */
+static int level;
+
+static int
+flush_config (const char *key, const char *value)
+{
+ if (strcmp (key, "level") == 0)
+ return nbdkit_parse_int (key, value, &level);
+ nbdkit_error ("unknown parameter '%s'", key);
+ return -1;
+}
+
+/* Implements both .can_flush and .can_fua */
+static int
+flush_level (void *handle)
+{
+ return level;
+}
+
+static void *
+flush_open (int readonly)
+{
+ return NBDKIT_HANDLE_NOT_NEEDED;
+}
+
+static int64_t
+flush_get_size (void *handle)
+{
+ return 1024*1024;
+}
+
+#define THREAD_MODEL NBDKIT_THREAD_MODEL_PARALLEL
+
+static int
+flush_pread (void *handle, void *buf, uint32_t count, uint64_t offset,
+ uint32_t flags)
+{
+ memset (buf, 0, count);
+ return 0;
+}
+
+static int
+flush_pwrite (void *handle, const void *buf, uint32_t count, uint64_t offset,
+ uint32_t flags)
+{
+ if (flags & NBDKIT_FLAG_FUA)
+ nbdkit_debug ("handling native FUA");
+ return 0;
+}
+
+static int
+flush_flush (void *handle, uint32_t flags)
+{
+ nbdkit_debug ("handling flush");
+ return 0;
+}
+
+static struct nbdkit_plugin plugin = {
+ .name = "flush",
+ .version = PACKAGE_VERSION,
+ .config = flush_config,
+ .magic_config_key = "level",
+ .open = flush_open,
+ .get_size = flush_get_size,
+ .pread = flush_pread,
+ .pwrite = flush_pwrite,
+ .can_flush = flush_level,
+ .can_fua = flush_level,
+ .flush = flush_flush,
+};
+
+NBDKIT_REGISTER_PLUGIN(plugin)
--
2.25.1
Richard W.M. Jones
2020-Mar-17 08:09 UTC
Re: [Libguestfs] [nbdkit PATCH 0/4] Fix testsuite hang with nbd-stadalone
On Mon, Mar 16, 2020 at 10:36:13PM -0500, Eric Blake wrote:> Either patch 1 or patch 2 in isolation is sufficient to fix the > problem that Rich forwarded on from an archlinux tester (name so I can > credit them?).Svenne Krap (https://svenne.dk) 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
Richard W.M. Jones
2020-Mar-17 08:10 UTC
Re: [Libguestfs] [nbdkit PATCH 1/4] server: Normalize plugin can_* values
On Mon, Mar 16, 2020 at 10:36:14PM -0500, Eric Blake wrote:> The documentation for .can_write and friends mentioned merely that the > callback must return a boolean value, or -1 on failure. Historically, > we have treated any non-zero value other than -1 as true, and some > plugins have sloppily relied on this behavior, including the > standalone nbd plugin prior to the use of libnbd in v1.14. But in > 1.15.1, we added an assert in our backend code if an enabled feature > bit is not exactly 1. To avoid tripping the assertion, we have to > normalize the plugin values before storing our internal value. > > Fixes: c306fa93ab > Signed-off-by: Eric Blake <eblake@redhat.com> > --- > server/plugins.c | 24 ++++++++++++++++-------- > 1 file changed, 16 insertions(+), 8 deletions(-) > > diff --git a/server/plugins.c b/server/plugins.c > index 78ed6723..4a4fcdee 100644 > --- a/server/plugins.c > +++ b/server/plugins.c > @@ -1,5 +1,5 @@ > /* nbdkit > - * Copyright (C) 2013-2019 Red Hat Inc. > + * Copyright (C) 2013-2020 Red Hat Inc. > * > * Redistribution and use in source and binary forms, with or without > * modification, are permitted provided that the following conditions are > @@ -308,13 +308,21 @@ plugin_get_size (struct backend *b, void *handle) > return p->plugin.get_size (handle); > } > > +static int normalize_bool (int value) > +{ > + if (value == -1 || value == 0) > + return value; > + /* Normalize all other non-zero values to true */ > + return 1; > +} > + > static int > plugin_can_write (struct backend *b, void *handle) > { > struct backend_plugin *p = container_of (b, struct backend_plugin, backend); > > if (p->plugin.can_write) > - return p->plugin.can_write (handle); > + return normalize_bool (p->plugin.can_write (handle)); > else > return p->plugin.pwrite || p->plugin._pwrite_v1; > } > @@ -325,7 +333,7 @@ plugin_can_flush (struct backend *b, void *handle) > struct backend_plugin *p = container_of (b, struct backend_plugin, backend); > > if (p->plugin.can_flush) > - return p->plugin.can_flush (handle); > + return normalize_bool (p->plugin.can_flush (handle)); > else > return p->plugin.flush || p->plugin._flush_v1; > } > @@ -336,7 +344,7 @@ plugin_is_rotational (struct backend *b, void *handle) > struct backend_plugin *p = container_of (b, struct backend_plugin, backend); > > if (p->plugin.is_rotational) > - return p->plugin.is_rotational (handle); > + return normalize_bool (p->plugin.is_rotational (handle)); > else > return 0; /* assume false */ > } > @@ -347,7 +355,7 @@ plugin_can_trim (struct backend *b, void *handle) > struct backend_plugin *p = container_of (b, struct backend_plugin, backend); > > if (p->plugin.can_trim) > - return p->plugin.can_trim (handle); > + return normalize_bool (p->plugin.can_trim (handle)); > else > return p->plugin.trim || p->plugin._trim_v1; > } > @@ -380,7 +388,7 @@ plugin_can_fast_zero (struct backend *b, void *handle) > int r; > > if (p->plugin.can_fast_zero) > - return p->plugin.can_fast_zero (handle); > + return normalize_bool (p->plugin.can_fast_zero (handle)); > /* Advertise support for fast zeroes if no .zero or .can_zero is > * false: in those cases, we fail fast instead of using .pwrite. > * This also works when v1 plugin has only ._zero_v1. > @@ -399,7 +407,7 @@ plugin_can_extents (struct backend *b, void *handle) > struct backend_plugin *p = container_of (b, struct backend_plugin, backend); > > if (p->plugin.can_extents) > - return p->plugin.can_extents (handle); > + return normalize_bool (p->plugin.can_extents (handle)); > else > return p->plugin.extents != NULL; > } > @@ -430,7 +438,7 @@ plugin_can_multi_conn (struct backend *b, void *handle) > struct backend_plugin *p = container_of (b, struct backend_plugin, backend); > > if (p->plugin.can_multi_conn) > - return p->plugin.can_multi_conn (handle); > + return normalize_bool (p->plugin.can_multi_conn (handle)); > else > return 0; /* assume false */ > } > --Nice one, ACK. I guess this one will need to be backported to (only?) 1.16 and 1.18? Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-p2v converts physical machines to virtual machines. Boot with a live CD or over the network (PXE) and turn machines into KVM guests. http://libguestfs.org/virt-v2v
Richard W.M. Jones
2020-Mar-17 08:12 UTC
Re: [Libguestfs] [nbdkit PATCH 2/4] nbd: Normalize return values of can_*
On Mon, Mar 16, 2020 at 10:36:15PM -0500, Eric Blake wrote:> Although nbdkit documents that any positive value should be treated as > success to the .can_* callbacks, we had a window of releases where > anything other than 1 could trigger an assertion failure, fixed in the > previous patch. Update what we return to avoid tripping the bug in > broken nbdkit. > > Our return value has been latent since 0e8e8eb11d (v1.1.17), but the > assertion failures did not occur until c306fa93ab and neighboring > commits (v1.15.1). As v1.13.6 was the point where we started > preferring builds against libnbd, where we always returned 1 on > success, the problem was not detected until now; but it is still in > the wild for any user mixing old plugins with new libnbd. > > Signed-off-by: Eric Blake <eblake@redhat.com> > --- > plugins/nbd/nbd-standalone.c | 12 ++++++------ > 1 file changed, 6 insertions(+), 6 deletions(-) > > diff --git a/plugins/nbd/nbd-standalone.c b/plugins/nbd/nbd-standalone.c > index cbf9dae7..4230ecd9 100644 > --- a/plugins/nbd/nbd-standalone.c > +++ b/plugins/nbd/nbd-standalone.c > @@ -1,5 +1,5 @@ > /* nbdkit > - * Copyright (C) 2017-2019 Red Hat Inc. > + * Copyright (C) 2017-2020 Red Hat Inc. > * > * Redistribution and use in source and binary forms, with or without > * modification, are permitted provided that the following conditions are > @@ -1175,7 +1175,7 @@ nbd_can_flush (void *handle) > { > struct handle *h = handle; > > - return h->flags & NBD_FLAG_SEND_FLUSH; > + return !!(h->flags & NBD_FLAG_SEND_FLUSH); > } > > static int > @@ -1183,7 +1183,7 @@ nbd_is_rotational (void *handle) > { > struct handle *h = handle; > > - return h->flags & NBD_FLAG_ROTATIONAL; > + return !!(h->flags & NBD_FLAG_ROTATIONAL); > } > > static int > @@ -1191,7 +1191,7 @@ nbd_can_trim (void *handle) > { > struct handle *h = handle; > > - return h->flags & NBD_FLAG_SEND_TRIM; > + return !!(h->flags & NBD_FLAG_SEND_TRIM); > } > > static int > @@ -1199,7 +1199,7 @@ nbd_can_zero (void *handle) > { > struct handle *h = handle; > > - return h->flags & NBD_FLAG_SEND_WRITE_ZEROES; > + return !!(h->flags & NBD_FLAG_SEND_WRITE_ZEROES); > } > > static int > @@ -1215,7 +1215,7 @@ nbd_can_multi_conn (void *handle) > { > struct handle *h = handle; > > - return h->flags & NBD_FLAG_CAN_MULTI_CONN; > + return !!(h->flags & NBD_FLAG_CAN_MULTI_CONN); > } > > static intWhile I don't like the idea of modifying plugins to fix broken nbdkit behaviour, you make a good case for this being safer, so: ACK Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com libguestfs lets you edit virtual machines. Supports shell scripting, bindings from many languages. http://libguestfs.org
Richard W.M. Jones
2020-Mar-17 08:16 UTC
Re: [Libguestfs] [nbdkit PATCH 3/4] tests: Don't let test-parallel-* hang on nbdkit bug
On Mon, Mar 16, 2020 at 10:36:16PM -0500, Eric Blake wrote:> If nbdkit has a bug (such as the nbd-standalone bug fixed in the > previous commit), qemu-io ends up waiting forever rather than > realizing that if the server disappears unexpectedly then qemu-io > should quit. So add timeouts so the testsuite will flag the problem > instead of hang (tested by reordering this commit before the > previous). > > It's trickier than I expected: from the command line, 'timeout 10s > qemu-io ...' works just fine, but from a script, it fails. Why? > Because timeout defaults to creating a new process group when run > non-interactively, but qemu-io is generally an interactive program > that expects to be able to modify the terminal settings when stdin is > a terminal, even when it will not be reading from stdin because of the > use of -c. When stdin is inherited but no longer the controlling > terminal because of timeout's new process group, qemu-io fails to make > progress. As solutions, we can either use 'timeout --foreground', or > redirect stdin to something that is not a terminal; this patch uses > the latter.Since the timeout is only used here as a last resort, can we make it much larger? This is only going to cause weird test failures on the usual suspects (z, armv7, valgrind, ...) The timeout could be 60s or even 5 minutes without it affecting normal users. With a change like that, ACK. Rich.> Signed-off-by: Eric Blake <eblake@redhat.com> > --- > tests/test-parallel-file.sh | 19 ++++++++++--------- > tests/test-parallel-nbd.sh | 13 ++++++++----- > tests/test-parallel-sh.sh | 19 ++++++++++--------- > 3 files changed, 28 insertions(+), 23 deletions(-) > > diff --git a/tests/test-parallel-file.sh b/tests/test-parallel-file.sh > index 20276d48..136c2db5 100755 > --- a/tests/test-parallel-file.sh > +++ b/tests/test-parallel-file.sh > @@ -1,6 +1,6 @@ > #!/usr/bin/env bash > # nbdkit > -# Copyright (C) 2017-2019 Red Hat Inc. > +# Copyright (C) 2017-2020 Red Hat Inc. > # > # Redistribution and use in source and binary forms, with or without > # modification, are permitted provided that the following conditions are > @@ -35,6 +35,7 @@ source ./functions.sh > # Check file-data was created by Makefile and qemu-io exists. > requires test -f file-data > requires qemu-io --version > +requires timeout --version > > nbdkit --dump-plugin file | grep -q ^thread_model=parallel || > { echo "nbdkit lacks support for parallel requests"; exit 77; } > @@ -43,8 +44,8 @@ cleanup_fn rm -f test-parallel-file.data test-parallel-file.out > > # Populate file, and sanity check that qemu-io can issue parallel requests > printf '%1024s' . > test-parallel-file.data > -qemu-io -f raw -c "aio_write -P 1 0 512" -c "aio_write -P 2 512 512" \ > - -c aio_flush test-parallel-file.data || > +timeout 10s </dev/null qemu-io -f raw -c "aio_write -P 1 0 512" \ > + -c "aio_write -P 2 512 512" -c aio_flush test-parallel-file.data || > { echo "'qemu-io' can't drive parallel requests"; exit 77; } > > # Set up the file plugin to delay both reads and writes (for a good chance > @@ -55,8 +56,8 @@ qemu-io -f raw -c "aio_write -P 1 0 512" -c "aio_write -P 2 512 512" \ > > # With --threads=1, the write should complete first because it was issued first > nbdkit -v -t 1 -U - --filter=delay file test-parallel-file.data \ > - wdelay=2 rdelay=1 --run 'qemu-io -f raw -c "aio_write -P 2 512 512" \ > - -c "aio_read -P 1 0 512" -c aio_flush $nbd' | > + wdelay=2 rdelay=1 --run 'timeout 10s </dev/null qemu-io -f raw \ > + -c "aio_write -P 2 512 512" -c "aio_read -P 1 0 512" -c aio_flush $nbd' | > tee test-parallel-file.out > if test "$(grep '512/512' test-parallel-file.out)" != \ > "wrote 512/512 bytes at offset 512 > @@ -66,8 +67,8 @@ fi > > # With default --threads, the faster read should complete first > nbdkit -v -U - --filter=delay file test-parallel-file.data \ > - wdelay=2 rdelay=1 --run 'qemu-io -f raw -c "aio_write -P 2 512 512" \ > - -c "aio_read -P 1 0 512" -c aio_flush $nbd' | > + wdelay=2 rdelay=1 --run 'timeout 10s </dev/null qemu-io -f raw \ > + -c "aio_write -P 2 512 512" -c "aio_read -P 1 0 512" -c aio_flush $nbd' | > tee test-parallel-file.out > if test "$(grep '512/512' test-parallel-file.out)" != \ > "read 512/512 bytes at offset 0 > @@ -77,8 +78,8 @@ fi > > # With --filter=noparallel, the write should complete first because it was issued first > nbdkit -v -U - --filter=noparallel --filter=delay file test-parallel-file.data \ > - wdelay=2 rdelay=1 --run 'qemu-io -f raw -c "aio_write -P 2 512 512" \ > - -c "aio_read -P 1 0 512" -c aio_flush $nbd' | > + wdelay=2 rdelay=1 --run 'timeout 10s </dev/null qemu-io -f raw \ > + -c "aio_write -P 2 512 512" -c "aio_read -P 1 0 512" -c aio_flush $nbd' | > tee test-parallel-file.out > if test "$(grep '512/512' test-parallel-file.out)" != \ > "wrote 512/512 bytes at offset 512 > diff --git a/tests/test-parallel-nbd.sh b/tests/test-parallel-nbd.sh > index 4e546df4..1fdf7df4 100755 > --- a/tests/test-parallel-nbd.sh > +++ b/tests/test-parallel-nbd.sh > @@ -1,6 +1,6 @@ > #!/usr/bin/env bash > # nbdkit > -# Copyright (C) 2017-2019 Red Hat Inc. > +# Copyright (C) 2017-2020 Red Hat Inc. > # > # Redistribution and use in source and binary forms, with or without > # modification, are permitted provided that the following conditions are > @@ -35,6 +35,7 @@ source ./functions.sh > # Check file-data was created by Makefile and qemu-io exists. > requires test -f file-data > requires qemu-io --version > +requires timeout --version > > nbdkit --dump-plugin nbd | grep -q ^thread_model=parallel || > { echo "nbdkit lacks support for parallel requests"; exit 77; } > @@ -46,8 +47,8 @@ cleanup_fn rm -f $files > > # Populate file, and sanity check that qemu-io can issue parallel requests > printf '%1024s' . > test-parallel-nbd.data > -qemu-io -f raw -c "aio_write -P 1 0 512" -c "aio_write -P 2 512 512" \ > - -c aio_flush test-parallel-nbd.data || > +timeout 10s </dev/null qemu-io -f raw -c "aio_write -P 1 0 512" \ > + -c "aio_write -P 2 512 512" -c aio_flush test-parallel-nbd.data || > { echo "'qemu-io' can't drive parallel requests"; exit 77; } > > # Set up the file plugin to delay both reads and writes (for a good chance > @@ -63,7 +64,8 @@ start_nbdkit -P test-parallel-nbd.pid \ > > # With --threads=1, the write should complete first because it was issued first > nbdkit -v -t 1 -U - nbd socket=$sock --run ' > - qemu-io -f raw -c "aio_write -P 2 512 512" -c "aio_read -P 1 0 512" \ > + timeout 10s </dev/null qemu-io -f raw \ > + -c "aio_write -P 2 512 512" -c "aio_read -P 1 0 512" \ > -c aio_flush $nbd' | tee test-parallel-nbd.out > if test "$(grep '512/512' test-parallel-nbd.out)" != \ > "wrote 512/512 bytes at offset 512 > @@ -73,7 +75,8 @@ fi > > # With default --threads, the faster read should complete first > nbdkit -v -U - nbd socket=$sock --run ' > - qemu-io -f raw -c "aio_write -P 2 512 512" -c "aio_read -P 1 0 512" \ > + timeout 10s </dev/null qemu-io -f raw \ > + -c "aio_write -P 2 512 512" -c "aio_read -P 1 0 512" \ > -c aio_flush $nbd' | tee test-parallel-nbd.out > if test "$(grep '512/512' test-parallel-nbd.out)" != \ > "read 512/512 bytes at offset 0 > diff --git a/tests/test-parallel-sh.sh b/tests/test-parallel-sh.sh > index 15d8875d..4cc93c33 100755 > --- a/tests/test-parallel-sh.sh > +++ b/tests/test-parallel-sh.sh > @@ -1,6 +1,6 @@ > #!/usr/bin/env bash > # nbdkit > -# Copyright (C) 2017-2019 Red Hat Inc. > +# Copyright (C) 2017-2020 Red Hat Inc. > # > # Redistribution and use in source and binary forms, with or without > # modification, are permitted provided that the following conditions are > @@ -35,6 +35,7 @@ source ./functions.sh > # Check file-data was created by Makefile and qemu-io exists. > requires test -f file-data > requires qemu-io --version > +requires timeout --version > > nbdkit --dump-plugin sh | grep -q ^thread_model=parallel || > { echo "nbdkit lacks support for parallel requests"; exit 77; } > @@ -43,8 +44,8 @@ cleanup_fn rm -f test-parallel-sh.data test-parallel-sh.out test-parallel-sh.scr > > # Populate file, and sanity check that qemu-io can issue parallel requests > printf '%1024s' . > test-parallel-sh.data > -qemu-io -f raw -c "aio_write -P 1 0 512" -c "aio_write -P 2 512 512" \ > - -c aio_flush test-parallel-sh.data || > +timeout 10s </dev/null qemu-io -f raw -c "aio_write -P 1 0 512" \ > + -c "aio_write -P 2 512 512" -c aio_flush test-parallel-sh.data || > { echo "'qemu-io' can't drive parallel requests"; exit 77; } > > # Set up the sh plugin to delay both reads and writes (for a good > @@ -96,8 +97,8 @@ chmod +x test-parallel-sh.script > > # With --threads=1, the write should complete first because it was issued first > nbdkit -v -t 1 -U - --filter=delay sh test-parallel-sh.script \ > - wdelay=2 rdelay=1 --run 'qemu-io -f raw -c "aio_write -P 2 512 512" \ > - -c "aio_read -P 1 0 512" -c aio_flush $nbd' | > + wdelay=2 rdelay=1 --run 'timeout 10s </dev/null qemu-io -f raw \ > + -c "aio_write -P 2 512 512" -c "aio_read -P 1 0 512" -c aio_flush $nbd' | > tee test-parallel-sh.out > if test "$(grep '512/512' test-parallel-sh.out)" != \ > "wrote 512/512 bytes at offset 512 > @@ -107,8 +108,8 @@ fi > > # With default --threads, the faster read should complete first > nbdkit -v -U - --filter=delay sh test-parallel-sh.script \ > - wdelay=2 rdelay=1 --run 'qemu-io -f raw -c "aio_write -P 2 512 512" \ > - -c "aio_read -P 1 0 512" -c aio_flush $nbd' | > + wdelay=2 rdelay=1 --run 'timeout 10s </dev/null qemu-io -f raw \ > + -c "aio_write -P 2 512 512" -c "aio_read -P 1 0 512" -c aio_flush $nbd' | > tee test-parallel-sh.out > if test "$(grep '512/512' test-parallel-sh.out)" != \ > "read 512/512 bytes at offset 0 > @@ -120,8 +121,8 @@ fi > # issued first. Also test that the log filter doesn't leak an fd > nbdkit -v -U - --filter=noparallel --filter=log --filter=delay \ > sh test-parallel-sh.script logfile=/dev/null \ > - wdelay=2 rdelay=1 --run 'qemu-io -f raw -c "aio_write -P 2 512 512" \ > - -c "aio_read -P 1 0 512" -c aio_flush $nbd' | > + wdelay=2 rdelay=1 --run 'timeout 10s </dev/null qemu-io -f raw \ > + -c "aio_write -P 2 512 512" -c "aio_read -P 1 0 512" -c aio_flush $nbd' | > tee test-parallel-sh.out > if test "$(grep '512/512' test-parallel-sh.out)" != \ > "wrote 512/512 bytes at offset 512 > -- > 2.25.1 > > _______________________________________________ > Libguestfs mailing list > Libguestfs@redhat.com > https://www.redhat.com/mailman/listinfo/libguestfs-- 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
Richard W.M. Jones
2020-Mar-17 08:19 UTC
Re: [Libguestfs] [nbdkit PATCH 4/4] RFC tests: Add test to cover unusual .can_flush return
On Mon, Mar 16, 2020 at 10:36:17PM -0500, Eric Blake wrote:> We want some testsuite coverage of handling non-1 .can_flush values. > The only in-tree plugin with this property is nbd-standalone, but it > doesn't get frequently tested, not to mention that the next patch will > change it to work with older nbdkit. > > Signed-off-by: Eric Blake <eblake@redhat.com> > --- > > work in progress: currently compiles but fails during test; posting it > now to show my thoughts before I go to bedYes, it all looks reasonable so far. Rich.> tests/Makefile.am | 21 +++++++ > tests/test-flush.sh | 85 +++++++++++++++++++++++++++ > tests/test-flush-plugin.c | 119 ++++++++++++++++++++++++++++++++++++++ > 3 files changed, 225 insertions(+) > create mode 100755 tests/test-flush.sh > create mode 100644 tests/test-flush-plugin.c > > diff --git a/tests/Makefile.am b/tests/Makefile.am > index 65dd148d..7e0024d3 100644 > --- a/tests/Makefile.am > +++ b/tests/Makefile.am > @@ -116,6 +116,7 @@ EXTRA_DIST = \ > test-extentlist.sh \ > test-file-extents.sh \ > test-floppy.sh \ > + test-flush.sh \ > test-foreground.sh \ > test-fua.sh \ > test-full.sh \ > @@ -267,6 +268,7 @@ TESTS += \ > test-foreground.sh \ > test-debug-flags.sh \ > test-long-name.sh \ > + test-flush.sh \ > test-swap.sh \ > test-shutdown.sh \ > $(NULL) > @@ -282,6 +284,25 @@ test_socket_activation_CPPFLAGS = \ > $(NULL) > test_socket_activation_CFLAGS = $(WARNINGS_CFLAGS) > > +# check_LTLIBRARIES won't build a shared library (see automake manual). > +# So we have to do this and add a dependency. > +noinst_LTLIBRARIES += \ > + test-flush-plugin.la \ > + $(NULL) > +test-flush.sh: test-flush-plugin.la > + > +test_flush_plugin_la_SOURCES = \ > + test-flush-plugin.c \ > + $(top_srcdir)/include/nbdkit-plugin.h \ > + $(NULL) > +test_flush_plugin_la_CPPFLAGS = -I$(top_srcdir)/include > +test_flush_plugin_la_CFLAGS = $(WARNINGS_CFLAGS) > +# For use of the -rpath option, see: > +# https://lists.gnu.org/archive/html/libtool/2007-07/msg00067.html > +test_flush_plugin_la_LDFLAGS = \ > + -module -avoid-version -shared -rpath /nowhere \ > + $(NULL) > + > # check_LTLIBRARIES won't build a shared library (see automake manual). > # So we have to do this and add a dependency. > noinst_LTLIBRARIES += \ > diff --git a/tests/test-flush.sh b/tests/test-flush.sh > new file mode 100755 > index 00000000..54801b3a > --- /dev/null > +++ b/tests/test-flush.sh > @@ -0,0 +1,85 @@ > +#!/usr/bin/env bash > +# nbdkit > +# Copyright (C) 2019-2020 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 -x > + > +requires nbdsh --version > + > +plugin=.libs/test-flush-plugin.so > +requires test -f $plugin > + > +# Test what happens when a plugin fails .can_flush > +nbdsh -c ' > +try: > + h.connect_command (["nbdkit", "-s", "-v", "'$plugin'", "level=-1"]) > +except nbd.Error as ex: > + exit (0) > +# If we got here, things are broken > +exit (1) > +' > + > +# A read-only connection never triggers .can_flush > +nbdsh -c ' > +h.connect_command (["nbdkit", "-s", "-r", "-v", "'$plugin'", "level=-1"]) > +assert h.is_read_only () == 1 > +assert h.can_flush () == 0 > +assert h.can_fua () == 0 > +' > + > +# Disable flush and FUA > +nbdsh -c ' > +h.connect_command (["nbdkit", "-s", "-v", "'$plugin'", "0"]) > +assert h.is_read_only () == 0 > +assert h.can_flush () == 0 > +assert h.can_fua () == 0 > +' > + > +# Normal flush, emulated FUA > +nbdsh -c ' > +h.connect_command (["nbdkit", "-s", "-v", "'$plugin'", "1"]) > +assert h.is_read_only () == 0 > +assert h.can_flush () == 1 > +assert h.can_fua () == 1 > +# try direct flush; check that output has "handling flush" > +# try write with FUA; check that output has "handling flush" > +' > + > +# Unusual return value for .can_flush, native FUA > +nbdsh -c ' > +h.connect_command (["nbdkit", "-s", "-v", "'$plugin'", "2"]) > +assert h.is_read_only () == 0 > +assert h.can_flush () == 1 > +assert h.can_fua () == 1 > +# try direct flush; check that output has "handling flush" > +# try write with FUA; check that output has "handling native FUA" > +' > diff --git a/tests/test-flush-plugin.c b/tests/test-flush-plugin.c > new file mode 100644 > index 00000000..5553c607 > --- /dev/null > +++ b/tests/test-flush-plugin.c > @@ -0,0 +1,119 @@ > +/* nbdkit > + * Copyright (C) 2013-2020 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. > + */ > + > +#include <config.h> > + > +#include <stdio.h> > +#include <stdlib.h> > +#include <string.h> > + > +#define NBDKIT_API_VERSION 2 > + > +#include <nbdkit-plugin.h> > + > +/* level abuses our knowledge of internal nbdkit values: > + * -1: force error during connect > + * 0: no flush, no FUA > + * 1: flush works, FUA is emulated > + * 2: flush works, FUA is native > + */ > +static int level; > + > +static int > +flush_config (const char *key, const char *value) > +{ > + if (strcmp (key, "level") == 0) > + return nbdkit_parse_int (key, value, &level); > + nbdkit_error ("unknown parameter '%s'", key); > + return -1; > +} > + > +/* Implements both .can_flush and .can_fua */ > +static int > +flush_level (void *handle) > +{ > + return level; > +} > + > +static void * > +flush_open (int readonly) > +{ > + return NBDKIT_HANDLE_NOT_NEEDED; > +} > + > +static int64_t > +flush_get_size (void *handle) > +{ > + return 1024*1024; > +} > + > +#define THREAD_MODEL NBDKIT_THREAD_MODEL_PARALLEL > + > +static int > +flush_pread (void *handle, void *buf, uint32_t count, uint64_t offset, > + uint32_t flags) > +{ > + memset (buf, 0, count); > + return 0; > +} > + > +static int > +flush_pwrite (void *handle, const void *buf, uint32_t count, uint64_t offset, > + uint32_t flags) > +{ > + if (flags & NBDKIT_FLAG_FUA) > + nbdkit_debug ("handling native FUA"); > + return 0; > +} > + > +static int > +flush_flush (void *handle, uint32_t flags) > +{ > + nbdkit_debug ("handling flush"); > + return 0; > +} > + > +static struct nbdkit_plugin plugin = { > + .name = "flush", > + .version = PACKAGE_VERSION, > + .config = flush_config, > + .magic_config_key = "level", > + .open = flush_open, > + .get_size = flush_get_size, > + .pread = flush_pread, > + .pwrite = flush_pwrite, > + .can_flush = flush_level, > + .can_fua = flush_level, > + .flush = flush_flush, > +}; > + > +NBDKIT_REGISTER_PLUGIN(plugin) > -- > 2.25.1 > > _______________________________________________ > Libguestfs mailing list > Libguestfs@redhat.com > https://www.redhat.com/mailman/listinfo/libguestfs-- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-top is 'top' for virtual machines. Tiny program with many powerful monitoring features, net stats, disk stats, logging, etc. http://people.redhat.com/~rjones/virt-top
Apparently Analagous Threads
- [nbdkit PATCH 0/2] more nbd tweaks
- [nbdkit PATCH] tests: Make parallel tests work at 512-byte granularity
- [PATCH nbdkit 0/2] tests: Minor reworking of tests.
- [PATCH nbdkit] tests: Remove QEMU_IO / HAVE_QEMU_IO.
- Re: [nbdkit PATCH v2 0/4] enable parallel nbd forwarding