[prev in list] [next in list] [prev in thread] [next in thread]
List: qemu-block
Subject: Re: [PATCH v3 14/15] qemu_iotests: add option to show qemu binary logs on stdout
From: Emanuele Giuseppe Esposito <eesposit () redhat ! com>
Date: 2021-04-30 21:04:10
Message-ID: 8431130c-0929-1ca5-a406-633534565f2c () redhat ! com
[Download RAW message or body]
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())
>>
>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic