Re: [PATCH v4 14/15] qemu-iotests: add option to show qemu binary logs on stdout

[Vladimir Sementsov-Ogievskiy]

20.05.2021 10:52, Emanuele Giuseppe Esposito wrote:
> Using the flag -p, allow the qemu binary to print to stdout.
>
> Reviewed-by: Max Reitz <mreitz@redhat.com>
> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> ---
>   tests/qemu-iotests/check      | 4 +++-
>   tests/qemu-iotests/iotests.py | 9 +++++++++
>   tests/qemu-iotests/testenv.py | 9 +++++++--
>   3 files changed, 19 insertions(+), 3 deletions(-)
>
> diff --git a/tests/qemu-iotests/check b/tests/qemu-iotests/check
> index 2101cedfe3..51b90681ab 100755
> --- a/tests/qemu-iotests/check
> +++ b/tests/qemu-iotests/check
> @@ -33,6 +33,8 @@ def make_argparser() -> argparse.ArgumentParser:
>                      help='pretty print output for make check')
>   
>       p.add_argument('-d', dest='debug', action='store_true', help='debug')
> +    p.add_argument('-p', dest='print', action='store_true',
> +                help='redirects qemu\'s stdout and stderr to the test output')
>       p.add_argument('-gdb', action='store_true',
>                      help="start gdbserver with $GDB_OPTIONS options \
>                           ('localhost:12345' if $GDB_OPTIONS is empty)")
> @@ -117,7 +119,7 @@ if __name__ == '__main__':
>                     aiomode=args.aiomode, cachemode=args.cachemode,
>                     imgopts=args.imgopts, misalign=args.misalign,
>                     debug=args.debug, valgrind=args.valgrind,
> -                  gdb=args.gdb)
> +                  gdb=args.gdb, qprint=args.print)
>   
>       testfinder = TestFinder(test_dir=env.source_iotests)
>   
> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
> index 75f1e1711c..53a3916a91 100644
> --- a/tests/qemu-iotests/iotests.py
> +++ b/tests/qemu-iotests/iotests.py
> @@ -80,6 +80,8 @@
>   if gdb_qemu_env:
>       qemu_gdb = ['gdbserver'] + gdb_qemu_env.strip().split(' ')
>   
> +qemu_print = os.environ.get('PRINT_QEMU', False)
> +
>   imgfmt = os.environ.get('IMGFMT', 'raw')
>   imgproto = os.environ.get('IMGPROTO', 'file')
>   output_dir = os.environ.get('OUTPUT_DIR', '.')
> @@ -614,6 +616,13 @@ def _post_shutdown(self) -> None:
>           super()._post_shutdown()
>           self.subprocess_check_valgrind(qemu_valgrind)
>   
> +    def _pre_launch(self) -> None:
> +        super()._pre_launch()
> +        if qemu_print and self._qemu_log_file is not None:
> +            # set QEMU binary output to stdout
> +            self._qemu_log_file.close()
> +            self._qemu_log_file = None
> +

So, many use of _private members actually show that proper way of doing this is 
adding an option to __init__ instead

Interesting will pylint complain on using _private members outside of the home 
class?

>       def add_object(self, opts):
>           self._args.append('-object')
>           self._args.append(opts)
> diff --git a/tests/qemu-iotests/testenv.py b/tests/qemu-iotests/testenv.py
> index 319d29cb0c..b79ce22fe9 100644
> --- a/tests/qemu-iotests/testenv.py
> +++ b/tests/qemu-iotests/testenv.py
> @@ -74,7 +74,7 @@ class TestEnv(ContextManager['TestEnv']):
>                        'AIOMODE', 'CACHEMODE', 'VALGRIND_QEMU',
>                        'CACHEMODE_IS_DEFAULT', 'IMGFMT_GENERIC', 
> 'IMGOPTSSYNTAX',
>                        'IMGKEYSECRET', 'QEMU_DEFAULT_MACHINE', 
> 'MALLOC_PERTURB_',
> -                     'GDB_OPTIONS']
> +                     'GDB_OPTIONS', 'PRINT_QEMU']
>   
>       def get_env(self) -> Dict[str, str]:
>           env = {}
> @@ -166,7 +166,8 @@ def __init__(self, imgfmt: str, imgproto: str, aiomode: str,
>                    misalign: bool = False,
>                    debug: bool = False,
>                    valgrind: bool = False,
> -                 gdb: bool = False) -> None:
> +                 gdb: bool = False,
> +                 qprint: bool = False) -> None:
>           self.imgfmt = imgfmt
>           self.imgproto = imgproto
>           self.aiomode = aiomode
> @@ -174,6 +175,9 @@ def __init__(self, imgfmt: str, imgproto: str, aiomode: str,
>           self.misalign = misalign
>           self.debug = debug
>   
> +        if qprint:
> +            self.print_qemu = 'y'
> +
>           if gdb:
>               self.gdb_options = os.environ.get('GDB_OPTIONS', DEF_GDB_OPTIONS)
>               if not self.gdb_options:
> @@ -283,6 +287,7 @@ def print_env(self) -> None:
>   SOCKET_SCM_HELPER -- {SOCKET_SCM_HELPER}
>   GDB_OPTIONS   -- {GDB_OPTIONS}
>   VALGRIND_QEMU -- {VALGRIND_QEMU}
> +PRINT_QEMU_OUTPUT -- {PRINT_QEMU}
>   """
>   
>           args = collections.defaultdict(str, self.get_env())

5 places we need to modify to add a new option. That's not very good :( (not 
about your patch).

And I still doubt, that we want to add any new variable to print_env. If we do, 
than it's simpler to print all variables here, than, one place less to modify 
by hand.


Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

--
Best regards,
Vladimir