From: Emanuele Giuseppe Esposito <eesposit@redhat.com>
To: Max Reitz <mreitz@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: Fri, 30 Apr 2021 23:04:10 +0200 [thread overview]
Message-ID: <8431130c-0929-1ca5-a406-633534565f2c@redhat.com> (raw)
In-Reply-To: <2935fed5-4330-bdb1-6062-7d4a7ed05893@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())
>>
>
next prev parent reply other threads:[~2021-04-30 21:43 UTC|newest]
Thread overview: 50+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-04-14 17:03 [PATCH v3 00/15] qemu_iotests: improve debugging options Emanuele Giuseppe Esposito
2021-04-14 17:03 ` [PATCH v3 01/15] python: qemu: add timer parameter for qmp.accept socket Emanuele Giuseppe Esposito
2021-04-30 11:23 ` Max Reitz
2021-05-13 17:54 ` John Snow
2021-05-14 8:16 ` Emanuele Giuseppe Esposito
2021-04-14 17:03 ` [PATCH v3 02/15] python: qemu: pass the wrapper field from QEMUQtestmachine to QEMUMachine Emanuele Giuseppe Esposito
2021-04-30 11:23 ` Max Reitz
2021-05-13 17:55 ` John Snow
2021-04-14 17:03 ` [PATCH v3 03/15] docs/devel/testing: add debug section to the QEMU iotests chapter Emanuele Giuseppe Esposito
2021-04-30 11:23 ` Max Reitz
2021-04-30 14:07 ` Paolo Bonzini
2021-04-14 17:03 ` [PATCH v3 04/15] qemu-iotests: add option to attach gdbserver Emanuele Giuseppe Esposito
2021-04-30 11:38 ` Max Reitz
2021-04-30 21:03 ` Emanuele Giuseppe Esposito
2021-05-03 14:38 ` Max Reitz
2021-04-30 12:03 ` Max Reitz
2021-04-14 17:03 ` [PATCH v3 05/15] qemu-iotests: delay QMP socket timers Emanuele Giuseppe Esposito
2021-04-30 11:59 ` Max Reitz
2021-04-30 21:03 ` Emanuele Giuseppe Esposito
2021-05-03 15:02 ` Max Reitz
2021-05-13 18:20 ` John Snow
2021-04-14 17:03 ` [PATCH v3 06/15] qemu_iotests: insert gdbserver command line as wrapper for qemu binary Emanuele Giuseppe Esposito
2021-04-30 12:05 ` Max Reitz
2021-04-14 17:03 ` [PATCH v3 07/15] qemu-iotests: add gdbserver option to script tests too Emanuele Giuseppe Esposito
2021-04-30 12:17 ` Max Reitz
2021-04-14 17:03 ` [PATCH v3 08/15] docs/devel/testing: add -gdb option to the debugging section of QEMU iotests Emanuele Giuseppe Esposito
2021-04-30 12:27 ` Max Reitz
2021-04-14 17:03 ` [PATCH v3 09/15] qemu_iotests: extend the check script to support valgrind for python tests Emanuele Giuseppe Esposito
2021-04-30 12:45 ` Max Reitz
2021-04-14 17:03 ` [PATCH v3 10/15] qemu_iotests: extent QMP socket timeout when using valgrind Emanuele Giuseppe Esposito
2021-04-30 13:02 ` Max Reitz
2021-04-30 21:03 ` Emanuele Giuseppe Esposito
2021-05-13 18:47 ` John Snow
2021-05-14 8:16 ` Emanuele Giuseppe Esposito
2021-05-14 20:02 ` John Snow
2021-05-18 13:58 ` Emanuele Giuseppe Esposito
2021-05-18 14:26 ` John Snow
2021-05-18 18:20 ` Emanuele Giuseppe Esposito
2021-04-14 17:03 ` [PATCH v3 11/15] qemu_iotests: allow valgrind to read/delete the generated log file Emanuele Giuseppe Esposito
2021-04-30 13:17 ` Max Reitz
2021-04-14 17:03 ` [PATCH v3 12/15] qemu_iotests: insert valgrind command line as wrapper for qemu binary Emanuele Giuseppe Esposito
2021-04-30 13:20 ` Max Reitz
2021-04-14 17:03 ` [PATCH v3 13/15] docs/devel/testing: add -valgrind option to the debug section of QEMU iotests Emanuele Giuseppe Esposito
2021-04-30 13:24 ` Max Reitz
2021-04-14 17:03 ` [PATCH v3 14/15] qemu_iotests: add option to show qemu binary logs on stdout Emanuele Giuseppe Esposito
2021-04-30 13:50 ` Max Reitz
2021-04-30 21:04 ` Emanuele Giuseppe Esposito [this message]
2021-05-03 15:03 ` Max Reitz
2021-04-14 17:03 ` [PATCH v3 15/15] docs/devel/testing: add -p option to the debug section of QEMU iotests Emanuele Giuseppe Esposito
2021-04-30 13:55 ` Max Reitz
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=8431130c-0929-1ca5-a406-633534565f2c@redhat.com \
--to=eesposit@redhat.com \
--cc=crosa@redhat.com \
--cc=ehabkost@redhat.com \
--cc=jsnow@redhat.com \
--cc=kwolf@redhat.com \
--cc=mreitz@redhat.com \
--cc=pbonzini@redhat.com \
--cc=qemu-block@nongnu.org \
--cc=qemu-devel@nongnu.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.