thr3ads.net - Libguestfs - Re: [Libguestfs] [nbdkit PATCH 2/4] nbd: Normalize return values of can

If this information is useful, please help other people find it:
Share via:

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

head link

[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

head link

[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

head link

[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

head link

[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

head link

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

head link

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

head link

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 int
While 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

head link

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

head link

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 bed
Yes, 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

Reasonably Related Threads

Search for more possibly parallel threads

Libguestfs - Mar 2020 - Re: [nbdkit PATCH 2/4] nbd: Normalize return values of can_*

[Libguestfs] [nbdkit PATCH 0/4] Fix testsuite hang with nbd-stadalone

[Libguestfs] [nbdkit PATCH 1/4] server: Normalize plugin can_* values

[Libguestfs] [nbdkit PATCH 2/4] nbd: Normalize return values of can_*

[Libguestfs] [nbdkit PATCH 3/4] tests: Don't let test-parallel-* hang on nbdkit bug

[Libguestfs] [nbdkit PATCH 4/4] RFC tests: Add test to cover unusual .can_flush return

Re: [Libguestfs] [nbdkit PATCH 0/4] Fix testsuite hang with nbd-stadalone

Re: [Libguestfs] [nbdkit PATCH 1/4] server: Normalize plugin can_* values

Re: [Libguestfs] [nbdkit PATCH 2/4] nbd: Normalize return values of can_*

Re: [Libguestfs] [nbdkit PATCH 3/4] tests: Don't let test-parallel-* hang on nbdkit bug

Re: [Libguestfs] [nbdkit PATCH 4/4] RFC tests: Add test to cover unusual .can_flush return

Reasonably Related Threads