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 05/15] qemu-iotests: delay QMP socket timers
Date: Mon, 3 May 2021 17:02:40 +0200 [thread overview]
Message-ID: <d652828c-a5c9-8f61-84b8-0f1d8a679911@redhat.com> (raw)
In-Reply-To: <bb2c8f3c-f1cf-6213-b67d-7b1ff2102992@redhat.com>
On 30.04.21 23:03, Emanuele Giuseppe Esposito wrote:
>
>
> On 30/04/2021 13:59, Max Reitz wrote:
>> On 14.04.21 19:03, Emanuele Giuseppe Esposito wrote:
>>> Attaching a gdbserver implies that the qmp socket
>>> should wait indefinitely for an answer from QEMU.
>>>
>>> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
>>> ---
>>> python/qemu/machine.py | 3 +++
>>> tests/qemu-iotests/iotests.py | 10 +++++++++-
>>> 2 files changed, 12 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/python/qemu/machine.py b/python/qemu/machine.py
>>> index 12752142c9..d6142271c2 100644
>>> --- a/python/qemu/machine.py
>>> +++ b/python/qemu/machine.py
>>> @@ -409,6 +409,9 @@ def _launch(self) -> None:
>>> stderr=subprocess.STDOUT,
>>> shell=False,
>>> close_fds=False)
>>> +
>>> + if 'gdbserver' in self._wrapper:
>>> + self._qmp_timer = None
>>
>> Why doesn’t __init__() evaluate this? This here doesn’t feel like the
>> right place for it. If we want to evaluate it here, self._qmp_timer
>> shouldn’t exist, and instead the timeout should be a _post_launch()
>> parameter. (Which I would have nothing against, by the way.)
>
> Uhm.. I got another comment in a previous version where for the "event"
> callbacks it was better a property than passing around a parameter.
> Which I honestly agree.
I think that comment was in the sense of providing a default value,
which can be expressed by having a property that is set in __init__.
I don’t have anything against making this a property, but I also don’t
have anything against making it a _post_launch() parameter. I could
even live with both, i.e. set _qmp_timer to 15 in __init__, then have a
_post_launch parameter, and pass either self._qmp_timer or None if
self._wrapper includes 'gdbserver'.
What I do mind is that I don’t understand why the property is modified
here. The value of self._qmp_timer is supposed to be 15 by default and
None if self._wrapper includes 'gdbserver'. It should thus be changed
to None the moment self._wrapper is made to include 'gdbserver'.
Because self._wrapper is set only in __init__, this should happen in
__init__.
> What should __init__() do? The check here is to see if the invocation
> has gdb (and a couple of patches ahead also valgrind), to remove the timer.
> If I understand what you mean, you want something like
> def __init__(self, timer):
Oh, no. We can optionally do that perhaps later, but what I meant is
just to put this in __init__() (without adding any parameters to it):
self._qmp_timer = 15.0 if 'gdbserver' not in self._wrapper else None
I think self._qmp_timer should always reflect what timeout we are going
to use when a VM is launched. So if the conditions influencing the
timeout change, it should be updated immediately to reflect this. The
only condition we have right now is the content of self._wrapper, which
is only set in __init__, so self._qmp_timer should be set once in
__init__ and not changed afterwards.
That sounds academic, but imagine what would happen if we had a
set_qmp_timer() method: The timout could be adjusted, but launch() would
just ignore it and update the property, even though the conditions
influencing the timout didn’t change between set_qmp_timer() and launch().
Or if we had a get_qmp_timer(); a caller would read a timeout of 15.0
before launch(), even though the timeout is going to be None.
Therefore, I think a property should not be updated just before it is
read, but instead when any condition that’s supposed to influence its
value changes.
I suggested making it a parameter because updating a property when
reading it sounds like it should be a parameter instead. I.e., one
would say
def __init__():
self._qmp_timeout_default = 15.0
def post_launch(qmp_timeout):
self._qmp.accept(qmp_timeout)
def launch(self):
...
qmp_timeout = None if 'gdbserver' in self._wrapper \
else self._qmp_timout_default
self.post_launch(qmp_timeout)
Which is basically the structure your patch has, which gave me the idea.
[...]
>>> self._post_launch()
>>> def _early_cleanup(self) -> None:
>>> diff --git a/tests/qemu-iotests/iotests.py
>>> b/tests/qemu-iotests/iotests.py
>>> index 05d0dc0751..380527245e 100644
>>> --- a/tests/qemu-iotests/iotests.py
>>> +++ b/tests/qemu-iotests/iotests.py
[...]
>>> @@ -684,6 +687,11 @@ def qmp_to_opts(self, obj):
>>> output_list += [key + '=' + obj[key]]
>>> return ','.join(output_list)
>>> + def get_qmp_events(self, wait: bool = False) -> List[QMPMessage]:
>>> + if qemu_gdb:
>>> + wait = 0.0
[...]
>>
>> Second, I don’t understand this. If the caller wants to block waiting
>> on an event, then that should have nothing to do with whether we have
>> gdb running or not. As far as I understand, setting wait to 0.0 is
>> the same as wait = False, i.e. we don’t block and just return None
>> immediately if there is no pending event.
>
> You're right, this might not be needed here. The problem I had was that
> calling gdb and pausing at a breakpoint or something for a while would
> make the QMP socket timeout, thus aborting the whole test. In order to
> avoid that, I need to stop or delay timers.
>
> I can't remember why I added this check here. At some point I am sure
> the test was failing because of socket timeout expiration, but I cannot
> reproduce the problem when commenting out this check above in
> get_qmp_events. The other check in patch 3 should be enough.
Hm, ok. I’d guessed that you intended the wait=0.0 or wait=False to
mean that we get an infinite timeout (i.e., no timeout), but that’s
exactly why I didn’t get it. wait=0.0 doesn’t give an infinite timeout,
but instead basically times out immediately.
Max
next prev parent reply other threads:[~2021-05-03 15:03 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 [this message]
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
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=d652828c-a5c9-8f61-84b8-0f1d8a679911@redhat.com \
--to=mreitz@redhat.com \
--cc=crosa@redhat.com \
--cc=eesposit@redhat.com \
--cc=ehabkost@redhat.com \
--cc=jsnow@redhat.com \
--cc=kwolf@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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).