Libguestfs - Jan 2018 - [PATCH nbdkit] tests: Remove QEMU_IO / HAVE_QEMU_IO.

Currently we test for qemu-img, socat, ss, certtool, etc at run time,
but we test for qemu-io at compile time (in ./configure).  This commit
removes this inconsistency.

I would consider the opposite patch (which makes qemu-img etc tested
at configure time).

The main advantage of testing for these binaries at run time is that
tests are not "silently" omitted.  Instead tests with insufficient
dependencies are now reported as ‘SKIP’, which I think gives packagers
a better overview of their coverage of the test suite.

I recently changed the Fedora build so we run the test suite on every
architecture, and it's interesting that x86_64 is now running and
passing 40 tests, but some architectures (s390x for instance) only
manage 17 passes with the rest being skipped for multiple reasons.

Disadvantages:

 - Can't override the location of these binaries eg by setting
   QEMU_IO=/opt/qemu/bin/qemu-io ./configure

 - ./configure doesn't report missing soft dependencies.

There are also other test dependencies that we test at compile time
but we cannot change that (guestfish, libguestfs).

Rich.

This is for consistency with qemu-img, socat, ss, etc where we test
for these binaries at run time.
---
 configure.ac                |  4 ----
 tests/Makefile.am           |  8 +++-----
 tests/test-parallel-file.sh | 21 +++++++++++++--------
 tests/test-parallel-nbd.sh  | 21 +++++++++++++--------
 4 files changed, 29 insertions(+), 25 deletions(-)

diff --git a/configure.ac b/configure.ac
index 6025ce0..30e4c43 100644
--- a/configure.ac
+++ b/configure.ac
@@ -407,10 +407,6 @@ dnl Check for guestfish (only needed for some of the
tests).
 AC_CHECK_PROG([GUESTFISH], [guestfish], [guestfish], [no])
 AM_CONDITIONAL([HAVE_GUESTFISH], [test "x$GUESTFISH" !=
"xno"])
 
-dnl Check for qemu-io (only needed for some of the tests).
-AC_CHECK_PROG([QEMU_IO], [qemu-io], [qemu-io], [no])
-AM_CONDITIONAL([HAVE_QEMU_IO], [test "x$QEMU_IO" != "xno"])
-
 dnl See plugins/vddk/README.VDDK.
 AC_CHECK_SIZEOF([size_t])
 AS_IF([test "x$ac_cv_sizeof_size_t" =
"x4"],[bits=32],[bits=64])
diff --git a/tests/Makefile.am b/tests/Makefile.am
index ec33109..9be57e6 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -178,11 +178,9 @@ file-data:
 
 # While most tests need libguestfs, testing parallel I/O is easier when
 # using qemu-io to kick off asynchronous requests.
-if HAVE_QEMU_IO
-TESTS_ENVIRONMENT += QEMU_IO=$(QEMU_IO)
-TESTS += test-parallel-file.sh
-TESTS += test-parallel-nbd.sh
-endif HAVE_QEMU_IO
+TESTS += \
+	test-parallel-file.sh \
+	test-parallel-nbd.sh
 
 # Most in-depth tests need libguestfs, since that is a convenient way to
 # drive qemu.
diff --git a/tests/test-parallel-file.sh b/tests/test-parallel-file.sh
index ed1f99c..695c53b 100755
--- a/tests/test-parallel-file.sh
+++ b/tests/test-parallel-file.sh
@@ -31,14 +31,19 @@
 # OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
 # SUCH DAMAGE.
 
-# Makefile sets $QEMU_IO and builds file-data, but it's also nice if the
-# script runs again standalone afterwards for diagnosing any failures
-test -f file-data || { echo "Missing file-data"; exit 77; }
-: ${QEMU_IO=qemu-io}
+# Check file-data was created by Makefile and qemu-io exists.
+if ! test -f file-data; then
+    echo "$0: missing file-data"
+    exit 77
+fi
+if ! qemu-io --version; then
+    echo "$0: missing qemu-io"
+    exit 77
+fi
 
 # Sanity check that qemu-io can issue parallel requests
-$QEMU_IO -f raw -c "aio_write -P 2 1 1" -c "aio_read -P 1 0
1" -c aio_flush \
-  file-data || { echo "'$QEMU_IO' can't drive parallel
requests"; exit 77; }
+qemu-io -f raw -c "aio_write -P 2 1 1" -c "aio_read -P 1 0
1" -c aio_flush \
+  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
 # that parallel requests are in flight), and with writes longer than reads
@@ -50,7 +55,7 @@ trap 'rm -f test-parallel-file.out' 0 1 2 3 15
 
 # With --threads=1, the write should complete first because it was issued first
 nbdkit -v -t 1 -U - --filter=delay file file=file-data wdelay=2 rdelay=1 --run
'
-  $QEMU_IO -f raw -c "aio_write -P 2 1 1" -c "aio_read -P 1 0
1" -c aio_flush $nbd
+  qemu-io -f raw -c "aio_write -P 2 1 1" -c "aio_read -P 1 0
1" -c aio_flush $nbd
 ' | tee test-parallel-file.out
 if test "$(grep '1/1' test-parallel-file.out)" != \
 "wrote 1/1 bytes at offset 1
@@ -60,7 +65,7 @@ fi
 
 # With default --threads, the faster read should complete first
 nbdkit -v -U - --filter=delay file file=file-data wdelay=2 rdelay=1 --run '
-  $QEMU_IO -f raw -c "aio_write -P 2 1 1" -c "aio_read -P 1 0
1" -c aio_flush $nbd
+  qemu-io -f raw -c "aio_write -P 2 1 1" -c "aio_read -P 1 0
1" -c aio_flush $nbd
 ' | tee test-parallel-file.out
 if test "$(grep '1/1' test-parallel-file.out)" != \
 "read 1/1 bytes at offset 0
diff --git a/tests/test-parallel-nbd.sh b/tests/test-parallel-nbd.sh
index 244faf6..eda45df 100755
--- a/tests/test-parallel-nbd.sh
+++ b/tests/test-parallel-nbd.sh
@@ -31,14 +31,19 @@
 # OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
 # SUCH DAMAGE.
 
-# Makefile sets $QEMU_IO and builds file-data, but it's also nice if the
-# script runs again standalone afterwards for diagnosing any failures
-test -f file-data || { echo "Missing file-data"; exit 77; }
-: ${QEMU_IO=qemu-io}
+# Check file-data was created by Makefile and qemu-io exists.
+if ! test -f file-data; then
+    echo "$0: missing file-data"
+    exit 77
+fi
+if ! qemu-io --version; then
+    echo "$0: missing qemu-io"
+    exit 77
+fi
 
 # Sanity check that qemu-io can issue parallel requests
-$QEMU_IO -f raw -c "aio_write -P 2 1 1" -c "aio_read -P 1 0
1" -c aio_flush \
-  file-data || { echo "'$QEMU_IO' can't drive parallel
requests"; exit 77; }
+qemu-io -f raw -c "aio_write -P 2 1 1" -c "aio_read -P 1 0
1" -c aio_flush \
+  file-data || { echo "'qemu-io' can't drive parallel
requests"; exit 77; }
 
 # We require --exit-with-parent to work
 ( nbdkit --exit-with-parent --help ) >/dev/null 2>&1 ||
@@ -59,7 +64,7 @@ nbdkit --exit-with-parent -v -U test-parallel-nbd.sock \
 
 # With --threads=1, the write should complete first because it was issued first
 nbdkit -v -t 1 -U - nbd socket=test-parallel-nbd.sock --run '
-  $QEMU_IO -f raw -c "aio_write -P 2 1 1" -c "aio_read -P 1 0
1" -c aio_flush $nbd
+  qemu-io -f raw -c "aio_write -P 2 1 1" -c "aio_read -P 1 0
1" -c aio_flush $nbd
 ' | tee test-parallel-nbd.out
 if test "$(grep '1/1' test-parallel-nbd.out)" != \
 "wrote 1/1 bytes at offset 1
@@ -69,7 +74,7 @@ fi
 
 # With default --threads, the faster read should complete first
 nbdkit -v -U - nbd socket=test-parallel-nbd.sock --run '
-  $QEMU_IO -f raw -c "aio_write -P 2 1 1" -c "aio_read -P 1 0
1" -c aio_flush $nbd
+  qemu-io -f raw -c "aio_write -P 2 1 1" -c "aio_read -P 1 0
1" -c aio_flush $nbd
 ' | tee test-parallel-nbd.out
 if test "$(grep '1/1' test-parallel-nbd.out)" != \
 "read 1/1 bytes at offset 0
-- 
2.13.2

On 01/27/2018 05:41 AM, Richard W.M. Jones wrote:
> Currently we test for qemu-img, socat, ss, certtool, etc at run time,
> but we test for qemu-io at compile time (in ./configure).  This commit
> removes this inconsistency.
> 
> I would consider the opposite patch (which makes qemu-img etc tested
> at configure time).
> 
> The main advantage of testing for these binaries at run time is that
> tests are not "silently" omitted.  Instead tests with
insufficient
> dependencies are now reported as ‘SKIP’, which I think gives packagers
> a better overview of their coverage of the test suite.
> 
> I recently changed the Fedora build so we run the test suite on every
> architecture, and it's interesting that x86_64 is now running and
> passing 40 tests, but some architectures (s390x for instance) only
> manage 17 passes with the rest being skipped for multiple reasons.
> 
> Disadvantages:
> 
>  - Can't override the location of these binaries eg by setting
>    QEMU_IO=/opt/qemu/bin/qemu-io ./configure
We could still do things like:

: ${QEMU_IO=qemu-io}

at the start of tests, which lets the user set $QEMU_IO as an override,
but defaults to calling the first one on the path otherwise; as long as
we still do runtime detection of whether $QEMU_IO behaves like the test
expects, that would be reasonable.
> 
>  - ./configure doesn't report missing soft dependencies.
Even if it reported them, it wouldn't change the exit status, so you'd
still have to read the logs to see why things were skipped.

The idea seems reasonable to me.


-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

On 01/27/2018 05:41 AM, Richard W.M. Jones wrote:
> This is for consistency with qemu-img, socat, ss, etc where we test
> for these binaries at run time.
> ---
> +++ b/tests/Makefile.am
> @@ -178,11 +178,9 @@ file-data:
>  
>  # While most tests need libguestfs, testing parallel I/O is easier when
>  # using qemu-io to kick off asynchronous requests.
Is this comment still necessary, given that...
> -if HAVE_QEMU_IO
> -TESTS_ENVIRONMENT += QEMU_IO=$(QEMU_IO)
> -TESTS += test-parallel-file.sh
> -TESTS += test-parallel-nbd.sh
> -endif HAVE_QEMU_IO
> +TESTS += \
> +	test-parallel-file.sh \
> +	test-parallel-nbd.sh
...you are removing the conditional?
> +++ b/tests/test-parallel-file.sh
> @@ -31,14 +31,19 @@
>  # OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
>  # SUCH DAMAGE.
>  
> -# Makefile sets $QEMU_IO and builds file-data, but it's also nice if
the
> -# script runs again standalone afterwards for diagnosing any failures
> -test -f file-data || { echo "Missing file-data"; exit 77; }
> -: ${QEMU_IO=qemu-io}
This still makes sense for user overrides, even if configure.ac/Makefile
doesn't set it.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org