[PATCH 0/5] iotests: make meson aware of individual I/O tests

[PATCH 0/5] iotests: make meson aware of individual I/O tests

[Daniel P. Berrangé]

To just repeat the patch 5 description...

Currently meson registers a single test that invokes an entire group of
I/O tests, hiding the test granularity from meson. There are various
downsides of doing this

 * You cannot ask 'meson test' to invoke a single I/O test
 * The meson test timeout can't be applied to the individual
   tests
 * Meson only gets a pass/fail for the overall I/O test group
   not individual tests
 * If a CI job gets killed by the GitLab timeout, we don't
   get visibility into how far through the I/O tests
   execution got.

This is not really specific to the I/O tests, the problem is common
to any case of us running a test which is in fact another test
harness which runs many tests. It would be nice to have meson have
the full view of all tests run. Adapting the I/O tests is as easy
win in this respect.

This switches meson to perform test discovery by invoking 'check' in
dry-run mode. It then registers one meson test case for each I/O
test. Parallel execution remains disabled since the I/O tests do not
use self contained execution environments and thus conflict with
each other.

Compare contrast output from a current job:

  https://gitlab.com/qemu-project/qemu/-/jobs/3863603546

[quote]
204/224 qemu:block / qemu-iotests qcow2   OK 329.94s   119 subtests passed
[/quote]

Vs what is seen with this series:

  https://gitlab.com/berrange/qemu/-/jobs/3865975463

[quote]
204/350 qemu:block / qemu-iotests-qcow2-001   OK    2.16s   1 subtests passed
205/350 qemu:block / qemu-iotests-qcow2-002   OK    2.77s   1 subtests passed

...snip...

329/350 qemu:block / qemu-iotests-qcow2-qemu-img-close-errors       OK    6.19s   1 subtests passed
330/350 qemu:block / qemu-iotests-qcow2-qsd-jobs          OK    0.55s   1 subtests passed
[/quote]

A few tweaks were needed to the iotests runner because it had a few
assumptions about it always running in a tree that has already been
built, which is obviously not the case at the time meson does test
discovery.

Daniel P. Berrangé (5):
  iotests: explicitly pass source/build dir to 'check' command
  iotests: allow test discovery before building
  iotests: strip subdir path when listing tests
  iotests: print TAP protocol version when reporting tests
  iotests: register each I/O test separately with meson

 tests/qemu-iotests/check         | 11 +++++++++--
 tests/qemu-iotests/meson.build   | 33 ++++++++++++++++++++++++++------
 tests/qemu-iotests/testenv.py    | 22 +++++++++++++++++----
 tests/qemu-iotests/testrunner.py |  1 +
 4 files changed, 55 insertions(+), 12 deletions(-)

-- 
2.39.2

[PATCH 1/5] iotests: explicitly pass source/build dir to 'check' command

[Daniel P. Berrangé]

The 'check' script has some rather dubious logic whereby it assumes
that if invoked as a symlink, then it is running from a separate
source tree and build tree, otherwise it assumes the current working
directory is a combined source and build tree.

This doesn't work if you want to invoke the 'check' script using
its full source tree path while still using a split source and build
tree layout. This would be a typical situation with meson if you ask
it to find the 'check' script path using files('check').

Rather than trying to make the logic more magical, add support for
explicitly passing the dirs using --source-dir and --build-dir. If
either is omitted the current logic is maintained.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 tests/qemu-iotests/check      |  8 +++++++-
 tests/qemu-iotests/testenv.py | 17 +++++++++++++----
 2 files changed, 20 insertions(+), 5 deletions(-)

diff --git a/tests/qemu-iotests/check b/tests/qemu-iotests/check
index 9bdda1394e..806abc21d6 100755
--- a/tests/qemu-iotests/check
+++ b/tests/qemu-iotests/check
@@ -113,6 +113,10 @@ def make_argparser() -> argparse.ArgumentParser:
                        'middle of the process.')
     g_sel.add_argument('tests', metavar='TEST_FILES', nargs='*',
                        help='tests to run, or "--" followed by a command')
+    g_sel.add_argument('--build-dir', default=None,
+                       help='Path to iotests build directory')
+    g_sel.add_argument('--source-dir', default=None,
+                       help='Path to iotests build directory')
 
     return p
 
@@ -124,7 +128,9 @@ if __name__ == '__main__':
                   aiomode=args.aiomode, cachemode=args.cachemode,
                   imgopts=args.imgopts, misalign=args.misalign,
                   debug=args.debug, valgrind=args.valgrind,
-                  gdb=args.gdb, qprint=args.print)
+                  gdb=args.gdb, qprint=args.print,
+                  source_dir=args.source_dir,
+                  build_dir=args.build_dir)
 
     if len(sys.argv) > 1 and sys.argv[-len(args.tests)-1] == '--':
         if not args.tests:
diff --git a/tests/qemu-iotests/testenv.py b/tests/qemu-iotests/testenv.py
index a864c74b12..9bf37cd381 100644
--- a/tests/qemu-iotests/testenv.py
+++ b/tests/qemu-iotests/testenv.py
@@ -177,7 +177,9 @@ def __init__(self, imgfmt: str, imgproto: str, aiomode: str,
                  debug: bool = False,
                  valgrind: bool = False,
                  gdb: bool = False,
-                 qprint: bool = False) -> None:
+                 qprint: bool = False,
+                 source_dir: Optional[str] = None,
+                 build_dir: Optional[str] = None) -> None:
         self.imgfmt = imgfmt
         self.imgproto = imgproto
         self.aiomode = aiomode
@@ -213,12 +215,19 @@ def __init__(self, imgfmt: str, imgproto: str, aiomode: str,
 
         if os.path.islink(sys.argv[0]):
             # called from the build tree
-            self.source_iotests = os.path.dirname(os.readlink(sys.argv[0]))
-            self.build_iotests = os.path.dirname(os.path.abspath(sys.argv[0]))
+            self.source_iotests = os.path.dirname(
+                os.readlink(sys.argv[0]))
+            self.build_iotests = os.path.dirname(
+                os.path.abspath(sys.argv[0]))
         else:
             # called from the source tree
             self.source_iotests = os.getcwd()
-            self.build_iotests = self.source_iotests
+            self.build_iotests = os.getcwd()
+
+        if source_dir is not None:
+            self.source_iotests = source_dir
+        if build_dir is not None:
+            self.build_iotests = build_dir
 
         self.build_root = os.path.join(self.build_iotests, '..', '..')
 
-- 
2.39.2

[PATCH 2/5] iotests: allow test discovery before building

[Daniel P. Berrangé]

The 'check' script can be invoked in "dry run" mode, in which case it
merely does test discovery and prints out all their names. Despite only
doing test discovery it still validates that the various QEMU binaries
can be found. This makes it impossible todo test discovery prior to
building QEMU. This is a desirable feature to support, because it will
let meson discover tests.

Fortunately the code in the TestEnv constructor is ordered in a way
that makes this fairly trivial to achieve. We can just short circuit
the constructor after the basic directory paths have been set.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 tests/qemu-iotests/check      | 1 +
 tests/qemu-iotests/testenv.py | 5 +++++
 2 files changed, 6 insertions(+)

diff --git a/tests/qemu-iotests/check b/tests/qemu-iotests/check
index 806abc21d6..7e287a79a3 100755
--- a/tests/qemu-iotests/check
+++ b/tests/qemu-iotests/check
@@ -129,6 +129,7 @@ if __name__ == '__main__':
                   imgopts=args.imgopts, misalign=args.misalign,
                   debug=args.debug, valgrind=args.valgrind,
                   gdb=args.gdb, qprint=args.print,
+                  dry_run=args.dry_run,
                   source_dir=args.source_dir,
                   build_dir=args.build_dir)
 
diff --git a/tests/qemu-iotests/testenv.py b/tests/qemu-iotests/testenv.py
index 9bf37cd381..952efa0e63 100644
--- a/tests/qemu-iotests/testenv.py
+++ b/tests/qemu-iotests/testenv.py
@@ -178,6 +178,7 @@ def __init__(self, imgfmt: str, imgproto: str, aiomode: str,
                  valgrind: bool = False,
                  gdb: bool = False,
                  qprint: bool = False,
+                 dry_run: bool = False,
                  source_dir: Optional[str] = None,
                  build_dir: Optional[str] = None) -> None:
         self.imgfmt = imgfmt
@@ -232,6 +233,10 @@ def __init__(self, imgfmt: str, imgproto: str, aiomode: str,
         self.build_root = os.path.join(self.build_iotests, '..', '..')
 
         self.init_directories()
+
+        if dry_run:
+            return
+
         self.init_binaries()
 
         self.malloc_perturb_ = os.getenv('MALLOC_PERTURB_',
-- 
2.39.2

[PATCH 3/5] iotests: strip subdir path when listing tests

[Daniel P. Berrangé]

When asking 'check' to list individual tests by invoking it in dry run
mode, it prints the paths to the tests relative to the base of the
I/O test directory.

When asking 'check' to run an individual test, however, it mandates that
only the unqualified test name is given, without any path prefix. This
inconsistency makes it harder to ask for a list of tests and then invoke
each one.

Thus the test listing code is change to flatten the test names, by
printing only the base name, which can be directly invoked.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 tests/qemu-iotests/check | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/qemu-iotests/check b/tests/qemu-iotests/check
index 7e287a79a3..3a8744220a 100755
--- a/tests/qemu-iotests/check
+++ b/tests/qemu-iotests/check
@@ -169,7 +169,7 @@ if __name__ == '__main__':
         sys.exit(str(e))
 
     if args.dry_run:
-        print('\n'.join(tests))
+        print('\n'.join([os.path.basename(t) for t in tests]))
     else:
         with TestRunner(env, tap=args.tap,
                         color=args.color) as tr:
-- 
2.39.2

[PATCH 4/5] iotests: print TAP protocol version when reporting tests

[Daniel P. Berrangé]

Recently meson started complaining that TAP test reports don't include
the TAP protocol version. While this warning is bogus and has since been
removed from Meson, it looks like good practice to include this header
going forward. The GLib library test harness has started unconditionally
printing the version, so this brings the I/O tests into line.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 tests/qemu-iotests/testrunner.py | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tests/qemu-iotests/testrunner.py b/tests/qemu-iotests/testrunner.py
index 5a771da86e..e734800b3d 100644
--- a/tests/qemu-iotests/testrunner.py
+++ b/tests/qemu-iotests/testrunner.py
@@ -391,6 +391,7 @@ def run_tests(self, tests: List[str], jobs: int = 1) -> bool:
         casenotrun = []
 
         if self.tap:
+            print('TAP version 13')
             self.env.print_env('# ')
             print('1..%d' % len(tests))
         else:
-- 
2.39.2

[PATCH 5/5] iotests: register each I/O test separately with meson

[Daniel P. Berrangé]

Currently meson registers a single test that invokes an entire group of
I/O tests, hiding the test granularity from meson. There are various
downsides of doing this

 * You cannot ask 'meson test' to invoke a single I/O test
 * The meson test timeout can't be applied to the individual
   tests
 * Meson only gets a pass/fail for the overall I/O test group
   not individual tests
 * If a CI job gets killed by the GitLab timeout, we don't
   get visibility into how far through the I/O tests
   execution got.

This switches meson to perform test discovery by invoking 'check' in
dry-run mode. It then registers one meson test case for each I/O
test. Parallel execution remains disabled since the I/O tests do not
use self contained execution environments and thus conflict with
each other.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 tests/qemu-iotests/meson.build | 33 +++++++++++++++++++++++++++------
 1 file changed, 27 insertions(+), 6 deletions(-)

diff --git a/tests/qemu-iotests/meson.build b/tests/qemu-iotests/meson.build
index 323a4acb6a..48c82085af 100644
--- a/tests/qemu-iotests/meson.build
+++ b/tests/qemu-iotests/meson.build
@@ -32,16 +32,37 @@ foreach k, v : emulators
   endif
 endforeach
 
+qemu_iotests_check_cmd = files('check')
+
 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)
+
+  args = ['-tap', '-' + format]
+  if speed == 'quick'
+      args += ['-g', 'auto']
+  endif
+
+  rc = run_command(
+      [qemu_iotests_check_cmd] + args + ['-n'],
+      check: true,
+  )
+
+  foreach item: rc.stdout().strip().split()
+      message('Adding test qemu-iotests-' + format + '-' + item)
+      args = ['-tap', '-' + format, item,
+              '--source-dir', meson.current_source_dir(),
+              '--build-dir', meson.current_build_dir()]
+      test('qemu-iotests-' + format + '-' + item,
+           qemu_iotests_check_cmd,
+           args: args,
+           is_parallel: false,
+           depends: qemu_iotests_binaries,
+           env: qemu_iotests_env,
+           protocol: 'tap',
+           suite: suites)
+  endforeach
 endforeach
-- 
2.39.2

Re: [PATCH 0/5] iotests: make meson aware of individual I/O tests

[Daniel P. Berrangé]

On Thu, Mar 02, 2023 at 06:46:01PM +0000, Daniel P. Berrangé wrote:
> To just repeat the patch 5 description...
> 
> Currently meson registers a single test that invokes an entire group of
> I/O tests, hiding the test granularity from meson. There are various
> downsides of doing this
> 
>  * You cannot ask 'meson test' to invoke a single I/O test
>  * The meson test timeout can't be applied to the individual
>    tests
>  * Meson only gets a pass/fail for the overall I/O test group
>    not individual tests

Another big benefit is that we get to see the execution time of
every I/O test, so can identify which handful of tests are making
it take almost 5 minutes to run in gitlab...

The really big 4 are:

199/314 qemu:block / qemu-iotests-qcow2-040   OK    16.62s   1 subtests passed
200/314 qemu:block / qemu-iotests-qcow2-041   OK    44.51s   1 subtests passed
265/314 qemu:block / qemu-iotests-qcow2-191   OK    17.93s   1 subtests passed
282/314 qemu:block / qemu-iotests-qcow2-271   OK    25.03s   1 subtests passed

there are a bunch in the ~5 second range, and then most are
1 second or less.

>  * If a CI job gets killed by the GitLab timeout, we don't
>    get visibility into how far through the I/O tests
>    execution got.
> 
> This is not really specific to the I/O tests, the problem is common
> to any case of us running a test which is in fact another test
> harness which runs many tests. It would be nice to have meson have
> the full view of all tests run. Adapting the I/O tests is as easy
> win in this respect.
> 
> This switches meson to perform test discovery by invoking 'check' in
> dry-run mode. It then registers one meson test case for each I/O
> test. Parallel execution remains disabled since the I/O tests do not
> use self contained execution environments and thus conflict with
> each other.
> 
> Compare contrast output from a current job:
> 
>   https://gitlab.com/qemu-project/qemu/-/jobs/3863603546
> 
> [quote]
> 204/224 qemu:block / qemu-iotests qcow2   OK 329.94s   119 subtests passed
> [/quote]
> 
> Vs what is seen with this series:
> 
>   https://gitlab.com/berrange/qemu/-/jobs/3865975463
> 
> [quote]
> 204/350 qemu:block / qemu-iotests-qcow2-001   OK    2.16s   1 subtests passed
> 205/350 qemu:block / qemu-iotests-qcow2-002   OK    2.77s   1 subtests passed
> 
> ...snip...
> 
> 329/350 qemu:block / qemu-iotests-qcow2-qemu-img-close-errors       OK    6.19s   1 subtests passed
> 330/350 qemu:block / qemu-iotests-qcow2-qsd-jobs          OK    0.55s   1 subtests passed
> [/quote]
> 
> A few tweaks were needed to the iotests runner because it had a few
> assumptions about it always running in a tree that has already been
> built, which is obviously not the case at the time meson does test
> discovery.
> 
> Daniel P. Berrangé (5):
>   iotests: explicitly pass source/build dir to 'check' command
>   iotests: allow test discovery before building
>   iotests: strip subdir path when listing tests
>   iotests: print TAP protocol version when reporting tests
>   iotests: register each I/O test separately with meson
> 
>  tests/qemu-iotests/check         | 11 +++++++++--
>  tests/qemu-iotests/meson.build   | 33 ++++++++++++++++++++++++++------
>  tests/qemu-iotests/testenv.py    | 22 +++++++++++++++++----
>  tests/qemu-iotests/testrunner.py |  1 +
>  4 files changed, 55 insertions(+), 12 deletions(-)
> 
> -- 
> 2.39.2
> 

With 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 2/5] iotests: allow test discovery before building

[Thomas Huth]

On 02/03/2023 19.46, Daniel P. Berrangé wrote:
> The 'check' script can be invoked in "dry run" mode, in which case it
> merely does test discovery and prints out all their names. Despite only
> doing test discovery it still validates that the various QEMU binaries
> can be found. This makes it impossible todo test discovery prior to
> building QEMU. This is a desirable feature to support, because it will
> let meson discover tests.
> 
> Fortunately the code in the TestEnv constructor is ordered in a way
> that makes this fairly trivial to achieve. We can just short circuit
> the constructor after the basic directory paths have been set.
> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>   tests/qemu-iotests/check      | 1 +
>   tests/qemu-iotests/testenv.py | 5 +++++
>   2 files changed, 6 insertions(+)
> 
> diff --git a/tests/qemu-iotests/check b/tests/qemu-iotests/check
> index 806abc21d6..7e287a79a3 100755
> --- a/tests/qemu-iotests/check
> +++ b/tests/qemu-iotests/check
> @@ -129,6 +129,7 @@ if __name__ == '__main__':
>                     imgopts=args.imgopts, misalign=args.misalign,
>                     debug=args.debug, valgrind=args.valgrind,
>                     gdb=args.gdb, qprint=args.print,
> +                  dry_run=args.dry_run,
>                     source_dir=args.source_dir,
>                     build_dir=args.build_dir)
>   
> diff --git a/tests/qemu-iotests/testenv.py b/tests/qemu-iotests/testenv.py
> index 9bf37cd381..952efa0e63 100644
> --- a/tests/qemu-iotests/testenv.py
> +++ b/tests/qemu-iotests/testenv.py
> @@ -178,6 +178,7 @@ def __init__(self, imgfmt: str, imgproto: str, aiomode: str,
>                    valgrind: bool = False,
>                    gdb: bool = False,
>                    qprint: bool = False,
> +                 dry_run: bool = False,
>                    source_dir: Optional[str] = None,
>                    build_dir: Optional[str] = None) -> None:
>           self.imgfmt = imgfmt
> @@ -232,6 +233,10 @@ def __init__(self, imgfmt: str, imgproto: str, aiomode: str,
>           self.build_root = os.path.join(self.build_iotests, '..', '..')
>   
>           self.init_directories()
> +
> +        if dry_run:
> +            return
> +
>           self.init_binaries()
>   
>           self.malloc_perturb_ = os.getenv('MALLOC_PERTURB_',

This gives me a feeling of Deja-Vu:

  https://lore.kernel.org/all/20220209101530.3442837-5-thuth@redhat.com/

... to bad that I never got that merged ;-)

Anyway:
Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [PATCH 4/5] iotests: print TAP protocol version when reporting tests

[Thomas Huth]

On 02/03/2023 19.46, Daniel P. Berrangé wrote:
> Recently meson started complaining that TAP test reports don't include
> the TAP protocol version. While this warning is bogus and has since been
> removed from Meson, it looks like good practice to include this header
> going forward. The GLib library test harness has started unconditionally
> printing the version, so this brings the I/O tests into line.
> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>   tests/qemu-iotests/testrunner.py | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/tests/qemu-iotests/testrunner.py b/tests/qemu-iotests/testrunner.py
> index 5a771da86e..e734800b3d 100644
> --- a/tests/qemu-iotests/testrunner.py
> +++ b/tests/qemu-iotests/testrunner.py
> @@ -391,6 +391,7 @@ def run_tests(self, tests: List[str], jobs: int = 1) -> bool:
>           casenotrun = []
>   
>           if self.tap:
> +            print('TAP version 13')
>               self.env.print_env('# ')
>               print('1..%d' % len(tests))
>           else:

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [PATCH 0/5] iotests: make meson aware of individual I/O tests

[Thomas Huth]

On 02/03/2023 19.46, Daniel P. Berrangé wrote:
> To just repeat the patch 5 description...
> 
> Currently meson registers a single test that invokes an entire group of
> I/O tests, hiding the test granularity from meson. There are various
> downsides of doing this
> 
>   * You cannot ask 'meson test' to invoke a single I/O test
>   * The meson test timeout can't be applied to the individual
>     tests
>   * Meson only gets a pass/fail for the overall I/O test group
>     not individual tests
>   * If a CI job gets killed by the GitLab timeout, we don't
>     get visibility into how far through the I/O tests
>     execution got.
> 
> This is not really specific to the I/O tests, the problem is common
> to any case of us running a test which is in fact another test
> harness which runs many tests. It would be nice to have meson have
> the full view of all tests run. Adapting the I/O tests is as easy
> win in this respect.
> 
> This switches meson to perform test discovery by invoking 'check' in
> dry-run mode. It then registers one meson test case for each I/O
> test. Parallel execution remains disabled since the I/O tests do not
> use self contained execution environments and thus conflict with
> each other.

Great to see some movement in this area again!

Some questions/remarks:

1) Could you remove tests/check-block.sh now? See also:
    https://lore.kernel.org/all/20220209101530.3442837-9-thuth@redhat.com/

2) With regards to parallel execution ... I think it should be
    possible nowadays - the "check" script is normally also run
    with the "-j" switch by the tests/check-block.sh script, so
    if you remove the possibility to run in parallel, it's a
    regression from the previous behavior!

3) When I tried this last year, I had a weird problem that
    the terminal sometimes gets messed up ... I wasn't able
    to track it down back then - could you check by running
    "make check-block" many times (>10 times) to see whether
    it happens with your series or not?

  Thomas

Re: [PATCH 0/5] iotests: make meson aware of individual I/O tests

[Daniel P. Berrangé]

On Fri, Mar 03, 2023 at 09:30:39AM +0100, Thomas Huth wrote:
> On 02/03/2023 19.46, Daniel P. Berrangé wrote:
> > To just repeat the patch 5 description...
> > 
> > Currently meson registers a single test that invokes an entire group of
> > I/O tests, hiding the test granularity from meson. There are various
> > downsides of doing this
> > 
> >   * You cannot ask 'meson test' to invoke a single I/O test
> >   * The meson test timeout can't be applied to the individual
> >     tests
> >   * Meson only gets a pass/fail for the overall I/O test group
> >     not individual tests
> >   * If a CI job gets killed by the GitLab timeout, we don't
> >     get visibility into how far through the I/O tests
> >     execution got.
> > 
> > This is not really specific to the I/O tests, the problem is common
> > to any case of us running a test which is in fact another test
> > harness which runs many tests. It would be nice to have meson have
> > the full view of all tests run. Adapting the I/O tests is as easy
> > win in this respect.
> > 
> > This switches meson to perform test discovery by invoking 'check' in
> > dry-run mode. It then registers one meson test case for each I/O
> > test. Parallel execution remains disabled since the I/O tests do not
> > use self contained execution environments and thus conflict with
> > each other.
> 
> Great to see some movement in this area again!
> 
> Some questions/remarks:
> 
> 1) Could you remove tests/check-block.sh now? See also:
>    https://lore.kernel.org/all/20220209101530.3442837-9-thuth@redhat.com/

Possibly, I wasn't sure if that was wanted as a general entry
point for humans, or was solely for meson ?

> 2) With regards to parallel execution ... I think it should be
>    possible nowadays - the "check" script is normally also run
>    with the "-j" switch by the tests/check-block.sh script, so
>    if you remove the possibility to run in parallel, it's a
>    regression from the previous behavior!

Hmmm, I got *masses* of test failures when running in parallel
but I'll check again to be sure.

With 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 5/5] iotests: register each I/O test separately with meson

[Thomas Huth]

On 02/03/2023 19.46, Daniel P. Berrangé wrote:
> Currently meson registers a single test that invokes an entire group of
> I/O tests, hiding the test granularity from meson. There are various
> downsides of doing this
> 
>   * You cannot ask 'meson test' to invoke a single I/O test
>   * The meson test timeout can't be applied to the individual
>     tests
>   * Meson only gets a pass/fail for the overall I/O test group
>     not individual tests
>   * If a CI job gets killed by the GitLab timeout, we don't
>     get visibility into how far through the I/O tests
>     execution got.
> 
> This switches meson to perform test discovery by invoking 'check' in
> dry-run mode. It then registers one meson test case for each I/O
> test. Parallel execution remains disabled since the I/O tests do not
> use self contained execution environments and thus conflict with
> each other.
> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>   tests/qemu-iotests/meson.build | 33 +++++++++++++++++++++++++++------
>   1 file changed, 27 insertions(+), 6 deletions(-)
> 
> diff --git a/tests/qemu-iotests/meson.build b/tests/qemu-iotests/meson.build
> index 323a4acb6a..48c82085af 100644
> --- a/tests/qemu-iotests/meson.build
> +++ b/tests/qemu-iotests/meson.build
> @@ -32,16 +32,37 @@ foreach k, v : emulators
>     endif
>   endforeach
>   
> +qemu_iotests_check_cmd = files('check')
> +
>   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)
> +
> +  args = ['-tap', '-' + format]
> +  if speed == 'quick'
> +      args += ['-g', 'auto']
> +  endif
> +
> +  rc = run_command(
> +      [qemu_iotests_check_cmd] + args + ['-n'],
> +      check: true,
> +  )
> +
> +  foreach item: rc.stdout().strip().split()
> +      message('Adding test qemu-iotests-' + format + '-' + item)

This message spoils the output during "configure" quite a bit, please remove 
that line.

Apart from that, patch looks fine to me!

  Thomas


> +      args = ['-tap', '-' + format, item,
> +              '--source-dir', meson.current_source_dir(),
> +              '--build-dir', meson.current_build_dir()]
> +      test('qemu-iotests-' + format + '-' + item,
> +           qemu_iotests_check_cmd,
> +           args: args,
> +           is_parallel: false,
> +           depends: qemu_iotests_binaries,
> +           env: qemu_iotests_env,
> +           protocol: 'tap',
> +           suite: suites)
> +  endforeach
>   endforeach

Re: [PATCH 0/5] iotests: make meson aware of individual I/O tests

[Daniel P. Berrangé]

On Fri, Mar 03, 2023 at 08:53:32AM +0000, Daniel P. Berrangé wrote:
> On Fri, Mar 03, 2023 at 09:30:39AM +0100, Thomas Huth wrote:
> > On 02/03/2023 19.46, Daniel P. Berrangé wrote:
> > > To just repeat the patch 5 description...
> > > 
> > > Currently meson registers a single test that invokes an entire group of
> > > I/O tests, hiding the test granularity from meson. There are various
> > > downsides of doing this
> > > 
> > >   * You cannot ask 'meson test' to invoke a single I/O test
> > >   * The meson test timeout can't be applied to the individual
> > >     tests
> > >   * Meson only gets a pass/fail for the overall I/O test group
> > >     not individual tests
> > >   * If a CI job gets killed by the GitLab timeout, we don't
> > >     get visibility into how far through the I/O tests
> > >     execution got.
> > > 
> > > This is not really specific to the I/O tests, the problem is common
> > > to any case of us running a test which is in fact another test
> > > harness which runs many tests. It would be nice to have meson have
> > > the full view of all tests run. Adapting the I/O tests is as easy
> > > win in this respect.
> > > 
> > > This switches meson to perform test discovery by invoking 'check' in
> > > dry-run mode. It then registers one meson test case for each I/O
> > > test. Parallel execution remains disabled since the I/O tests do not
> > > use self contained execution environments and thus conflict with
> > > each other.
> > 
> > Great to see some movement in this area again!
> > 
> > Some questions/remarks:
> > 
> > 1) Could you remove tests/check-block.sh now? See also:
> >    https://lore.kernel.org/all/20220209101530.3442837-9-thuth@redhat.com/
> 
> Possibly, I wasn't sure if that was wanted as a general entry
> point for humans, or was solely for meson ?
> 
> > 2) With regards to parallel execution ... I think it should be
> >    possible nowadays - the "check" script is normally also run
> >    with the "-j" switch by the tests/check-block.sh script, so
> >    if you remove the possibility to run in parallel, it's a
> >    regression from the previous behavior!
> 
> Hmmm, I got *masses* of test failures when running in parallel
> but I'll check again to be sure.

Ooooh, the 'check' script only uses a unique sub-dir for
tests when passing -j with a value >= 2. It also merely
appends the test name, so it is still broken for anyone
who runs multiple copies of 'check' in parallel for
different configurations, even if they passed -j. It
needs to just always use an isolated subdir no matter
what, with process level uniqueness, not merely test.

With 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 5/5] iotests: register each I/O test separately with meson

[Daniel P. Berrangé]

On Fri, Mar 03, 2023 at 10:34:11AM +0100, Thomas Huth wrote:
> On 02/03/2023 19.46, Daniel P. Berrangé wrote:
> > Currently meson registers a single test that invokes an entire group of
> > I/O tests, hiding the test granularity from meson. There are various
> > downsides of doing this
> > 
> >   * You cannot ask 'meson test' to invoke a single I/O test
> >   * The meson test timeout can't be applied to the individual
> >     tests
> >   * Meson only gets a pass/fail for the overall I/O test group
> >     not individual tests
> >   * If a CI job gets killed by the GitLab timeout, we don't
> >     get visibility into how far through the I/O tests
> >     execution got.
> > 
> > This switches meson to perform test discovery by invoking 'check' in
> > dry-run mode. It then registers one meson test case for each I/O
> > test. Parallel execution remains disabled since the I/O tests do not
> > use self contained execution environments and thus conflict with
> > each other.
> > 
> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> > ---
> >   tests/qemu-iotests/meson.build | 33 +++++++++++++++++++++++++++------
> >   1 file changed, 27 insertions(+), 6 deletions(-)
> > 
> > diff --git a/tests/qemu-iotests/meson.build b/tests/qemu-iotests/meson.build
> > index 323a4acb6a..48c82085af 100644
> > --- a/tests/qemu-iotests/meson.build
> > +++ b/tests/qemu-iotests/meson.build
> > @@ -32,16 +32,37 @@ foreach k, v : emulators
> >     endif
> >   endforeach
> > +qemu_iotests_check_cmd = files('check')
> > +
> >   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)
> > +
> > +  args = ['-tap', '-' + format]
> > +  if speed == 'quick'
> > +      args += ['-g', 'auto']
> > +  endif
> > +
> > +  rc = run_command(
> > +      [qemu_iotests_check_cmd] + args + ['-n'],
> > +      check: true,
> > +  )
> > +
> > +  foreach item: rc.stdout().strip().split()
> > +      message('Adding test qemu-iotests-' + format + '-' + item)
> 
> This message spoils the output during "configure" quite a bit, please remove
> that line.

Yes, that wasn't supposed to be left it, it was me debugging :-)

> 
> Apart from that, patch looks fine to me!
> 
>  Thomas
> 
> 
> > +      args = ['-tap', '-' + format, item,
> > +              '--source-dir', meson.current_source_dir(),
> > +              '--build-dir', meson.current_build_dir()]
> > +      test('qemu-iotests-' + format + '-' + item,
> > +           qemu_iotests_check_cmd,
> > +           args: args,
> > +           is_parallel: false,
> > +           depends: qemu_iotests_binaries,
> > +           env: qemu_iotests_env,
> > +           protocol: 'tap',
> > +           suite: suites)
> > +  endforeach
> >   endforeach
> 

With 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 0/5] iotests: make meson aware of individual I/O tests

[Thomas Huth]

On 03/03/2023 09.53, Daniel P. Berrangé wrote:
> On Fri, Mar 03, 2023 at 09:30:39AM +0100, Thomas Huth wrote:
>> On 02/03/2023 19.46, Daniel P. Berrangé wrote:
>>> To just repeat the patch 5 description...
>>>
>>> Currently meson registers a single test that invokes an entire group of
>>> I/O tests, hiding the test granularity from meson. There are various
>>> downsides of doing this
>>>
>>>    * You cannot ask 'meson test' to invoke a single I/O test
>>>    * The meson test timeout can't be applied to the individual
>>>      tests
>>>    * Meson only gets a pass/fail for the overall I/O test group
>>>      not individual tests
>>>    * If a CI job gets killed by the GitLab timeout, we don't
>>>      get visibility into how far through the I/O tests
>>>      execution got.
>>>
>>> This is not really specific to the I/O tests, the problem is common
>>> to any case of us running a test which is in fact another test
>>> harness which runs many tests. It would be nice to have meson have
>>> the full view of all tests run. Adapting the I/O tests is as easy
>>> win in this respect.
>>>
>>> This switches meson to perform test discovery by invoking 'check' in
>>> dry-run mode. It then registers one meson test case for each I/O
>>> test. Parallel execution remains disabled since the I/O tests do not
>>> use self contained execution environments and thus conflict with
>>> each other.
>>
>> Great to see some movement in this area again!
>>
>> Some questions/remarks:
>>
>> 1) Could you remove tests/check-block.sh now? See also:
>>     https://lore.kernel.org/all/20220209101530.3442837-9-thuth@redhat.com/
> 
> Possibly, I wasn't sure if that was wanted as a general entry
> point for humans, or was solely for meson ?

I think this script was only ever used for "make check-block", I never heard 
of anybody really using this script directly in a regular fashion. Humans 
rather run the tests/qemu-iotests/check script directly. Also see its origins:

  https://gitlab.com/qemu-project/qemu/-/commit/b8c6f29eb84cd3ccbbf

  HTH,
   Thomas

Re: [PATCH 0/5] iotests: make meson aware of individual I/O tests

[Daniel P. Berrangé]

On Fri, Mar 03, 2023 at 09:30:39AM +0100, Thomas Huth wrote:
> On 02/03/2023 19.46, Daniel P. Berrangé wrote:
> 3) When I tried this last year, I had a weird problem that
>    the terminal sometimes gets messed up ... I wasn't able
>    to track it down back then - could you check by running
>    "make check-block" many times (>10 times) to see whether
>    it happens with your series or not?

I've just seen this - echo got disabled on my terminal.

With 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/5] iotests: explicitly pass source/build dir to 'check' command

[Alex Bennée]

Daniel P. Berrangé <berrange@redhat.com> writes:

> The 'check' script has some rather dubious logic whereby it assumes
> that if invoked as a symlink, then it is running from a separate
> source tree and build tree, otherwise it assumes the current working
> directory is a combined source and build tree.
>
> This doesn't work if you want to invoke the 'check' script using
> its full source tree path while still using a split source and build
> tree layout. This would be a typical situation with meson if you ask
> it to find the 'check' script path using files('check').
>
> Rather than trying to make the logic more magical, add support for
> explicitly passing the dirs using --source-dir and --build-dir. If
> either is omitted the current logic is maintained.
>
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>  tests/qemu-iotests/check      |  8 +++++++-
>  tests/qemu-iotests/testenv.py | 17 +++++++++++++----
>  2 files changed, 20 insertions(+), 5 deletions(-)
>
> diff --git a/tests/qemu-iotests/check b/tests/qemu-iotests/check
> index 9bdda1394e..806abc21d6 100755
> --- a/tests/qemu-iotests/check
> +++ b/tests/qemu-iotests/check
> @@ -113,6 +113,10 @@ def make_argparser() -> argparse.ArgumentParser:
>                         'middle of the process.')
>      g_sel.add_argument('tests', metavar='TEST_FILES', nargs='*',
>                         help='tests to run, or "--" followed by a command')
> +    g_sel.add_argument('--build-dir', default=None,
> +                       help='Path to iotests build directory')
> +    g_sel.add_argument('--source-dir', default=None,
> +                       help='Path to iotests build directory')
>

I'd be tempted to push all the automagic stuff into the options so you
have something like:

--8<---------------cut here---------------start------------->8---
modified   tests/qemu-iotests/check
@@ -27,8 +27,23 @@ from testenv import TestEnv
 from testrunner import TestRunner
 
 
+def get_default_path(follow_link=False):
+    """
+    Try to automagically figure out the path we are running from.
+    """
+    # called from the build tree?
+    if os.path.islink(sys.argv[0]):
+        if follow_link:
+            return os.path.dirname(os.readlink(sys.argv[0]))
+        else:
+            return os.path.dirname(os.path.abspath(sys.argv[0]))
+    else:  # or source tree?
+        return os.getcwd()
+
+
 def make_argparser() -> argparse.ArgumentParser:
-    p = argparse.ArgumentParser(description="Test run options")
+    p = argparse.ArgumentParser(description="Test run options",
+                                formatter_class=argparse.ArgumentDefaultsHelpFormatter)
 
     p.add_argument('-n', '--dry-run', action='store_true',
                    help='show me, do not run tests')
@@ -113,9 +128,9 @@ def make_argparser() -> argparse.ArgumentParser:
                        'middle of the process.')
     g_sel.add_argument('tests', metavar='TEST_FILES', nargs='*',
                        help='tests to run, or "--" followed by a command')
-    g_sel.add_argument('--build-dir', default=None,
+    g_sel.add_argument('--build-dir', default=get_default_path(),
                        help='Path to iotests build directory')
-    g_sel.add_argument('--source-dir', default=None,
+    g_sel.add_argument('--source-dir', default=get_default_path(follow_link=True),
                        help='Path to iotests build directory')
 
     return p
modified   tests/qemu-iotests/testenv.py
@@ -213,23 +213,8 @@ def __init__(self, imgfmt: str, imgproto: str, aiomode: str,
         # Initialize generic paths: build_root, build_iotests, source_iotests,
         # which are needed to initialize some environment variables. They are
         # used by init_*() functions as well.
-
-        if os.path.islink(sys.argv[0]):
-            # called from the build tree
-            self.source_iotests = os.path.dirname(
-                os.readlink(sys.argv[0]))
-            self.build_iotests = os.path.dirname(
-                os.path.abspath(sys.argv[0]))
-        else:
-            # called from the source tree
-            self.source_iotests = os.getcwd()
-            self.build_iotests = os.getcwd()
-
-        if source_dir is not None:
-            self.source_iotests = source_dir
-        if build_dir is not None:
-            self.build_iotests = build_dir
-
+        self.source_iotests = source_dir
+        self.build_iotests = build_dir
         self.build_root = os.path.join(self.build_iotests, '..', '..')
 
         self.init_directories()
--8<---------------cut here---------------end--------------->8---


>      return p
>  
> @@ -124,7 +128,9 @@ if __name__ == '__main__':
>                    aiomode=args.aiomode, cachemode=args.cachemode,
>                    imgopts=args.imgopts, misalign=args.misalign,
>                    debug=args.debug, valgrind=args.valgrind,
> -                  gdb=args.gdb, qprint=args.print)
> +                  gdb=args.gdb, qprint=args.print,
> +                  source_dir=args.source_dir,
> +                  build_dir=args.build_dir)
>  
>      if len(sys.argv) > 1 and sys.argv[-len(args.tests)-1] == '--':
>          if not args.tests:
> diff --git a/tests/qemu-iotests/testenv.py b/tests/qemu-iotests/testenv.py
> index a864c74b12..9bf37cd381 100644
> --- a/tests/qemu-iotests/testenv.py
> +++ b/tests/qemu-iotests/testenv.py
> @@ -177,7 +177,9 @@ def __init__(self, imgfmt: str, imgproto: str, aiomode: str,
>                   debug: bool = False,
>                   valgrind: bool = False,
>                   gdb: bool = False,
> -                 qprint: bool = False) -> None:
> +                 qprint: bool = False,
> +                 source_dir: Optional[str] = None,
> +                 build_dir: Optional[str] = None) -> None:
>          self.imgfmt = imgfmt
>          self.imgproto = imgproto
>          self.aiomode = aiomode
> @@ -213,12 +215,19 @@ def __init__(self, imgfmt: str, imgproto: str, aiomode: str,
>  
>          if os.path.islink(sys.argv[0]):
>              # called from the build tree
> -            self.source_iotests = os.path.dirname(os.readlink(sys.argv[0]))
> -            self.build_iotests = os.path.dirname(os.path.abspath(sys.argv[0]))
> +            self.source_iotests = os.path.dirname(
> +                os.readlink(sys.argv[0]))
> +            self.build_iotests = os.path.dirname(
> +                os.path.abspath(sys.argv[0]))
>          else:
>              # called from the source tree
>              self.source_iotests = os.getcwd()
> -            self.build_iotests = self.source_iotests
> +            self.build_iotests = os.getcwd()
> +
> +        if source_dir is not None:
> +            self.source_iotests = source_dir
> +        if build_dir is not None:
> +            self.build_iotests = build_dir
>  
>          self.build_root = os.path.join(self.build_iotests, '..', '..')


-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [PATCH 2/5] iotests: allow test discovery before building

[Alex Bennée]

Daniel P. Berrangé <berrange@redhat.com> writes:

> The 'check' script can be invoked in "dry run" mode, in which case it
> merely does test discovery and prints out all their names. Despite only
> doing test discovery it still validates that the various QEMU binaries
> can be found. This makes it impossible todo test discovery prior to
> building QEMU. This is a desirable feature to support, because it will
> let meson discover tests.
>
> Fortunately the code in the TestEnv constructor is ordered in a way
> that makes this fairly trivial to achieve. We can just short circuit
> the constructor after the basic directory paths have been set.
>
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [PATCH 3/5] iotests: strip subdir path when listing tests

[Alex Bennée]

Daniel P. Berrangé <berrange@redhat.com> writes:

> When asking 'check' to list individual tests by invoking it in dry run
> mode, it prints the paths to the tests relative to the base of the
> I/O test directory.
>
> When asking 'check' to run an individual test, however, it mandates that
> only the unqualified test name is given, without any path prefix. This
> inconsistency makes it harder to ask for a list of tests and then invoke
> each one.
>
> Thus the test listing code is change to flatten the test names, by
> printing only the base name, which can be directly invoked.
>
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [PATCH 4/5] iotests: print TAP protocol version when reporting tests

[Alex Bennée]

Daniel P. Berrangé <berrange@redhat.com> writes:

> Recently meson started complaining that TAP test reports don't include
> the TAP protocol version. While this warning is bogus and has since been
> removed from Meson, it looks like good practice to include this header
> going forward. The GLib library test harness has started unconditionally
> printing the version, so this brings the I/O tests into line.
>
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [PATCH 1/5] iotests: explicitly pass source/build dir to 'check' command

[Daniel P. Berrangé]

On Fri, Mar 03, 2023 at 12:55:26PM +0000, Alex Bennée wrote:
> 
> Daniel P. Berrangé <berrange@redhat.com> writes:
> 
> > The 'check' script has some rather dubious logic whereby it assumes
> > that if invoked as a symlink, then it is running from a separate
> > source tree and build tree, otherwise it assumes the current working
> > directory is a combined source and build tree.
> >
> > This doesn't work if you want to invoke the 'check' script using
> > its full source tree path while still using a split source and build
> > tree layout. This would be a typical situation with meson if you ask
> > it to find the 'check' script path using files('check').
> >
> > Rather than trying to make the logic more magical, add support for
> > explicitly passing the dirs using --source-dir and --build-dir. If
> > either is omitted the current logic is maintained.
> >
> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> > ---
> >  tests/qemu-iotests/check      |  8 +++++++-
> >  tests/qemu-iotests/testenv.py | 17 +++++++++++++----
> >  2 files changed, 20 insertions(+), 5 deletions(-)
> >
> > diff --git a/tests/qemu-iotests/check b/tests/qemu-iotests/check
> > index 9bdda1394e..806abc21d6 100755
> > --- a/tests/qemu-iotests/check
> > +++ b/tests/qemu-iotests/check
> > @@ -113,6 +113,10 @@ def make_argparser() -> argparse.ArgumentParser:
> >                         'middle of the process.')
> >      g_sel.add_argument('tests', metavar='TEST_FILES', nargs='*',
> >                         help='tests to run, or "--" followed by a command')
> > +    g_sel.add_argument('--build-dir', default=None,
> > +                       help='Path to iotests build directory')
> > +    g_sel.add_argument('--source-dir', default=None,
> > +                       help='Path to iotests build directory')
> >
> 
> I'd be tempted to push all the automagic stuff into the options so you
> have something like:

Hmm, yes, that's a nice idea, will give it a go as you suggest.


With 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 0/5] iotests: make meson aware of individual I/O tests

[Daniel P. Berrangé]

On Fri, Mar 03, 2023 at 10:45:40AM +0000, Daniel P. Berrangé wrote:
> On Fri, Mar 03, 2023 at 09:30:39AM +0100, Thomas Huth wrote:
> > On 02/03/2023 19.46, Daniel P. Berrangé wrote:
> > 3) When I tried this last year, I had a weird problem that
> >    the terminal sometimes gets messed up ... I wasn't able
> >    to track it down back then - could you check by running
> >    "make check-block" many times (>10 times) to see whether
> >    it happens with your series or not?
> 
> I've just seen this - echo got disabled on my terminal.

The problem is that testrunner.py script doing

# We want to save current tty settings during test run,
# since an aborting qemu call may leave things screwed up.
@contextmanager
def savetty() -> Iterator[None]:
    isterm = sys.stdin.isatty()
    if isterm:
        fd = sys.stdin.fileno()
        attr = termios.tcgetattr(fd)

    try:
        yield
    finally:
        if isterm:
            termios.tcsetattr(fd, termios.TCSADRAIN, attr)


When invoking 'check' this wraps around execution of the entire
'check' script. IOW it saves/restores the terminal once.

When we invoke 'check' in parallel it will save/restore the same
terminal for each invokation.

If the 'termios.tcgetattr' call runs at the same time as there is
a QEMU running which has put the terminal in raw mode, then when
'check' exits it'll "restore" the terminal to raw mode.

IOW, this savetty() logic is fundamentally unsafe when invoking
'check' in parallel.

AFAICT, the more reliable thing todo would be to spawn a new PTY
when invoking each test, so there's no cleanup required. If QEMU
aborts leaving the TTY messed up it dosn't matter was it was a
temporary throwaway PTY.


With 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 0/5] iotests: make meson aware of individual I/O tests

[Thomas Huth]

On 03/03/2023 14.06, Daniel P. Berrangé wrote:
> On Fri, Mar 03, 2023 at 10:45:40AM +0000, Daniel P. Berrangé wrote:
>> On Fri, Mar 03, 2023 at 09:30:39AM +0100, Thomas Huth wrote:
>>> On 02/03/2023 19.46, Daniel P. Berrangé wrote:
>>> 3) When I tried this last year, I had a weird problem that
>>>     the terminal sometimes gets messed up ... I wasn't able
>>>     to track it down back then - could you check by running
>>>     "make check-block" many times (>10 times) to see whether
>>>     it happens with your series or not?
>>
>> I've just seen this - echo got disabled on my terminal.
> 
> The problem is that testrunner.py script doing
> 
> # We want to save current tty settings during test run,
> # since an aborting qemu call may leave things screwed up.
> @contextmanager
> def savetty() -> Iterator[None]:
>      isterm = sys.stdin.isatty()
>      if isterm:
>          fd = sys.stdin.fileno()
>          attr = termios.tcgetattr(fd)
> 
>      try:
>          yield
>      finally:
>          if isterm:
>              termios.tcsetattr(fd, termios.TCSADRAIN, attr)
> 
> 
> When invoking 'check' this wraps around execution of the entire
> 'check' script. IOW it saves/restores the terminal once.
> 
> When we invoke 'check' in parallel it will save/restore the same
> terminal for each invokation.
> 
> If the 'termios.tcgetattr' call runs at the same time as there is
> a QEMU running which has put the terminal in raw mode, then when
> 'check' exits it'll "restore" the terminal to raw mode.
> 
> IOW, this savetty() logic is fundamentally unsafe when invoking
> 'check' in parallel.

Hmm, couldn't we e.g. also simply skip this termios stuff when running with 
"--tap" ?

  Thomas

Re: [PATCH 0/5] iotests: make meson aware of individual I/O tests

[Daniel P. Berrangé]

On Fri, Mar 03, 2023 at 04:49:36PM +0100, Thomas Huth wrote:
> On 03/03/2023 14.06, Daniel P. Berrangé wrote:
> > On Fri, Mar 03, 2023 at 10:45:40AM +0000, Daniel P. Berrangé wrote:
> > > On Fri, Mar 03, 2023 at 09:30:39AM +0100, Thomas Huth wrote:
> > > > On 02/03/2023 19.46, Daniel P. Berrangé wrote:
> > > > 3) When I tried this last year, I had a weird problem that
> > > >     the terminal sometimes gets messed up ... I wasn't able
> > > >     to track it down back then - could you check by running
> > > >     "make check-block" many times (>10 times) to see whether
> > > >     it happens with your series or not?
> > > 
> > > I've just seen this - echo got disabled on my terminal.
> > 
> > The problem is that testrunner.py script doing
> > 
> > # We want to save current tty settings during test run,
> > # since an aborting qemu call may leave things screwed up.
> > @contextmanager
> > def savetty() -> Iterator[None]:
> >      isterm = sys.stdin.isatty()
> >      if isterm:
> >          fd = sys.stdin.fileno()
> >          attr = termios.tcgetattr(fd)
> > 
> >      try:
> >          yield
> >      finally:
> >          if isterm:
> >              termios.tcsetattr(fd, termios.TCSADRAIN, attr)
> > 
> > 
> > When invoking 'check' this wraps around execution of the entire
> > 'check' script. IOW it saves/restores the terminal once.
> > 
> > When we invoke 'check' in parallel it will save/restore the same
> > terminal for each invokation.
> > 
> > If the 'termios.tcgetattr' call runs at the same time as there is
> > a QEMU running which has put the terminal in raw mode, then when
> > 'check' exits it'll "restore" the terminal to raw mode.
> > 
> > IOW, this savetty() logic is fundamentally unsafe when invoking
> > 'check' in parallel.
> 
> Hmm, couldn't we e.g. also simply skip this termios stuff when running with
> "--tap" ?

It actually turns out to be way simpler. We merely need to set
stdin=subprocess.DEVNULL. There is no valid reason for the test
cases to be reading for stdin, as that would block execution.
By setting stdin==/dev/null, the isatty(0) checks will fail and
thus QEMU won't mess with termios, and thus we don't need any
save/restore dance either.


With 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 :|