Re: [PATCH v3 05/15] qemu-iotests: delay QMP socket timers

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: Fri, 30 Apr 2021 13:59:19 +0200	[thread overview]
Message-ID: <df1df43e-cfdc-ddeb-f7c1-f9399f252b35@redhat.com> (raw)
In-Reply-To: <20210414170352.29927-6-eesposit@redhat.com>

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

Also, mypy complains that this variable is a float, so iotest 297 (which 
runs mypy) fails.

>           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
> @@ -478,7 +478,10 @@ def log(msg: Msg,
>   
>   class Timeout:
>       def __init__(self, seconds, errmsg="Timeout"):
> -        self.seconds = seconds
> +        if qemu_gdb:
> +            self.seconds = 3000
> +        else:
> +            self.seconds = seconds

We might as well completely disable the timeout then, that would be more 
honest, I think.  (I.e. to make __enter__ and __exit__ no-ops.)

>           self.errmsg = errmsg
>       def __enter__(self):
>           signal.signal(signal.SIGALRM, self.timeout)
> @@ -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

First, this is a bool.  I can see that qmp.py expects a
Union[bool, float], but machine.py expects just a bool.  (Also, mypy 
complains that this specific `wait` here is a `bool`.  You can see that 
by running iotest 297.)

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.

Max

> +        return super().get_qmp_events(wait=wait)
> +
>       def get_qmp_events_filtered(self, wait=60.0):
>           result = []
>           for ev in self.get_qmp_events(wait=wait):
> 