Re: [PATCH v3 14/15] qemu_iotests: add option to show qemu binary logs on stdout

[Emanuele Giuseppe Esposito <eesposit@redhat.com>]

On 30/04/2021 15:50, Max Reitz wrote:
> On 14.04.21 19:03, Emanuele Giuseppe Esposito wrote:
>> Using the flag -p, allow the qemu binary to print to stdout.
>> This helps especially when doing print-debugging.
> 
> I think this shouldn’t refer to prints but to qemu’s stdout/stderr in 
> general, i.e....
> 
>> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
>> ---
>>   tests/qemu-iotests/check      | 3 ++-
>>   tests/qemu-iotests/iotests.py | 9 +++++++++
>>   tests/qemu-iotests/testenv.py | 9 +++++++--
>>   3 files changed, 18 insertions(+), 3 deletions(-)
>>
>> diff --git a/tests/qemu-iotests/check b/tests/qemu-iotests/check
>> index 489178d9a4..84483922eb 100755
>> --- a/tests/qemu-iotests/check
>> +++ b/tests/qemu-iotests/check
>> @@ -33,6 +33,7 @@ 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='shows qemu binary prints to stdout')
> 
> I’d prefer for the description to be “redirects qemu's stdout and stderr 
> to the test output”.
> 
> Also, this line is too long.
> 
>>       p.add_argument('-gdb', action='store_true',
>>                      help="start gdbserver with $GDB_QEMU options. \
>>                            Default is localhost:12345")
>> @@ -117,7 +118,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 f9832558a0..52ff7332f8 100644
>> --- a/tests/qemu-iotests/iotests.py
>> +++ b/tests/qemu-iotests/iotests.py
>> @@ -79,6 +79,8 @@
>>   if os.environ.get('GDB_QEMU'):
>>       qemu_gdb = ['gdbserver'] + 
>> os.environ.get('GDB_QEMU').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', '.')
>> @@ -621,6 +623,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 != None:
> 
> I think "is not None" is mostly used in Python.  (I’m saying this in 
> this weird way because I’m not the one to ask what’s mostly used in 
> Python...)
> 
> (That also silences mypy, which otherwise complains and then fails 297.)
> 
>> +            # set QEMU binary output to stdout
>> +            self._qemu_log_file.close()
>> +            self._qemu_log_file = None
> 
> I don’t know enough Python to know how this works.  I suppose this does 
> access the super class’s member?  (I could also imagine it creates a new 
> member, just for this child class, but that then effectively overwrites 
> the super class’s member.)
> 
> Considering _qemu_log_file is apparently meant to be a private attribute 
> (from the leading underscore), I think it would be better to have a 
> function provided by the super class for this.

It should access the superclass member, and it's the same that 
_post_shutdown does. I can make a function for that.

Regarding all other feedback in all patches that I did not answer: agree 
on all of them, will adjust everything in v4.

Thank you,
Emanuele

> 
>> +
>>       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 39ae7ace33..6ae099114e 100644
>> --- a/tests/qemu-iotests/testenv.py
>> +++ b/tests/qemu-iotests/testenv.py
>> @@ -73,7 +73,7 @@ class TestEnv(ContextManager['TestEnv']):
>>                        'AIOMODE', 'CACHEMODE', 'VALGRIND_QEMU',
>>                        'CACHEMODE_IS_DEFAULT', 'IMGFMT_GENERIC', 
>> 'IMGOPTSSYNTAX',
>>                        'IMGKEYSECRET', 'QEMU_DEFAULT_MACHINE', 
>> 'MALLOC_PERTURB_',
>> -                     'GDB_QEMU']
>> +                     'GDB_QEMU', 'PRINT_QEMU']
>>       def get_env(self) -> Dict[str, str]:
>>           env = {}
>> @@ -165,7 +165,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
>> @@ -173,6 +174,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_qemu = os.environ.get('GDB_QEMU', 
>> 'localhost:12345')
>>           elif 'GDB_QEMU' in os.environ:
>> @@ -278,6 +282,7 @@ def print_env(self) -> None:
>>   SOCKET_SCM_HELPER -- {SOCKET_SCM_HELPER}
>>   GDB_QEMU      -- "{GDB_QEMU}"
>>   VALGRIND_QEMU     -- "{VALGRIND_QEMU}"
>> +PRINT_QEMU    --  "{PRINT_QEMU}"
> 
> Again, I personally don’t like the quotes.
> 
> (I think “PRINT_QEMU -- y” and “PRINT_QEMU --” look better.)
> 
> Another thing: Here in this place, the name of the environment variable 
> is visible.  “PRINT_QEMU” doesn’t really have meaning; now, if it 
> weren’t visible, I wouldn’t care, but here it is visible.  Perhaps 
> “PRINT_QEMU_OUTPUT”, or "SHOW_QEMU_OUTPUT" would mean more?
> 
> Well, it’s all bike-shedding, so with “is not None” and the add_argument 
> line broken so it doesn’t exceed 79 characters:
> 
> Reviewed-by: Max Reitz <mreitz@redhat.com>
> 
>>   """
>>           args = collections.defaultdict(str, self.get_env())
>>
>