[PATCH 0/6] Improve integration of iotests in the meson test harness

[PATCH 0/6] Improve integration of iotests in the meson test harness

[Thomas Huth]

Though "make check-block" is currently already run via the meson test
runner, it still looks like an oddball in the output of "make check" since
the tests are still run separately via the check-block.sh script. It would
be nicer if the iotests would show up like the other tests suites. For this
we have to tweak the tests/qemu-iotests/check script so that it can already
be run with "-g auto -n" during the configuration step [*], then we can
directly add the individual tests in the tests/qemu-iotests/meson.build file
already and finally get rid of the check-block.sh script.

[*] Alternatively, I think we could also get rid of the "auto" group
and add the test list to the tests/qemu-iotests/meson.build file
directly ... not sure whether that's so much nicer, though.

Thomas Huth (6):
  tests/qemu-iotests: Improve the check for GNU sed
  tests/qemu-iotests/meson.build: Improve the indentation
  tests/qemu-iotests: Allow to run "./check -n" from the source
    directory, too
  tests/qemu-iotests/meson.build: Call the 'check' script directly
  tests: Do not treat the iotests as separate meson test target anymore
  tests: Remove check-block.sh

 meson.build                    |  6 +--
 scripts/mtest2make.py          |  4 --
 tests/Makefile.include         |  9 +---
 tests/check-block.sh           | 85 ----------------------------------
 tests/qemu-iotests/check       | 52 ++++++++++++---------
 tests/qemu-iotests/common.rc   | 26 +++++------
 tests/qemu-iotests/meson.build | 84 ++++++++++++++++++++++-----------
 7 files changed, 104 insertions(+), 162 deletions(-)
 delete mode 100755 tests/check-block.sh

-- 
2.27.0

[PATCH 1/6] tests/qemu-iotests: Improve the check for GNU sed

[Thomas Huth]

Instead of failing the iotests if GNU sed is not available (or skipping
them completely in the check-block.sh script), it would be better to
simply skip the bash-based tests, so that the python-based tests could
still be run. Thus add the check for BusyBox sed to common.rc and mark
the tests as "not run" if GNU sed is not available. Then we can also
remove the sed checks from the check-block.sh script.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 tests/check-block.sh         | 12 ------------
 tests/qemu-iotests/common.rc | 26 +++++++++++++-------------
 2 files changed, 13 insertions(+), 25 deletions(-)

diff --git a/tests/check-block.sh b/tests/check-block.sh
index 720a46bc36..af0c574812 100755
--- a/tests/check-block.sh
+++ b/tests/check-block.sh
@@ -52,18 +52,6 @@ if LANG=C bash --version | grep -q 'GNU bash, version [123]' ; then
     skip "bash version too old ==> Not running the qemu-iotests."
 fi
 
-if ! (sed --version | grep 'GNU sed') > /dev/null 2>&1 ; then
-    if ! command -v gsed >/dev/null 2>&1; then
-        skip "GNU sed not available ==> Not running the qemu-iotests."
-    fi
-else
-    # Double-check that we're not using BusyBox' sed which says
-    # that "This is not GNU sed version 4.0" ...
-    if sed --version | grep -q 'not GNU sed' ; then
-        skip "BusyBox sed not supported ==> Not running the qemu-iotests."
-    fi
-fi
-
 cd tests/qemu-iotests
 
 # QEMU_CHECK_BLOCK_AUTO is used to disable some unstable sub-tests
diff --git a/tests/qemu-iotests/common.rc b/tests/qemu-iotests/common.rc
index 9885030b43..9ea504810c 100644
--- a/tests/qemu-iotests/common.rc
+++ b/tests/qemu-iotests/common.rc
@@ -17,17 +17,27 @@
 # along with this program.  If not, see <http://www.gnu.org/licenses/>.
 #
 
+# bail out, setting up .notrun file
+_notrun()
+{
+    echo "$*" >"$OUTPUT_DIR/$seq.notrun"
+    echo "$seq not run: $*"
+    status=0
+    exit
+}
+
+# We need GNU sed for the iotests. Make sure to not use BusyBox sed
+# which says that "This is not GNU sed version 4.0"
 SED=
 for sed in sed gsed; do
-    ($sed --version | grep 'GNU sed') > /dev/null 2>&1
+    ($sed --version | grep -v "not GNU sed" | grep 'GNU sed') > /dev/null 2>&1
     if [ "$?" -eq 0 ]; then
         SED=$sed
         break
     fi
 done
 if [ -z "$SED" ]; then
-    echo "$0: GNU sed not found"
-    exit 1
+    _notrun "GNU sed not found"
 fi
 
 dd()
@@ -722,16 +732,6 @@ _img_info()
         done
 }
 
-# bail out, setting up .notrun file
-#
-_notrun()
-{
-    echo "$*" >"$OUTPUT_DIR/$seq.notrun"
-    echo "$seq not run: $*"
-    status=0
-    exit
-}
-
 # bail out, setting up .casenotrun file
 # The function _casenotrun() is used as a notifier. It is the
 # caller's responsibility to make skipped a particular test.
-- 
2.27.0

[PATCH 2/6] tests/qemu-iotests/meson.build: Improve the indentation

[Thomas Huth]

By using subdir_done(), we can get rid of one level of indentation
in this file. This will make it easier to add more conditions to
skip the iotests in future patches.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 tests/qemu-iotests/meson.build | 61 ++++++++++++++++++----------------
 1 file changed, 32 insertions(+), 29 deletions(-)

diff --git a/tests/qemu-iotests/meson.build b/tests/qemu-iotests/meson.build
index 5be3c74127..e1832c90e0 100644
--- a/tests/qemu-iotests/meson.build
+++ b/tests/qemu-iotests/meson.build
@@ -1,30 +1,33 @@
-if have_tools and targetos != 'windows'
-  qemu_iotests_binaries = [qemu_img, qemu_io, qemu_nbd, qsd]
-  qemu_iotests_env = {'PYTHON': python.full_path()}
-  qemu_iotests_formats = {
-    'qcow2': 'quick',
-    'raw': 'slow',
-    'qed': 'thorough',
-    'vmdk': 'thorough',
-    'vpc': 'thorough'
-  }
-
-  foreach k, v : emulators
-    if k.startswith('qemu-system-')
-      qemu_iotests_binaries += v
-    endif
-  endforeach
-  foreach format, speed: qemu_iotests_formats
-    if speed == 'quick'
-      suites = 'block'
-    else
-      suites = ['block-' + speed, speed]
-    endif
-    test('qemu-iotests ' + format, sh, args: [files('../check-block.sh'), format],
-         depends: qemu_iotests_binaries, env: qemu_iotests_env,
-         protocol: 'tap',
-         suite: suites,
-         timeout: 0,
-         is_parallel: false)
-  endforeach
+if not have_tools or targetos == 'windows'
+  subdir_done()
 endif
+
+qemu_iotests_binaries = [qemu_img, qemu_io, qemu_nbd, qsd]
+qemu_iotests_env = {'PYTHON': python.full_path()}
+qemu_iotests_formats = {
+  'qcow2': 'quick',
+  'raw': 'slow',
+  'qed': 'thorough',
+  'vmdk': 'thorough',
+  'vpc': 'thorough'
+}
+
+foreach k, v : emulators
+  if k.startswith('qemu-system-')
+    qemu_iotests_binaries += v
+  endif
+endforeach
+
+foreach format, speed: qemu_iotests_formats
+  if speed == 'quick'
+    suites = 'block'
+  else
+    suites = ['block-' + speed, speed]
+  endif
+  test('qemu-iotests ' + format, sh, args: [files('../check-block.sh'), format],
+       depends: qemu_iotests_binaries, env: qemu_iotests_env,
+       protocol: 'tap',
+       suite: suites,
+       timeout: 0,
+       is_parallel: false)
+endforeach
-- 
2.27.0

[PATCH 3/6] tests/qemu-iotests: Allow to run "./check -n" from the source directory, too

[Thomas Huth]

For better integration of the iotests into the meson build system, it
would be very helpful to get the list of the tests in the "auto" group
during the "configure" step already. However, "check -n -g auto"
currently only works if the binaries have already been built. Re-order
the code in the "check" a little bit so that we can use the -n option
without building the binaries first.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 tests/qemu-iotests/check | 52 ++++++++++++++++++++++------------------
 1 file changed, 29 insertions(+), 23 deletions(-)

diff --git a/tests/qemu-iotests/check b/tests/qemu-iotests/check
index 75de1b4691..0fa75abf13 100755
--- a/tests/qemu-iotests/check
+++ b/tests/qemu-iotests/check
@@ -120,6 +120,30 @@ def make_argparser() -> argparse.ArgumentParser:
 if __name__ == '__main__':
     args = make_argparser().parse_args()
 
+    if os.path.islink(sys.argv[0]):
+        # called from the build tree
+        source_iotests = os.path.dirname(os.readlink(sys.argv[0]))
+    else:
+        source_iotests = os.getcwd()
+
+    testfinder = TestFinder(source_iotests)
+
+    groups = args.groups.split(',') if args.groups else None
+    x_groups = args.exclude_groups.split(',') if args.exclude_groups else None
+
+    try:
+        tests = testfinder.find_tests(groups=groups, exclude_groups=x_groups,
+                                      tests=args.tests,
+                                      start_from=args.start_from)
+        if not tests:
+            raise ValueError('No tests selected')
+    except ValueError as e:
+        sys.exit(e)
+
+    if args.dry_run:
+        print('\n'.join(tests))
+        sys.exit(0)
+
     env = TestEnv(imgfmt=args.imgfmt, imgproto=args.imgproto,
                   aiomode=args.aiomode, cachemode=args.cachemode,
                   imgopts=args.imgopts, misalign=args.misalign,
@@ -140,11 +164,6 @@ if __name__ == '__main__':
         os.chdir(exec_path.parent)
         os.execve(cmd[0], cmd, full_env)
 
-    testfinder = TestFinder(test_dir=env.source_iotests)
-
-    groups = args.groups.split(',') if args.groups else None
-    x_groups = args.exclude_groups.split(',') if args.exclude_groups else None
-
     group_local = os.path.join(env.source_iotests, 'group.local')
     if os.path.isfile(group_local):
         try:
@@ -152,21 +171,8 @@ if __name__ == '__main__':
         except ValueError as e:
             sys.exit(f"Failed to parse group file '{group_local}': {e}")
 
-    try:
-        tests = testfinder.find_tests(groups=groups, exclude_groups=x_groups,
-                                      tests=args.tests,
-                                      start_from=args.start_from)
-        if not tests:
-            raise ValueError('No tests selected')
-    except ValueError as e:
-        sys.exit(e)
-
-    if args.dry_run:
-        print('\n'.join(tests))
-    else:
-        with TestRunner(env, tap=args.tap,
-                        color=args.color) as tr:
-            paths = [os.path.join(env.source_iotests, t) for t in tests]
-            ok = tr.run_tests(paths, args.jobs)
-            if not ok:
-                sys.exit(1)
+    with TestRunner(env, tap=args.tap, color=args.color) as tr:
+        paths = [os.path.join(env.source_iotests, t) for t in tests]
+        ok = tr.run_tests(paths, args.jobs)
+        if not ok:
+            sys.exit(1)
-- 
2.27.0

[PATCH 4/6] tests/qemu-iotests/meson.build: Call the 'check' script directly

[Thomas Huth]

We can get a nicer progress indication if we add the iotests
individually via the 'check' script instead of going through
the check-block.sh wrapper.

For this, we have to add some of the sanity checks that have
originally been done in the tests/check-block.sh script (whether
"bash" is available or whether CFLAGS contain -fsanitize switches)
to the meson.build file now, and add the environment variables
that have been set up by the tests/check-block.sh script before.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 tests/qemu-iotests/meson.build | 45 ++++++++++++++++++++++++++++------
 1 file changed, 37 insertions(+), 8 deletions(-)

diff --git a/tests/qemu-iotests/meson.build b/tests/qemu-iotests/meson.build
index e1832c90e0..5a6ccd35d8 100644
--- a/tests/qemu-iotests/meson.build
+++ b/tests/qemu-iotests/meson.build
@@ -1,9 +1,29 @@
-if not have_tools or targetos == 'windows'
+if not have_tools or targetos == 'windows' or \
+   config_host.has_key('CONFIG_GPROF')
   subdir_done()
 endif
 
+bash = find_program('bash', required: false)
+if not bash.found() or \
+   run_command(bash, ['--version']).stdout().contains('GNU bash, version 3')
+  message('bash >= v4.0 not available ==> Disabled the qemu-iotests.')
+  subdir_done()
+endif
+
+foreach cflag: config_host['QEMU_CFLAGS'].split()
+  if cflag.startswith('-fsanitize') and \
+     not cflag.contains('safe-stack') and not cflag.contains('cfi-icall')
+    message('Sanitizers are enabled ==> Disabled the qemu-iotests.')
+    subdir_done()
+  endif
+endforeach
+
 qemu_iotests_binaries = [qemu_img, qemu_io, qemu_nbd, qsd]
-qemu_iotests_env = {'PYTHON': python.full_path()}
+qemu_iotests_env = {
+  'PYTHON': python.full_path(),
+  'PYTHONUTF8': '1',
+  'QEMU_CHECK_BLOCK_AUTO': '1'
+}
 qemu_iotests_formats = {
   'qcow2': 'quick',
   'raw': 'slow',
@@ -18,16 +38,25 @@ foreach k, v : emulators
   endif
 endforeach
 
+check_script = find_program(meson.current_build_dir() / 'check')
+iotests = run_command(python, [check_script.full_path(), '-g', 'auto', '-n'],
+                      check: true).stdout().strip().replace('tests/', '').split('\n')
+
 foreach format, speed: qemu_iotests_formats
   if speed == 'quick'
     suites = 'block'
   else
     suites = ['block-' + speed, speed]
   endif
-  test('qemu-iotests ' + format, sh, args: [files('../check-block.sh'), format],
-       depends: qemu_iotests_binaries, env: qemu_iotests_env,
-       protocol: 'tap',
-       suite: suites,
-       timeout: 0,
-       is_parallel: false)
+  foreach tst: iotests
+    test('iotest-' + format + '-' + tst,
+         python, args: [check_script.full_path(), '-tap', '-' + format, tst],
+         depends: qemu_iotests_binaries,
+         env: qemu_iotests_env + \
+              { 'TEST_DIR':
+                meson.current_build_dir() / 'scratch' / format + '-' + tst },
+         protocol: 'tap',
+         suite: suites,
+         timeout: 0)
+  endforeach
 endforeach
-- 
2.27.0

[PATCH 5/6] tests: Do not treat the iotests as separate meson test target anymore

[Thomas Huth]

Now that we add the single iotests directly in meson.build, we do
not have to separate the block suite from the other suites anymore.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 meson.build            | 6 +++---
 scripts/mtest2make.py  | 4 ----
 tests/Makefile.include | 9 +--------
 3 files changed, 4 insertions(+), 15 deletions(-)

diff --git a/meson.build b/meson.build
index 5f43355071..b203402ee1 100644
--- a/meson.build
+++ b/meson.build
@@ -3,9 +3,9 @@ project('qemu', ['c'], meson_version: '>=0.58.2',
                           'b_staticpic=false', 'stdsplit=false'],
         version: files('VERSION'))
 
-add_test_setup('quick', exclude_suites: ['block', 'slow', 'thorough'], is_default: true)
-add_test_setup('slow', exclude_suites: ['block', 'thorough'], env: ['G_TEST_SLOW=1', 'SPEED=slow'])
-add_test_setup('thorough', exclude_suites: ['block'], env: ['G_TEST_SLOW=1', 'SPEED=thorough'])
+add_test_setup('quick', exclude_suites: ['slow', 'thorough'], is_default: true)
+add_test_setup('slow', exclude_suites: ['thorough'], env: ['G_TEST_SLOW=1', 'SPEED=slow'])
+add_test_setup('thorough', env: ['G_TEST_SLOW=1', 'SPEED=thorough'])
 
 not_found = dependency('', required: false)
 keyval = import('keyval')
diff --git a/scripts/mtest2make.py b/scripts/mtest2make.py
index 4d542e8aaa..304634b71e 100644
--- a/scripts/mtest2make.py
+++ b/scripts/mtest2make.py
@@ -101,10 +101,6 @@ def emit_suite(name, suite, prefix):
 testsuites = defaultdict(Suite)
 for test in introspect['tests']:
     process_tests(test, targets, testsuites)
-# HACK: check-block is a separate target so that it runs with --verbose;
-# only write the dependencies
-emit_suite_deps('block', testsuites['block'], 'check')
-del testsuites['block']
 emit_prolog(testsuites, 'check')
 for name, suite in testsuites.items():
     emit_suite(name, suite, 'check')
diff --git a/tests/Makefile.include b/tests/Makefile.include
index 9157a57b1a..f93ae5b479 100644
--- a/tests/Makefile.include
+++ b/tests/Makefile.include
@@ -151,16 +151,9 @@ check-acceptance: check-acceptance-deprecated-warning | check-avocado
 
 # Consolidated targets
 
-.PHONY: check-block check check-clean get-vm-images
+.PHONY: check check-clean get-vm-images
 check:
 
-ifeq ($(CONFIG_TOOLS)$(CONFIG_POSIX),yy)
-check: check-block
-check-block: run-ninja
-	$(if $(MAKE.n),,+)$(MESON) test $(MTESTARGS) $(.mtestargs) --verbose \
-		--logbase iotestslog $(call .speed.$(SPEED), block block-slow block-thorough)
-endif
-
 check-build: run-ninja
 
 check-clean:
-- 
2.27.0

[PATCH 6/6] tests: Remove check-block.sh

[Thomas Huth]

Now that the iotests are added by the meson.build file already,
we do not need the check-block.sh wrapper script anymore.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 tests/check-block.sh | 73 --------------------------------------------
 1 file changed, 73 deletions(-)
 delete mode 100755 tests/check-block.sh

diff --git a/tests/check-block.sh b/tests/check-block.sh
deleted file mode 100755
index af0c574812..0000000000
--- a/tests/check-block.sh
+++ /dev/null
@@ -1,73 +0,0 @@
-#!/bin/sh
-
-if [ "$#" -eq 0 ]; then
-    echo "Usage: $0 fmt..." >&2
-    exit 99
-fi
-
-# Honor the SPEED environment variable, just like we do it for "meson test"
-format_list="$@"
-if [ "$SPEED" = "slow" ] || [ "$SPEED" = "thorough" ]; then
-    group=
-else
-    group="-g auto"
-fi
-
-skip() {
-    echo "1..0 #SKIP $*"
-    exit 0
-}
-
-if grep -q "CONFIG_GPROF=y" config-host.mak 2>/dev/null ; then
-    skip "GPROF is enabled ==> Not running the qemu-iotests."
-fi
-
-# Disable tests with any sanitizer except for specific ones
-SANITIZE_FLAGS=$( grep "CFLAGS.*-fsanitize" config-host.mak 2>/dev/null )
-ALLOWED_SANITIZE_FLAGS="safe-stack cfi-icall"
-#Remove all occurrencies of allowed Sanitize flags
-for j in ${ALLOWED_SANITIZE_FLAGS}; do
-    TMP_FLAGS=${SANITIZE_FLAGS}
-    SANITIZE_FLAGS=""
-    for i in ${TMP_FLAGS}; do
-        if ! echo ${i} | grep -q "${j}" 2>/dev/null; then
-            SANITIZE_FLAGS="${SANITIZE_FLAGS} ${i}"
-        fi
-    done
-done
-if echo ${SANITIZE_FLAGS} | grep -q "\-fsanitize" 2>/dev/null; then
-    # Have a sanitize flag that is not allowed, stop
-    skip "Sanitizers are enabled ==> Not running the qemu-iotests."
-fi
-
-if [ -z "$(find . -name 'qemu-system-*' -print)" ]; then
-    skip "No qemu-system binary available ==> Not running the qemu-iotests."
-fi
-
-if ! command -v bash >/dev/null 2>&1 ; then
-    skip "bash not available ==> Not running the qemu-iotests."
-fi
-
-if LANG=C bash --version | grep -q 'GNU bash, version [123]' ; then
-    skip "bash version too old ==> Not running the qemu-iotests."
-fi
-
-cd tests/qemu-iotests
-
-# QEMU_CHECK_BLOCK_AUTO is used to disable some unstable sub-tests
-export QEMU_CHECK_BLOCK_AUTO=1
-export PYTHONUTF8=1
-# If make was called with -jN we want to call ./check with -j N. Extract the
-# flag from MAKEFLAGS, so that if it absent (or MAKEFLAGS is not defined), JOBS
-# would be an empty line otherwise JOBS is prepared string of flag with value:
-# "-j N"
-# Note, that the following works even if make was called with "-j N" or even
-# "--jobs N", as all these variants becomes simply "-jN" in MAKEFLAGS variable.
-JOBS=$(echo "$MAKEFLAGS" | sed -n 's/\(^\|.* \)-j\([0-9]\+\)\( .*\|$\)/-j \2/p')
-
-ret=0
-for fmt in $format_list ; do
-    ${PYTHON} ./check $JOBS -tap -$fmt $group || ret=1
-done
-
-exit $ret
-- 
2.27.0

Re: [PATCH 5/6] tests: Do not treat the iotests as separate meson test target anymore

[Peter Maydell]

On Tue, 8 Feb 2022 at 10:18, Thomas Huth <thuth@redhat.com> wrote:
>
> Now that we add the single iotests directly in meson.build, we do
> not have to separate the block suite from the other suites anymore.
>
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  meson.build            | 6 +++---
>  scripts/mtest2make.py  | 4 ----
>  tests/Makefile.include | 9 +--------
>  3 files changed, 4 insertions(+), 15 deletions(-)
>
> diff --git a/meson.build b/meson.build
> index 5f43355071..b203402ee1 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -3,9 +3,9 @@ project('qemu', ['c'], meson_version: '>=0.58.2',
>                            'b_staticpic=false', 'stdsplit=false'],
>          version: files('VERSION'))
>
> -add_test_setup('quick', exclude_suites: ['block', 'slow', 'thorough'], is_default: true)
> -add_test_setup('slow', exclude_suites: ['block', 'thorough'], env: ['G_TEST_SLOW=1', 'SPEED=slow'])
> -add_test_setup('thorough', exclude_suites: ['block'], env: ['G_TEST_SLOW=1', 'SPEED=thorough'])
> +add_test_setup('quick', exclude_suites: ['slow', 'thorough'], is_default: true)
> +add_test_setup('slow', exclude_suites: ['thorough'], env: ['G_TEST_SLOW=1', 'SPEED=slow'])
> +add_test_setup('thorough', env: ['G_TEST_SLOW=1', 'SPEED=thorough'])
>
>  not_found = dependency('', required: false)
>  keyval = import('keyval')
> diff --git a/scripts/mtest2make.py b/scripts/mtest2make.py
> index 4d542e8aaa..304634b71e 100644
> --- a/scripts/mtest2make.py
> +++ b/scripts/mtest2make.py
> @@ -101,10 +101,6 @@ def emit_suite(name, suite, prefix):
>  testsuites = defaultdict(Suite)
>  for test in introspect['tests']:
>      process_tests(test, targets, testsuites)
> -# HACK: check-block is a separate target so that it runs with --verbose;
> -# only write the dependencies
> -emit_suite_deps('block', testsuites['block'], 'check')
> -del testsuites['block']

This code being deleted claims to be doing something to ensure that
the tests get run and output the useful messages on failure.
What is the mechanism for this in the new meson setup ?
(As far as I can tell at the moment this is broken. At some
point I will start agitating for reverting that conversion if
it isn't fixed :-))

-- PMM

Re: [PATCH 2/6] tests/qemu-iotests/meson.build: Improve the indentation

[Philippe Mathieu-Daudé via]

On 8/2/22 11:13, Thomas Huth wrote:
> By using subdir_done(), we can get rid of one level of indentation
> in this file. This will make it easier to add more conditions to
> skip the iotests in future patches.
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>   tests/qemu-iotests/meson.build | 61 ++++++++++++++++++----------------
>   1 file changed, 32 insertions(+), 29 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Re: [PATCH 5/6] tests: Do not treat the iotests as separate meson test target anymore

[Thomas Huth]

On 08/02/2022 11.26, Peter Maydell wrote:
> On Tue, 8 Feb 2022 at 10:18, Thomas Huth <thuth@redhat.com> wrote:
>>
>> Now that we add the single iotests directly in meson.build, we do
>> not have to separate the block suite from the other suites anymore.
>>
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>> ---
>>   meson.build            | 6 +++---
>>   scripts/mtest2make.py  | 4 ----
>>   tests/Makefile.include | 9 +--------
>>   3 files changed, 4 insertions(+), 15 deletions(-)
>>
>> diff --git a/meson.build b/meson.build
>> index 5f43355071..b203402ee1 100644
>> --- a/meson.build
>> +++ b/meson.build
>> @@ -3,9 +3,9 @@ project('qemu', ['c'], meson_version: '>=0.58.2',
>>                             'b_staticpic=false', 'stdsplit=false'],
>>           version: files('VERSION'))
>>
>> -add_test_setup('quick', exclude_suites: ['block', 'slow', 'thorough'], is_default: true)
>> -add_test_setup('slow', exclude_suites: ['block', 'thorough'], env: ['G_TEST_SLOW=1', 'SPEED=slow'])
>> -add_test_setup('thorough', exclude_suites: ['block'], env: ['G_TEST_SLOW=1', 'SPEED=thorough'])
>> +add_test_setup('quick', exclude_suites: ['slow', 'thorough'], is_default: true)
>> +add_test_setup('slow', exclude_suites: ['thorough'], env: ['G_TEST_SLOW=1', 'SPEED=slow'])
>> +add_test_setup('thorough', env: ['G_TEST_SLOW=1', 'SPEED=thorough'])
>>
>>   not_found = dependency('', required: false)
>>   keyval = import('keyval')
>> diff --git a/scripts/mtest2make.py b/scripts/mtest2make.py
>> index 4d542e8aaa..304634b71e 100644
>> --- a/scripts/mtest2make.py
>> +++ b/scripts/mtest2make.py
>> @@ -101,10 +101,6 @@ def emit_suite(name, suite, prefix):
>>   testsuites = defaultdict(Suite)
>>   for test in introspect['tests']:
>>       process_tests(test, targets, testsuites)
>> -# HACK: check-block is a separate target so that it runs with --verbose;
>> -# only write the dependencies
>> -emit_suite_deps('block', testsuites['block'], 'check')
>> -del testsuites['block']
> 
> This code being deleted claims to be doing something to ensure that
> the tests get run and output the useful messages on failure.

No, AFAIK that --verbose switch just influences how meson prints the 
progress during the test runs (i.e. either a brief or a slightly more 
verbose output).

> What is the mechanism for this in the new meson setup ?

cat meson-logs/testlog.txt

... I guess we should either dump that to stdout or publish that file as a 
test artifact?

  Thomas

Re: [PATCH 5/6] tests: Do not treat the iotests as separate meson test target anymore

[Peter Maydell]

On Tue, 8 Feb 2022 at 11:16, Thomas Huth <thuth@redhat.com> wrote:
>
> On 08/02/2022 11.26, Peter Maydell wrote:
> > What is the mechanism for this in the new meson setup ?
>
> cat meson-logs/testlog.txt
>
> ... I guess we should either dump that to stdout

Yes, it needs to actually appear in the stdout for CI jobs,
otherwise it is inaccessible and might as well not exist.
V=1 is the switch we have for "be verbose", and meson's
test facility should honour it.

thanks
-- PMM

Re: [PATCH 1/6] tests/qemu-iotests: Improve the check for GNU sed

[Hanna Reitz]

On 08.02.22 11:13, Thomas Huth wrote:
> Instead of failing the iotests if GNU sed is not available (or skipping
> them completely in the check-block.sh script), it would be better to
> simply skip the bash-based tests, so that the python-based tests could
> still be run. Thus add the check for BusyBox sed to common.rc and mark
> the tests as "not run" if GNU sed is not available. Then we can also
> remove the sed checks from the check-block.sh script.
>
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>   tests/check-block.sh         | 12 ------------
>   tests/qemu-iotests/common.rc | 26 +++++++++++++-------------
>   2 files changed, 13 insertions(+), 25 deletions(-)
>
> diff --git a/tests/check-block.sh b/tests/check-block.sh
> index 720a46bc36..af0c574812 100755
> --- a/tests/check-block.sh
> +++ b/tests/check-block.sh
> @@ -52,18 +52,6 @@ if LANG=C bash --version | grep -q 'GNU bash, version [123]' ; then
>       skip "bash version too old ==> Not running the qemu-iotests."
>   fi
>   
> -if ! (sed --version | grep 'GNU sed') > /dev/null 2>&1 ; then

This specifically tests for `sed`, whereas...

[...]

> diff --git a/tests/qemu-iotests/common.rc b/tests/qemu-iotests/common.rc
> index 9885030b43..9ea504810c 100644
> --- a/tests/qemu-iotests/common.rc
> +++ b/tests/qemu-iotests/common.rc
> @@ -17,17 +17,27 @@
>   # along with this program.  If not, see <http://www.gnu.org/licenses/>.
>   #
>   
> +# bail out, setting up .notrun file
> +_notrun()
> +{
> +    echo "$*" >"$OUTPUT_DIR/$seq.notrun"
> +    echo "$seq not run: $*"
> +    status=0
> +    exit
> +}
> +
> +# We need GNU sed for the iotests. Make sure to not use BusyBox sed
> +# which says that "This is not GNU sed version 4.0"
>   SED=
>   for sed in sed gsed; do
> -    ($sed --version | grep 'GNU sed') > /dev/null 2>&1
> +    ($sed --version | grep -v "not GNU sed" | grep 'GNU sed') > /dev/null 2>&1

...this will accept `gsed`, too.  The problem is that many bash iotests 
just use `sed` instead of `$SED`, so I think if we let this do the 
gatekeeping, then we should change this to just check for `sed`.

Hanna

>       if [ "$?" -eq 0 ]; then
>           SED=$sed
>           break
>       fi
>   done
>   if [ -z "$SED" ]; then
> -    echo "$0: GNU sed not found"
> -    exit 1
> +    _notrun "GNU sed not found"
>   fi

Re: [PATCH 2/6] tests/qemu-iotests/meson.build: Improve the indentation

[Hanna Reitz]

On 08.02.22 11:13, Thomas Huth wrote:
> By using subdir_done(), we can get rid of one level of indentation
> in this file. This will make it easier to add more conditions to
> skip the iotests in future patches.
>
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>   tests/qemu-iotests/meson.build | 61 ++++++++++++++++++----------------
>   1 file changed, 32 insertions(+), 29 deletions(-)

Reviewed-by: Hanna Reitz <hreitz@redhat.com>

Re: [PATCH 1/6] tests/qemu-iotests: Improve the check for GNU sed

[Thomas Huth]

On 08/02/2022 12.46, Hanna Reitz wrote:
> On 08.02.22 11:13, Thomas Huth wrote:
>> Instead of failing the iotests if GNU sed is not available (or skipping
>> them completely in the check-block.sh script), it would be better to
>> simply skip the bash-based tests, so that the python-based tests could
>> still be run. Thus add the check for BusyBox sed to common.rc and mark
>> the tests as "not run" if GNU sed is not available. Then we can also
>> remove the sed checks from the check-block.sh script.
>>
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>> ---
>>   tests/check-block.sh         | 12 ------------
>>   tests/qemu-iotests/common.rc | 26 +++++++++++++-------------
>>   2 files changed, 13 insertions(+), 25 deletions(-)
>>
>> diff --git a/tests/check-block.sh b/tests/check-block.sh
>> index 720a46bc36..af0c574812 100755
>> --- a/tests/check-block.sh
>> +++ b/tests/check-block.sh
>> @@ -52,18 +52,6 @@ if LANG=C bash --version | grep -q 'GNU bash, version 
>> [123]' ; then
>>       skip "bash version too old ==> Not running the qemu-iotests."
>>   fi
>> -if ! (sed --version | grep 'GNU sed') > /dev/null 2>&1 ; then
> 
> This specifically tests for `sed`, whereas...

There was a check for "gsed" one line later:

  if ! command -v gsed >/dev/null 2>&1; then

... so the check-block.sh script ran the iotests also if "sed" was not GNU, 
but gsed was available.

> [...]
> 
>> diff --git a/tests/qemu-iotests/common.rc b/tests/qemu-iotests/common.rc
>> index 9885030b43..9ea504810c 100644
>> --- a/tests/qemu-iotests/common.rc
>> +++ b/tests/qemu-iotests/common.rc
>> @@ -17,17 +17,27 @@
>>   # along with this program.  If not, see <http://www.gnu.org/licenses/>.
>>   #
>> +# bail out, setting up .notrun file
>> +_notrun()
>> +{
>> +    echo "$*" >"$OUTPUT_DIR/$seq.notrun"
>> +    echo "$seq not run: $*"
>> +    status=0
>> +    exit
>> +}
>> +
>> +# We need GNU sed for the iotests. Make sure to not use BusyBox sed
>> +# which says that "This is not GNU sed version 4.0"
>>   SED=
>>   for sed in sed gsed; do
>> -    ($sed --version | grep 'GNU sed') > /dev/null 2>&1
>> +    ($sed --version | grep -v "not GNU sed" | grep 'GNU sed') > /dev/null 
>> 2>&1
> 
> ...this will accept `gsed`, too.  The problem is that many bash iotests just 
> use `sed` instead of `$SED`, so I think if we let this do the gatekeeping, 
> then we should change this to just check for `sed`.

I think we should be fine - at least for the tests in the "auto" group. 
Otherwise we would have seen test failures on non-Linux systems like *BSD 
earlier already.

  Thomas

Re: [PATCH 3/6] tests/qemu-iotests: Allow to run "./check -n" from the source directory, too

[Hanna Reitz]

On 08.02.22 11:13, Thomas Huth wrote:
> For better integration of the iotests into the meson build system, it
> would be very helpful to get the list of the tests in the "auto" group
> during the "configure" step already. However, "check -n -g auto"
> currently only works if the binaries have already been built. Re-order
> the code in the "check" a little bit so that we can use the -n option
> without building the binaries first.
>
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>   tests/qemu-iotests/check | 52 ++++++++++++++++++++++------------------
>   1 file changed, 29 insertions(+), 23 deletions(-)

I don’t love how this patch completely separates dry_run from running 
the tests, and how finding source_iotests is replicated from 
testenv.py.  For the latter, we should at least assert somewhere that 
source_iotests == env.source_iotests.

Wouldn’t it instead be possible to pass args.dry_run to TestEnv and have 
it just skip all build-dir-related stuff, which I think is just the 
self.init_binaries() call (and perhaps the self.qemu_prog check in the 
`for suffix, machine in machine_map` loop)?

Hanna

Re: [PATCH 1/6] tests/qemu-iotests: Improve the check for GNU sed

[Hanna Reitz]

On 08.02.22 13:13, Thomas Huth wrote:
> On 08/02/2022 12.46, Hanna Reitz wrote:
>> On 08.02.22 11:13, Thomas Huth wrote:
>>> Instead of failing the iotests if GNU sed is not available (or skipping
>>> them completely in the check-block.sh script), it would be better to
>>> simply skip the bash-based tests, so that the python-based tests could
>>> still be run. Thus add the check for BusyBox sed to common.rc and mark
>>> the tests as "not run" if GNU sed is not available. Then we can also
>>> remove the sed checks from the check-block.sh script.
>>>
>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>>> ---
>>>   tests/check-block.sh         | 12 ------------
>>>   tests/qemu-iotests/common.rc | 26 +++++++++++++-------------
>>>   2 files changed, 13 insertions(+), 25 deletions(-)
>>>
>>> diff --git a/tests/check-block.sh b/tests/check-block.sh
>>> index 720a46bc36..af0c574812 100755
>>> --- a/tests/check-block.sh
>>> +++ b/tests/check-block.sh
>>> @@ -52,18 +52,6 @@ if LANG=C bash --version | grep -q 'GNU bash, 
>>> version [123]' ; then
>>>       skip "bash version too old ==> Not running the qemu-iotests."
>>>   fi
>>> -if ! (sed --version | grep 'GNU sed') > /dev/null 2>&1 ; then
>>
>> This specifically tests for `sed`, whereas...
>
> There was a check for "gsed" one line later:
>
>  if ! command -v gsed >/dev/null 2>&1; then
>
> ... so the check-block.sh script ran the iotests also if "sed" was not 
> GNU, but gsed was available.

Oh, right.

>> [...]
>>
>>> diff --git a/tests/qemu-iotests/common.rc 
>>> b/tests/qemu-iotests/common.rc
>>> index 9885030b43..9ea504810c 100644
>>> --- a/tests/qemu-iotests/common.rc
>>> +++ b/tests/qemu-iotests/common.rc
>>> @@ -17,17 +17,27 @@
>>>   # along with this program.  If not, see 
>>> <http://www.gnu.org/licenses/>.
>>>   #
>>> +# bail out, setting up .notrun file
>>> +_notrun()
>>> +{
>>> +    echo "$*" >"$OUTPUT_DIR/$seq.notrun"
>>> +    echo "$seq not run: $*"
>>> +    status=0
>>> +    exit
>>> +}
>>> +
>>> +# We need GNU sed for the iotests. Make sure to not use BusyBox sed
>>> +# which says that "This is not GNU sed version 4.0"
>>>   SED=
>>>   for sed in sed gsed; do
>>> -    ($sed --version | grep 'GNU sed') > /dev/null 2>&1
>>> +    ($sed --version | grep -v "not GNU sed" | grep 'GNU sed') > 
>>> /dev/null 2>&1
>>
>> ...this will accept `gsed`, too.  The problem is that many bash 
>> iotests just use `sed` instead of `$SED`, so I think if we let this 
>> do the gatekeeping, then we should change this to just check for `sed`.
>
> I think we should be fine - at least for the tests in the "auto" 
> group. Otherwise we would have seen test failures on non-Linux systems 
> like *BSD earlier already.

Makes sense, but I’m quite uncomfortable with this.  Can’t we just do 
`alias sed=gsed`?

Hanna

Re: [PATCH 4/6] tests/qemu-iotests/meson.build: Call the 'check' script directly

[Paolo Bonzini]

On 2/8/22 11:13, Thomas Huth wrote:
> We can get a nicer progress indication if we add the iotests
> individually via the 'check' script instead of going through
> the check-block.sh wrapper.
> 
> For this, we have to add some of the sanity checks that have
> originally been done in the tests/check-block.sh script (whether
> "bash" is available or whether CFLAGS contain -fsanitize switches)
> to the meson.build file now, and add the environment variables
> that have been set up by the tests/check-block.sh script before.
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>   tests/qemu-iotests/meson.build | 45 ++++++++++++++++++++++++++++------
>   1 file changed, 37 insertions(+), 8 deletions(-)
> 
> diff --git a/tests/qemu-iotests/meson.build b/tests/qemu-iotests/meson.build
> index e1832c90e0..5a6ccd35d8 100644
> --- a/tests/qemu-iotests/meson.build
> +++ b/tests/qemu-iotests/meson.build
> @@ -1,9 +1,29 @@
> -if not have_tools or targetos == 'windows'
> +if not have_tools or targetos == 'windows' or \
> +   config_host.has_key('CONFIG_GPROF')
>     subdir_done()
>   endif
>   
> +bash = find_program('bash', required: false)
> +if not bash.found() or \
> +   run_command(bash, ['--version']).stdout().contains('GNU bash, version 3')
> +  message('bash >= v4.0 not available ==> Disabled the qemu-iotests.')
> +  subdir_done()
> +endif
> +
> +foreach cflag: config_host['QEMU_CFLAGS'].split()
> +  if cflag.startswith('-fsanitize') and \
> +     not cflag.contains('safe-stack') and not cflag.contains('cfi-icall')
> +    message('Sanitizers are enabled ==> Disabled the qemu-iotests.')
> +    subdir_done()
> +  endif
> +endforeach
> +
>   qemu_iotests_binaries = [qemu_img, qemu_io, qemu_nbd, qsd]
> -qemu_iotests_env = {'PYTHON': python.full_path()}
> +qemu_iotests_env = {
> +  'PYTHON': python.full_path(),
> +  'PYTHONUTF8': '1',
> +  'QEMU_CHECK_BLOCK_AUTO': '1'
> +}
>   qemu_iotests_formats = {
>     'qcow2': 'quick',
>     'raw': 'slow',
> @@ -18,16 +38,25 @@ foreach k, v : emulators
>     endif
>   endforeach
>   
> +check_script = find_program(meson.current_build_dir() / 'check')
> +iotests = run_command(python, [check_script.full_path(), '-g', 'auto', '-n'],
> +                      check: true).stdout().strip().replace('tests/', '').split('\n')

The main disadvantage here is that changes to the test directory will 
not rebuild meson.build.  So when one creates a new test, meson has to 
be rerun manually.

It is probably possible to do something like:

- run "check -g auto -n" with configure_file() and store the output in a 
file

- in the Makefile, always run "check -g auto -n", like

check-block-test-list: check-block-test-list.stamp
check-block-test-list.stamp:
	./check -g auto -n > check-block-test-list.stamp; \
	if cmp check-block-test-list.stamp check-block-test-list; then \
		cp check-block-test-list.stamp check-block-test-list; \
	fi
.PHONY: check-block-test-list.stamp

check check-block: check-block-test-list.stamp

before giving control to "meson test"; but I'm not sure if that will 
also cause a rebuild of Makefile.mtest and a meson rerun.

But I'm not sure it's worth it...

Regarding the issue that Peter pointed out, that's my fault for not 
being aware of the "diff" being in the output of failing tests.  There 
are three ways to fix it:

1) reverting my patches

2) Thomas's series, but only if the above issue is fixed

3) shipping the tests/qemu-iotests/*.bad files as artifacts

4) not using -tap (either reverting commit d316859f4e, "check-block: 
replace -makecheck with TAP output", or just removing -tap from 
tests/qemu-iotests/meson.build).

My preference is 4, then 1 or 2 (depending on the complexity), then 3.

Paolo

>   foreach format, speed: qemu_iotests_formats
>     if speed == 'quick'
>       suites = 'block'
>     else
>       suites = ['block-' + speed, speed]
>     endif
> -  test('qemu-iotests ' + format, sh, args: [files('../check-block.sh'), format],
> -       depends: qemu_iotests_binaries, env: qemu_iotests_env,
> -       protocol: 'tap',
> -       suite: suites,
> -       timeout: 0,
> -       is_parallel: false)
> +  foreach tst: iotests
> +    test('iotest-' + format + '-' + tst,
> +         python, args: [check_script.full_path(), '-tap', '-' + format, tst],
> +         depends: qemu_iotests_binaries,
> +         env: qemu_iotests_env + \
> +              { 'TEST_DIR':
> +                meson.current_build_dir() / 'scratch' / format + '-' + tst },
> +         protocol: 'tap',
> +         suite: suites,
> +         timeout: 0)
> +  endforeach
>   endforeach

Re: [PATCH 5/6] tests: Do not treat the iotests as separate meson test target anymore

[Paolo Bonzini]

On 2/8/22 12:16, Thomas Huth wrote:
>>
>> This code being deleted claims to be doing something to ensure that
>> the tests get run and output the useful messages on failure.
> 
> No, AFAIK that --verbose switch just influences how meson prints the 
> progress during the test runs (i.e. either a brief or a slightly more 
> verbose output).

I replied to another patch, but anyway here is Peter's report: 
https://patchew.org/QEMU/20220128101513.646792-1-pbonzini@redhat.com/#CAFEAcA_ttmvA0emS-41R5+k3w_KAbFvC30qdAShJfr7U+3q=CA@mail.gmail.com 
(I had not seen it until now).

Paolo

Re: [PATCH 1/6] tests/qemu-iotests: Improve the check for GNU sed

[Thomas Huth]

On 08/02/2022 13.28, Hanna Reitz wrote:
> On 08.02.22 13:13, Thomas Huth wrote:
>> On 08/02/2022 12.46, Hanna Reitz wrote:
>>> On 08.02.22 11:13, Thomas Huth wrote:
>>>> Instead of failing the iotests if GNU sed is not available (or skipping
>>>> them completely in the check-block.sh script), it would be better to
>>>> simply skip the bash-based tests, so that the python-based tests could
>>>> still be run. Thus add the check for BusyBox sed to common.rc and mark
>>>> the tests as "not run" if GNU sed is not available. Then we can also
>>>> remove the sed checks from the check-block.sh script.
>>>>
>>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>>>> ---
>>>>   tests/check-block.sh         | 12 ------------
>>>>   tests/qemu-iotests/common.rc | 26 +++++++++++++-------------
>>>>   2 files changed, 13 insertions(+), 25 deletions(-)
>>>>
>>>> diff --git a/tests/check-block.sh b/tests/check-block.sh
>>>> index 720a46bc36..af0c574812 100755
>>>> --- a/tests/check-block.sh
>>>> +++ b/tests/check-block.sh
>>>> @@ -52,18 +52,6 @@ if LANG=C bash --version | grep -q 'GNU bash, version 
>>>> [123]' ; then
>>>>       skip "bash version too old ==> Not running the qemu-iotests."
>>>>   fi
>>>> -if ! (sed --version | grep 'GNU sed') > /dev/null 2>&1 ; then
>>>
>>> This specifically tests for `sed`, whereas...
>>
>> There was a check for "gsed" one line later:
>>
>>  if ! command -v gsed >/dev/null 2>&1; then
>>
>> ... so the check-block.sh script ran the iotests also if "sed" was not 
>> GNU, but gsed was available.
> 
> Oh, right.
> 
>>> [...]
>>>
>>>> diff --git a/tests/qemu-iotests/common.rc b/tests/qemu-iotests/common.rc
>>>> index 9885030b43..9ea504810c 100644
>>>> --- a/tests/qemu-iotests/common.rc
>>>> +++ b/tests/qemu-iotests/common.rc
>>>> @@ -17,17 +17,27 @@
>>>>   # along with this program.  If not, see <http://www.gnu.org/licenses/>.
>>>>   #
>>>> +# bail out, setting up .notrun file
>>>> +_notrun()
>>>> +{
>>>> +    echo "$*" >"$OUTPUT_DIR/$seq.notrun"
>>>> +    echo "$seq not run: $*"
>>>> +    status=0
>>>> +    exit
>>>> +}
>>>> +
>>>> +# We need GNU sed for the iotests. Make sure to not use BusyBox sed
>>>> +# which says that "This is not GNU sed version 4.0"
>>>>   SED=
>>>>   for sed in sed gsed; do
>>>> -    ($sed --version | grep 'GNU sed') > /dev/null 2>&1
>>>> +    ($sed --version | grep -v "not GNU sed" | grep 'GNU sed') > 
>>>> /dev/null 2>&1
>>>
>>> ...this will accept `gsed`, too.  The problem is that many bash iotests 
>>> just use `sed` instead of `$SED`, so I think if we let this do the 
>>> gatekeeping, then we should change this to just check for `sed`.
>>
>> I think we should be fine - at least for the tests in the "auto" group. 
>> Otherwise we would have seen test failures on non-Linux systems like *BSD 
>> earlier already.
> 
> Makes sense, but I’m quite uncomfortable with this.

The current code with $SED has been introduced almost three years ago already...

>  Can’t we just do `alias sed=gsed`?

Maybe ... but let's ask Philippe and Kevin first, who Signed-off commit 
bde36af1ab4f476 that introduced the current way with $SED: What's your 
opinion about this?

  Thomas

Re: [PATCH 1/6] tests/qemu-iotests: Improve the check for GNU sed

[Philippe Mathieu-Daudé via]

On 8/2/22 13:38, Thomas Huth wrote:
> On 08/02/2022 13.28, Hanna Reitz wrote:
>> On 08.02.22 13:13, Thomas Huth wrote:
>>> On 08/02/2022 12.46, Hanna Reitz wrote:
>>>> On 08.02.22 11:13, Thomas Huth wrote:
>>>>> Instead of failing the iotests if GNU sed is not available (or 
>>>>> skipping
>>>>> them completely in the check-block.sh script), it would be better to
>>>>> simply skip the bash-based tests, so that the python-based tests could
>>>>> still be run. Thus add the check for BusyBox sed to common.rc and mark
>>>>> the tests as "not run" if GNU sed is not available. Then we can also
>>>>> remove the sed checks from the check-block.sh script.
>>>>>
>>>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>>>>> ---
>>>>>   tests/check-block.sh         | 12 ------------
>>>>>   tests/qemu-iotests/common.rc | 26 +++++++++++++-------------
>>>>>   2 files changed, 13 insertions(+), 25 deletions(-)
>>>>>
>>>>> diff --git a/tests/check-block.sh b/tests/check-block.sh
>>>>> index 720a46bc36..af0c574812 100755
>>>>> --- a/tests/check-block.sh
>>>>> +++ b/tests/check-block.sh
>>>>> @@ -52,18 +52,6 @@ if LANG=C bash --version | grep -q 'GNU bash, 
>>>>> version [123]' ; then
>>>>>       skip "bash version too old ==> Not running the qemu-iotests."
>>>>>   fi
>>>>> -if ! (sed --version | grep 'GNU sed') > /dev/null 2>&1 ; then
>>>>
>>>> This specifically tests for `sed`, whereas...
>>>
>>> There was a check for "gsed" one line later:
>>>
>>>  if ! command -v gsed >/dev/null 2>&1; then
>>>
>>> ... so the check-block.sh script ran the iotests also if "sed" was 
>>> not GNU, but gsed was available.
>>
>> Oh, right.
>>
>>>> [...]
>>>>
>>>>> diff --git a/tests/qemu-iotests/common.rc 
>>>>> b/tests/qemu-iotests/common.rc
>>>>> index 9885030b43..9ea504810c 100644
>>>>> --- a/tests/qemu-iotests/common.rc
>>>>> +++ b/tests/qemu-iotests/common.rc
>>>>> @@ -17,17 +17,27 @@
>>>>>   # along with this program.  If not, see 
>>>>> <http://www.gnu.org/licenses/>.
>>>>>   #
>>>>> +# bail out, setting up .notrun file
>>>>> +_notrun()
>>>>> +{
>>>>> +    echo "$*" >"$OUTPUT_DIR/$seq.notrun"
>>>>> +    echo "$seq not run: $*"
>>>>> +    status=0
>>>>> +    exit
>>>>> +}
>>>>> +
>>>>> +# We need GNU sed for the iotests. Make sure to not use BusyBox sed
>>>>> +# which says that "This is not GNU sed version 4.0"
>>>>>   SED=
>>>>>   for sed in sed gsed; do
>>>>> -    ($sed --version | grep 'GNU sed') > /dev/null 2>&1
>>>>> +    ($sed --version | grep -v "not GNU sed" | grep 'GNU sed') > 
>>>>> /dev/null 2>&1
>>>>
>>>> ...this will accept `gsed`, too.  The problem is that many bash 
>>>> iotests just use `sed` instead of `$SED`, so I think if we let this 
>>>> do the gatekeeping, then we should change this to just check for `sed`.
>>>
>>> I think we should be fine - at least for the tests in the "auto" 
>>> group. Otherwise we would have seen test failures on non-Linux 
>>> systems like *BSD earlier already.
>>
>> Makes sense, but I’m quite uncomfortable with this.
> 
> The current code with $SED has been introduced almost three years ago 
> already...
> 
>>   Can’t we just do `alias sed=gsed`?
> 
> Maybe ... but let's ask Philippe and Kevin first, who Signed-off commit 
> bde36af1ab4f476 that introduced the current way with $SED: What's your 
> opinion about this?

This commit was to have check-block working on the OpenBSD VM image.

Re: [PATCH 4/6] tests/qemu-iotests/meson.build: Call the 'check' script directly

[Hanna Reitz]

On 08.02.22 11:13, Thomas Huth wrote:
> We can get a nicer progress indication if we add the iotests
> individually via the 'check' script instead of going through
> the check-block.sh wrapper.
>
> For this, we have to add some of the sanity checks that have
> originally been done in the tests/check-block.sh script (whether
> "bash" is available or whether CFLAGS contain -fsanitize switches)
> to the meson.build file now, and add the environment variables
> that have been set up by the tests/check-block.sh script before.
>
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>   tests/qemu-iotests/meson.build | 45 ++++++++++++++++++++++++++++------
>   1 file changed, 37 insertions(+), 8 deletions(-)
>
> diff --git a/tests/qemu-iotests/meson.build b/tests/qemu-iotests/meson.build
> index e1832c90e0..5a6ccd35d8 100644
> --- a/tests/qemu-iotests/meson.build
> +++ b/tests/qemu-iotests/meson.build
> @@ -1,9 +1,29 @@
> -if not have_tools or targetos == 'windows'
> +if not have_tools or targetos == 'windows' or \
> +   config_host.has_key('CONFIG_GPROF')
>     subdir_done()
>   endif
>   
> +bash = find_program('bash', required: false)
> +if not bash.found() or \
> +   run_command(bash, ['--version']).stdout().contains('GNU bash, version 3')

Instead of me asking about where the LANG=C is, or me lamenting that we 
could test very simply for [123] before and can no longer now... Can we 
not just do `find_program('bash', required: false, version: '>= 4.0')`?

> +  message('bash >= v4.0 not available ==> Disabled the qemu-iotests.')
> +  subdir_done()
> +endif
> +
> +foreach cflag: config_host['QEMU_CFLAGS'].split()
> +  if cflag.startswith('-fsanitize') and \
> +     not cflag.contains('safe-stack') and not cflag.contains('cfi-icall')
> +    message('Sanitizers are enabled ==> Disabled the qemu-iotests.')
> +    subdir_done()
> +  endif
> +endforeach
> +
>   qemu_iotests_binaries = [qemu_img, qemu_io, qemu_nbd, qsd]
> -qemu_iotests_env = {'PYTHON': python.full_path()}
> +qemu_iotests_env = {
> +  'PYTHON': python.full_path(),
> +  'PYTHONUTF8': '1',
> +  'QEMU_CHECK_BLOCK_AUTO': '1'
> +}
>   qemu_iotests_formats = {
>     'qcow2': 'quick',
>     'raw': 'slow',
> @@ -18,16 +38,25 @@ foreach k, v : emulators
>     endif
>   endforeach
>   
> +check_script = find_program(meson.current_build_dir() / 'check')
> +iotests = run_command(python, [check_script.full_path(), '-g', 'auto', '-n'],
> +                      check: true).stdout().strip().replace('tests/', '').split('\n')
> +
>   foreach format, speed: qemu_iotests_formats
>     if speed == 'quick'
>       suites = 'block'
>     else
>       suites = ['block-' + speed, speed]
>     endif
> -  test('qemu-iotests ' + format, sh, args: [files('../check-block.sh'), format],
> -       depends: qemu_iotests_binaries, env: qemu_iotests_env,
> -       protocol: 'tap',
> -       suite: suites,
> -       timeout: 0,
> -       is_parallel: false)
> +  foreach tst: iotests
> +    test('iotest-' + format + '-' + tst,
> +         python, args: [check_script.full_path(), '-tap', '-' + format, tst],
> +         depends: qemu_iotests_binaries,
> +         env: qemu_iotests_env + \
> +              { 'TEST_DIR':
> +                meson.current_build_dir() / 'scratch' / format + '-' + tst },
> +         protocol: 'tap',
> +         suite: suites,
> +         timeout: 0)

So as far I understand you’d like to have meson run the iotests in 
parallel this way.  I don’t actually think that’s safely possible for 
multiple formats at once, because a test’s output is always written into 
`${build_dir}/tests/qemu-iotests/${seq}.out.bad`; so if you run e.g. 
test 001 both with raw and qcow2 simultaneously, then they can get in 
each other’s way.

(In my test branch, I have 
https://gitlab.com/hreitz/qemu/-/commit/f3110b1eeb93d02aeadc5c8b807594cfa10a6aad 
for this – maybe I should send something like this in a more refined 
form to the list some time...)

As a minor note, the `check` script has recently received a `-j` 
argument for parallel execution.  Kind of a shame that we wouldn’t be 
able to use it here, but that’s how it is, I suppose.

Hanna

Re: [PATCH 1/6] tests/qemu-iotests: Improve the check for GNU sed

[Thomas Huth]

On 08/02/2022 14.11, Philippe Mathieu-Daudé wrote:
> On 8/2/22 13:38, Thomas Huth wrote:
>> On 08/02/2022 13.28, Hanna Reitz wrote:
>>> On 08.02.22 13:13, Thomas Huth wrote:
>>>> On 08/02/2022 12.46, Hanna Reitz wrote:
>>>>> On 08.02.22 11:13, Thomas Huth wrote:
>>>>>> Instead of failing the iotests if GNU sed is not available (or skipping
>>>>>> them completely in the check-block.sh script), it would be better to
>>>>>> simply skip the bash-based tests, so that the python-based tests could
>>>>>> still be run. Thus add the check for BusyBox sed to common.rc and mark
>>>>>> the tests as "not run" if GNU sed is not available. Then we can also
>>>>>> remove the sed checks from the check-block.sh script.
>>>>>>
>>>>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>>>>>> ---
>>>>>>   tests/check-block.sh         | 12 ------------
>>>>>>   tests/qemu-iotests/common.rc | 26 +++++++++++++-------------
>>>>>>   2 files changed, 13 insertions(+), 25 deletions(-)
>>>>>>
>>>>>> diff --git a/tests/check-block.sh b/tests/check-block.sh
>>>>>> index 720a46bc36..af0c574812 100755
>>>>>> --- a/tests/check-block.sh
>>>>>> +++ b/tests/check-block.sh
>>>>>> @@ -52,18 +52,6 @@ if LANG=C bash --version | grep -q 'GNU bash, 
>>>>>> version [123]' ; then
>>>>>>       skip "bash version too old ==> Not running the qemu-iotests."
>>>>>>   fi
>>>>>> -if ! (sed --version | grep 'GNU sed') > /dev/null 2>&1 ; then
>>>>>
>>>>> This specifically tests for `sed`, whereas...
>>>>
>>>> There was a check for "gsed" one line later:
>>>>
>>>>  if ! command -v gsed >/dev/null 2>&1; then
>>>>
>>>> ... so the check-block.sh script ran the iotests also if "sed" was not 
>>>> GNU, but gsed was available.
>>>
>>> Oh, right.
>>>
>>>>> [...]
>>>>>
>>>>>> diff --git a/tests/qemu-iotests/common.rc b/tests/qemu-iotests/common.rc
>>>>>> index 9885030b43..9ea504810c 100644
>>>>>> --- a/tests/qemu-iotests/common.rc
>>>>>> +++ b/tests/qemu-iotests/common.rc
>>>>>> @@ -17,17 +17,27 @@
>>>>>>   # along with this program.  If not, see <http://www.gnu.org/licenses/>.
>>>>>>   #
>>>>>> +# bail out, setting up .notrun file
>>>>>> +_notrun()
>>>>>> +{
>>>>>> +    echo "$*" >"$OUTPUT_DIR/$seq.notrun"
>>>>>> +    echo "$seq not run: $*"
>>>>>> +    status=0
>>>>>> +    exit
>>>>>> +}
>>>>>> +
>>>>>> +# We need GNU sed for the iotests. Make sure to not use BusyBox sed
>>>>>> +# which says that "This is not GNU sed version 4.0"
>>>>>>   SED=
>>>>>>   for sed in sed gsed; do
>>>>>> -    ($sed --version | grep 'GNU sed') > /dev/null 2>&1
>>>>>> +    ($sed --version | grep -v "not GNU sed" | grep 'GNU sed') > 
>>>>>> /dev/null 2>&1
>>>>>
>>>>> ...this will accept `gsed`, too.  The problem is that many bash iotests 
>>>>> just use `sed` instead of `$SED`, so I think if we let this do the 
>>>>> gatekeeping, then we should change this to just check for `sed`.
>>>>
>>>> I think we should be fine - at least for the tests in the "auto" group. 
>>>> Otherwise we would have seen test failures on non-Linux systems like 
>>>> *BSD earlier already.
>>>
>>> Makes sense, but I’m quite uncomfortable with this.
>>
>> The current code with $SED has been introduced almost three years ago 
>> already...
>>
>>>   Can’t we just do `alias sed=gsed`?
>>
>> Maybe ... but let's ask Philippe and Kevin first, who Signed-off commit 
>> bde36af1ab4f476 that introduced the current way with $SED: What's your 
>> opinion about this?
> 
> This commit was to have check-block working on the OpenBSD VM image.

Sure. The question was whether using an alias as suggested by Hanna would be 
nicer instead of using $SED ?

  Thomas

Re: [PATCH 4/6] tests/qemu-iotests/meson.build: Call the 'check' script directly

[Thomas Huth]

On 08/02/2022 13.36, Paolo Bonzini wrote:
> On 2/8/22 11:13, Thomas Huth wrote:
>> We can get a nicer progress indication if we add the iotests
>> individually via the 'check' script instead of going through
>> the check-block.sh wrapper.
>>
>> For this, we have to add some of the sanity checks that have
>> originally been done in the tests/check-block.sh script (whether
>> "bash" is available or whether CFLAGS contain -fsanitize switches)
>> to the meson.build file now, and add the environment variables
>> that have been set up by the tests/check-block.sh script before.
>>
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>> ---
>>   tests/qemu-iotests/meson.build | 45 ++++++++++++++++++++++++++++------
>>   1 file changed, 37 insertions(+), 8 deletions(-)
>>
>> diff --git a/tests/qemu-iotests/meson.build b/tests/qemu-iotests/meson.build
>> index e1832c90e0..5a6ccd35d8 100644
>> --- a/tests/qemu-iotests/meson.build
>> +++ b/tests/qemu-iotests/meson.build
>> @@ -1,9 +1,29 @@
>> -if not have_tools or targetos == 'windows'
>> +if not have_tools or targetos == 'windows' or \
>> +   config_host.has_key('CONFIG_GPROF')
>>     subdir_done()
>>   endif
>> +bash = find_program('bash', required: false)
>> +if not bash.found() or \
>> +   run_command(bash, ['--version']).stdout().contains('GNU bash, version 3')
>> +  message('bash >= v4.0 not available ==> Disabled the qemu-iotests.')
>> +  subdir_done()
>> +endif
>> +
>> +foreach cflag: config_host['QEMU_CFLAGS'].split()
>> +  if cflag.startswith('-fsanitize') and \
>> +     not cflag.contains('safe-stack') and not cflag.contains('cfi-icall')
>> +    message('Sanitizers are enabled ==> Disabled the qemu-iotests.')
>> +    subdir_done()
>> +  endif
>> +endforeach
>> +
>>   qemu_iotests_binaries = [qemu_img, qemu_io, qemu_nbd, qsd]
>> -qemu_iotests_env = {'PYTHON': python.full_path()}
>> +qemu_iotests_env = {
>> +  'PYTHON': python.full_path(),
>> +  'PYTHONUTF8': '1',
>> +  'QEMU_CHECK_BLOCK_AUTO': '1'
>> +}
>>   qemu_iotests_formats = {
>>     'qcow2': 'quick',
>>     'raw': 'slow',
>> @@ -18,16 +38,25 @@ foreach k, v : emulators
>>     endif
>>   endforeach
>> +check_script = find_program(meson.current_build_dir() / 'check')
>> +iotests = run_command(python, [check_script.full_path(), '-g', 'auto', 
>> '-n'],
>> +                      check: true).stdout().strip().replace('tests/', 
>> '').split('\n')
> 
> The main disadvantage here is that changes to the test directory will not 
> rebuild meson.build.  So when one creates a new test, meson has to be rerun 
> manually.
> 
> It is probably possible to do something like:
> 
> - run "check -g auto -n" with configure_file() and store the output in a file
> 
> - in the Makefile, always run "check -g auto -n", like
> 
> check-block-test-list: check-block-test-list.stamp
> check-block-test-list.stamp:
>      ./check -g auto -n > check-block-test-list.stamp; \
>      if cmp check-block-test-list.stamp check-block-test-list; then \
>          cp check-block-test-list.stamp check-block-test-list; \
>      fi
> .PHONY: check-block-test-list.stamp
> 
> check check-block: check-block-test-list.stamp
> 
> before giving control to "meson test"; but I'm not sure if that will also 
> cause a rebuild of Makefile.mtest and a meson rerun.
> 
> But I'm not sure it's worth it...

Before we go down that road, I think it would be better to get rid of the 
"auto" group and add the list of the iotests that should get run during 
"make check" to the meson.build file directly. That's easier to understand 
and less confusing.

> Regarding the issue that Peter pointed out, that's my fault for not being 
> aware of the "diff" being in the output of failing tests.  There are three 
> ways to fix it:
> 
> 1) reverting my patches
> 
> 2) Thomas's series, but only if the above issue is fixed
> 
> 3) shipping the tests/qemu-iotests/*.bad files as artifacts
> 
> 4) not using -tap (either reverting commit d316859f4e, "check-block: replace 
> -makecheck with TAP output", or just removing -tap from 
> tests/qemu-iotests/meson.build).
> 
> My preference is 4, then 1 or 2 (depending on the complexity), then 3.

What about simply printing the diff to stderr instead of stdout?
Something like this seems to work, at least for a quick glance:

diff --git a/tests/qemu-iotests/testrunner.py b/tests/qemu-iotests/testrunner.py
index 0eace147b8..d45a2688a0 100644
--- a/tests/qemu-iotests/testrunner.py
+++ b/tests/qemu-iotests/testrunner.py
@@ -404,7 +404,7 @@ def run_tests(self, tests: List[str], jobs: int = 1) -> 
bool:
              if res.status == 'fail':
                  failed.append(name)
                  if res.diff:
-                    print('\n'.join(res.diff))
+                    print('\n'.join(res.diff), file=sys.stderr)
              elif res.status == 'not run':
                  notrun.append(name)
              elif res.status == 'pass':

  Thomas

Re: [PATCH 4/6] tests/qemu-iotests/meson.build: Call the 'check' script directly

[Thomas Huth]

On 08/02/2022 14.12, Hanna Reitz wrote:
> On 08.02.22 11:13, Thomas Huth wrote:
>> We can get a nicer progress indication if we add the iotests
>> individually via the 'check' script instead of going through
>> the check-block.sh wrapper.
>>
>> For this, we have to add some of the sanity checks that have
>> originally been done in the tests/check-block.sh script (whether
>> "bash" is available or whether CFLAGS contain -fsanitize switches)
>> to the meson.build file now, and add the environment variables
>> that have been set up by the tests/check-block.sh script before.
>>
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>> ---
>>   tests/qemu-iotests/meson.build | 45 ++++++++++++++++++++++++++++------
>>   1 file changed, 37 insertions(+), 8 deletions(-)
>>
>> diff --git a/tests/qemu-iotests/meson.build b/tests/qemu-iotests/meson.build
>> index e1832c90e0..5a6ccd35d8 100644
>> --- a/tests/qemu-iotests/meson.build
>> +++ b/tests/qemu-iotests/meson.build
>> @@ -1,9 +1,29 @@
>> -if not have_tools or targetos == 'windows'
>> +if not have_tools or targetos == 'windows' or \
>> +   config_host.has_key('CONFIG_GPROF')
>>     subdir_done()
>>   endif
>> +bash = find_program('bash', required: false)
>> +if not bash.found() or \
>> +   run_command(bash, ['--version']).stdout().contains('GNU bash, version 3')
> 
> Instead of me asking about where the LANG=C is, or me lamenting that we 
> could test very simply for [123] before and can no longer now... Can we not 
> just do `find_program('bash', required: false, version: '>= 4.0')`?

Oh, cool, find_program() has a version parameter, didn't know that before! 
Thanks for the hint, I'll give it a try!

>> +  foreach tst: iotests
>> +    test('iotest-' + format + '-' + tst,
>> +         python, args: [check_script.full_path(), '-tap', '-' + format, 
>> tst],
>> +         depends: qemu_iotests_binaries,
>> +         env: qemu_iotests_env + \
>> +              { 'TEST_DIR':
>> +                meson.current_build_dir() / 'scratch' / format + '-' + 
>> tst },
>> +         protocol: 'tap',
>> +         suite: suites,
>> +         timeout: 0)
> 
> So as far I understand you’d like to have meson run the iotests in parallel 
> this way.  I don’t actually think that’s safely possible for multiple 
> formats at once, because a test’s output is always written into 
> `${build_dir}/tests/qemu-iotests/${seq}.out.bad`; so if you run e.g. test 
> 001 both with raw and qcow2 simultaneously, then they can get in each 
> other’s way.

Drat, I think you're right. I was testing with "make check SPEED=slow" and 
that was still working fine, but with "make check SPEED=thorough" I'm 
getting errors, indeed.

> (In my test branch, I have 
> https://gitlab.com/hreitz/qemu/-/commit/f3110b1eeb93d02aeadc5c8b807594cfa10a6aad 
> for this – maybe I should send something like this in a more refined form to 
> the list some time...)

Thanks a lot, that fixes the problems with SPEED=thorough indeed!

  Thomas

Re: [PATCH 4/6] tests/qemu-iotests/meson.build: Call the 'check' script directly

[Paolo Bonzini]

On 2/8/22 16:10, Thomas Huth wrote:
> Before we go down that road, I think it would be better to get rid of
> the "auto" group and add the list of the iotests that should get run
> during "make check" to the meson.build file directly. That's easier to
> understand and less confusing.

There are other groups than "auto", though.  It's a bit contrarian to 
the aim that changing one aspect of the build should only touch a single 
place (see commit 32fcc6244c, "contrib/vhost-user-input: convert to 
meson", for a reminder of what it was like to add an executable with 
Makefiles!).

> diff --git a/tests/qemu-iotests/testrunner.py 
> b/tests/qemu-iotests/testrunner.py
> index 0eace147b8..d45a2688a0 100644
> --- a/tests/qemu-iotests/testrunner.py
> +++ b/tests/qemu-iotests/testrunner.py
> @@ -404,7 +404,7 @@ def run_tests(self, tests: List[str], jobs: int = 1) 
> -> bool:
>               if res.status == 'fail':
>                   failed.append(name)
>                   if res.diff:
> -                    print('\n'.join(res.diff))
> +                    print('\n'.join(res.diff), file=sys.stderr)
>               elif res.status == 'not run':
>                   notrun.append(name)
>               elif res.status == 'pass':

Interesting.  But it should be done only if self.tap is true.

Paolo

Re: [PATCH 1/6] tests/qemu-iotests: Improve the check for GNU sed

[Eric Blake]

On Tue, Feb 08, 2022 at 03:52:19PM +0100, Thomas Huth wrote:
> > > The current code with $SED has been introduced almost three years
> > > ago already...
> > > 
> > > >   Can’t we just do `alias sed=gsed`?
> > > 
> > > Maybe ... but let's ask Philippe and Kevin first, who Signed-off
> > > commit bde36af1ab4f476 that introduced the current way with $SED:
> > > What's your opinion about this?
> > 
> > This commit was to have check-block working on the OpenBSD VM image.
> 
> Sure. The question was whether using an alias as suggested by Hanna would be
> nicer instead of using $SED ?

Scripting with aliases becomes a nightmare to debug, since it is
relatively uncommon.  In particular, in bash, you have to explicitly
opt in to using aliases (contrary to POSIX sh where aliases are
available to scripts at startup).  Using $SED everywhere may require
more hunting, but it is more obvious when reading a test that "oh
yeah, I might be using extensions that the default 'sed' can't
support" than a script that blindly uses 'sed' and depends on it
aliasing to a more-capable sed at a distance.

The other question is how many GNU sed features are we actually
depending on?  Which tests break if we have BSD sed or busybox sed?
Can we rewrite those sed scripts to avoid GNU extensions?  But
auditing for s/sed/$SED/ seems easier than auditing for which
non-portable sed extensions we depend on.

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

Re: [PATCH 1/6] tests/qemu-iotests: Improve the check for GNU sed

[Thomas Huth]

On 11/02/2022 17.14, Eric Blake wrote:
> On Tue, Feb 08, 2022 at 03:52:19PM +0100, Thomas Huth wrote:
>>>> The current code with $SED has been introduced almost three years
>>>> ago already...
>>>>
>>>>>    Can’t we just do `alias sed=gsed`?
>>>>
>>>> Maybe ... but let's ask Philippe and Kevin first, who Signed-off
>>>> commit bde36af1ab4f476 that introduced the current way with $SED:
>>>> What's your opinion about this?
>>>
>>> This commit was to have check-block working on the OpenBSD VM image.
>>
>> Sure. The question was whether using an alias as suggested by Hanna would be
>> nicer instead of using $SED ?
> 
> Scripting with aliases becomes a nightmare to debug, since it is
> relatively uncommon.  In particular, in bash, you have to explicitly
> opt in to using aliases (contrary to POSIX sh where aliases are
> available to scripts at startup).

shopt -s expand_aliases
... as I just learnt the hard way ;-)

> Using $SED everywhere may require
> more hunting, but it is more obvious when reading a test that "oh
> yeah, I might be using extensions that the default 'sed' can't
> support" than a script that blindly uses 'sed' and depends on it
> aliasing to a more-capable sed at a distance.
> 
> The other question is how many GNU sed features are we actually
> depending on?  Which tests break if we have BSD sed or busybox sed?
> Can we rewrite those sed scripts to avoid GNU extensions?  But
> auditing for s/sed/$SED/ seems easier than auditing for which
> non-portable sed extensions we depend on.

The most obvious part are the filter functions in common.filter - we're 
using "-r" here that is not part of the POSIX sed as far as I can see.

Not sure whether anybody really wants to rewrite all sed statements for full 
portability, but maybe we could also introduce a wrapper for GNU sed like this:

if ! command -v gsed >/dev/null 2>&1; then
     if sed --version | grep -v 'not GNU sed' | grep 'GNUx sed' \
        > /dev/null 2>&1;
     then
         gsed()
         {
             sed $*
         }
     else
         gsed()
         {
             _notrun "GNU sed not available"
         }
     fi
fi

... then we could simply use "gsed" everywhere we depend on the GNU 
behavior, and the tests don't look as ugly as with the $SED ?

  Thomas

Re: [PATCH 1/6] tests/qemu-iotests: Improve the check for GNU sed

[Thomas Huth]

On 11/02/2022 17.48, Thomas Huth wrote:
> On 11/02/2022 17.14, Eric Blake wrote:
>> On Tue, Feb 08, 2022 at 03:52:19PM +0100, Thomas Huth wrote:
>>>>> The current code with $SED has been introduced almost three years
>>>>> ago already...
>>>>>
>>>>>>    Can’t we just do `alias sed=gsed`?
>>>>>
>>>>> Maybe ... but let's ask Philippe and Kevin first, who Signed-off
>>>>> commit bde36af1ab4f476 that introduced the current way with $SED:
>>>>> What's your opinion about this?
>>>>
>>>> This commit was to have check-block working on the OpenBSD VM image.
>>>
>>> Sure. The question was whether using an alias as suggested by Hanna would be
>>> nicer instead of using $SED ?
>>
>> Scripting with aliases becomes a nightmare to debug, since it is
>> relatively uncommon.  In particular, in bash, you have to explicitly
>> opt in to using aliases (contrary to POSIX sh where aliases are
>> available to scripts at startup).
> 
> shopt -s expand_aliases
> ... as I just learnt the hard way ;-)
> 
>> Using $SED everywhere may require
>> more hunting, but it is more obvious when reading a test that "oh
>> yeah, I might be using extensions that the default 'sed' can't
>> support" than a script that blindly uses 'sed' and depends on it
>> aliasing to a more-capable sed at a distance.
>>
>> The other question is how many GNU sed features are we actually
>> depending on?  Which tests break if we have BSD sed or busybox sed?
>> Can we rewrite those sed scripts to avoid GNU extensions?  But
>> auditing for s/sed/$SED/ seems easier than auditing for which
>> non-portable sed extensions we depend on.
> 
> The most obvious part are the filter functions in common.filter - we're 
> using "-r" here that is not part of the POSIX sed as far as I can see.

Now that I stepped through the list, the other major part that is failing on 
non-GNU seds are the statements that use "\r" or "\n" or "\e" to replace 
special characters. That seems to be a non-POSIX extension, too.

But for running with Alpine, there is also the additional problems that the 
libc uses slightly different error strings, e.g. "I/O error" instead of 
"Input/output error", see e.g.:

  https://gitlab.com/thuth/qemu/-/jobs/2094869856

Maybe it could be fixed with some extensions to the filters, but I'm not 
sure whether we really want to go down that road...?

  Thomas

Re: [PATCH 1/6] tests/qemu-iotests: Improve the check for GNU sed

[Daniel P. Berrangé]

On Tue, Feb 15, 2022 at 02:28:24PM +0100, Thomas Huth wrote:
> On 11/02/2022 17.48, Thomas Huth wrote:
> > On 11/02/2022 17.14, Eric Blake wrote:
> > > On Tue, Feb 08, 2022 at 03:52:19PM +0100, Thomas Huth wrote:
> > > > > > The current code with $SED has been introduced almost three years
> > > > > > ago already...
> > > > > > 
> > > > > > >    Can’t we just do `alias sed=gsed`?
> > > > > > 
> > > > > > Maybe ... but let's ask Philippe and Kevin first, who Signed-off
> > > > > > commit bde36af1ab4f476 that introduced the current way with $SED:
> > > > > > What's your opinion about this?
> > > > > 
> > > > > This commit was to have check-block working on the OpenBSD VM image.
> > > > 
> > > > Sure. The question was whether using an alias as suggested by Hanna would be
> > > > nicer instead of using $SED ?
> > > 
> > > Scripting with aliases becomes a nightmare to debug, since it is
> > > relatively uncommon.  In particular, in bash, you have to explicitly
> > > opt in to using aliases (contrary to POSIX sh where aliases are
> > > available to scripts at startup).
> > 
> > shopt -s expand_aliases
> > ... as I just learnt the hard way ;-)
> > 
> > > Using $SED everywhere may require
> > > more hunting, but it is more obvious when reading a test that "oh
> > > yeah, I might be using extensions that the default 'sed' can't
> > > support" than a script that blindly uses 'sed' and depends on it
> > > aliasing to a more-capable sed at a distance.
> > > 
> > > The other question is how many GNU sed features are we actually
> > > depending on?  Which tests break if we have BSD sed or busybox sed?
> > > Can we rewrite those sed scripts to avoid GNU extensions?  But
> > > auditing for s/sed/$SED/ seems easier than auditing for which
> > > non-portable sed extensions we depend on.
> > 
> > The most obvious part are the filter functions in common.filter - we're
> > using "-r" here that is not part of the POSIX sed as far as I can see.
> 
> Now that I stepped through the list, the other major part that is failing on
> non-GNU seds are the statements that use "\r" or "\n" or "\e" to replace
> special characters. That seems to be a non-POSIX extension, too.
> 
> But for running with Alpine, there is also the additional problems that the
> libc uses slightly different error strings, e.g. "I/O error" instead of
> "Input/output error", see e.g.:
> 
>  https://gitlab.com/thuth/qemu/-/jobs/2094869856
> 
> Maybe it could be fixed with some extensions to the filters, but I'm not
> sure whether we really want to go down that road...?

AFAIK, errno strings are not standardized by POSIX, so I presume this
problem will apply to running I/O tests on any non-Linux system too.

With this in mind I think we should consider what a portable solution
looks like. We can't simply match the Alpine strings and turn them
into GLibC strings, as that does nothing to help portability on *BSD,
macOS, Windows, etc. We would need to figure out how to blank out
arbitrary input error message strings.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH 1/6] tests/qemu-iotests: Improve the check for GNU sed

[Thomas Huth]

On 15/02/2022 14.51, Daniel P. Berrangé wrote:
> On Tue, Feb 15, 2022 at 02:28:24PM +0100, Thomas Huth wrote:
>> On 11/02/2022 17.48, Thomas Huth wrote:
>>> On 11/02/2022 17.14, Eric Blake wrote:
>>>> On Tue, Feb 08, 2022 at 03:52:19PM +0100, Thomas Huth wrote:
>>>>>>> The current code with $SED has been introduced almost three years
>>>>>>> ago already...
>>>>>>>
>>>>>>>>     Can’t we just do `alias sed=gsed`?
>>>>>>>
>>>>>>> Maybe ... but let's ask Philippe and Kevin first, who Signed-off
>>>>>>> commit bde36af1ab4f476 that introduced the current way with $SED:
>>>>>>> What's your opinion about this?
>>>>>>
>>>>>> This commit was to have check-block working on the OpenBSD VM image.
>>>>>
>>>>> Sure. The question was whether using an alias as suggested by Hanna would be
>>>>> nicer instead of using $SED ?
>>>>
>>>> Scripting with aliases becomes a nightmare to debug, since it is
>>>> relatively uncommon.  In particular, in bash, you have to explicitly
>>>> opt in to using aliases (contrary to POSIX sh where aliases are
>>>> available to scripts at startup).
>>>
>>> shopt -s expand_aliases
>>> ... as I just learnt the hard way ;-)
>>>
>>>> Using $SED everywhere may require
>>>> more hunting, but it is more obvious when reading a test that "oh
>>>> yeah, I might be using extensions that the default 'sed' can't
>>>> support" than a script that blindly uses 'sed' and depends on it
>>>> aliasing to a more-capable sed at a distance.
>>>>
>>>> The other question is how many GNU sed features are we actually
>>>> depending on?  Which tests break if we have BSD sed or busybox sed?
>>>> Can we rewrite those sed scripts to avoid GNU extensions?  But
>>>> auditing for s/sed/$SED/ seems easier than auditing for which
>>>> non-portable sed extensions we depend on.
>>>
>>> The most obvious part are the filter functions in common.filter - we're
>>> using "-r" here that is not part of the POSIX sed as far as I can see.
>>
>> Now that I stepped through the list, the other major part that is failing on
>> non-GNU seds are the statements that use "\r" or "\n" or "\e" to replace
>> special characters. That seems to be a non-POSIX extension, too.
>>
>> But for running with Alpine, there is also the additional problems that the
>> libc uses slightly different error strings, e.g. "I/O error" instead of
>> "Input/output error", see e.g.:
>>
>>   https://gitlab.com/thuth/qemu/-/jobs/2094869856
>>
>> Maybe it could be fixed with some extensions to the filters, but I'm not
>> sure whether we really want to go down that road...?
> 
> AFAIK, errno strings are not standardized by POSIX, so I presume this
> problem will apply to running I/O tests on any non-Linux system too.
> 
> With this in mind I think we should consider what a portable solution
> looks like. We can't simply match the Alpine strings and turn them
> into GLibC strings, as that does nothing to help portability on *BSD,
> macOS, Windows, etc.
Luckily, the strings did not cause that much problems on *BSDs and macOS 
yet... Most of the current set of tests works fine there. It's really just 
that libc from Alpine that is causing this trouble...

  Thomas