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

From: Max Reitz <mreitz@redhat.com>
To: Emanuele Giuseppe Esposito <eesposit@redhat.com>, qemu-block@nongnu.org
Cc: Kevin Wolf <kwolf@redhat.com>,
	Eduardo Habkost <ehabkost@redhat.com>,
	qemu-devel@nongnu.org, Cleber Rosa <crosa@redhat.com>,
	Paolo Bonzini <pbonzini@redhat.com>, John Snow <jsnow@redhat.com>
Subject: Re: [PATCH v3 14/15] qemu_iotests: add option to show qemu binary logs on stdout
Date: Mon, 3 May 2021 17:03:53 +0200	[thread overview]
Message-ID: <12cbc080-6c82-f90c-77ae-fea442fbbf14@redhat.com> (raw)
In-Reply-To: <8431130c-0929-1ca5-a406-633534565f2c@redhat.com>

On 30.04.21 23:04, Emanuele Giuseppe Esposito wrote:
> 
> 
> 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.

Understood.

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

Thanks, looking forward to it! :)

Max