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 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.