Re: [PATCH v3 10/15] qemu_iotests: extent QMP socket timeout when using valgrind

From: John Snow <jsnow@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, Max Reitz <mreitz@redhat.com>,
	Cleber Rosa <crosa@redhat.com>,
	Paolo Bonzini <pbonzini@redhat.com>
Subject: Re: [PATCH v3 10/15] qemu_iotests: extent QMP socket timeout when using valgrind
Date: Thu, 13 May 2021 14:47:16 -0400	[thread overview]
Message-ID: <a7353583-27d2-762f-34b3-a4bda861057b@redhat.com> (raw)
In-Reply-To: <20210414170352.29927-11-eesposit@redhat.com>

On 4/14/21 1:03 PM, Emanuele Giuseppe Esposito wrote:
> As with gdbserver, valgrind delays the test execution, so
> the default QMP socket timeout timeout too soon.
> 
> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> ---
>   python/qemu/machine.py        | 2 +-
>   tests/qemu-iotests/iotests.py | 4 ++--
>   2 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/python/qemu/machine.py b/python/qemu/machine.py
> index d6142271c2..dce96e1858 100644
> --- a/python/qemu/machine.py
> +++ b/python/qemu/machine.py
> @@ -410,7 +410,7 @@ def _launch(self) -> None:
>                                          shell=False,
>                                          close_fds=False)
>   
> -        if 'gdbserver' in self._wrapper:
> +        if 'gdbserver' in self._wrapper or 'valgrind' in self._wrapper:

This approaches me suggesting that we just change __init__ to accept a 
parameter that lets the caller decide what kind of timeout(s) they find 
acceptable. They know more about what they're trying to run than we do.

Certainly after launch occurs, the user is free to just grab the qmp 
object and tinker around with the timeouts, but that does not allow us 
to change the timeout(s) for accept itself.

D'oh.

(Spilled milk: It was probably a mistake to make the default launch 
behavior here have a timeout of 15 seconds. That logic likely belongs to 
the iotests implementation. The default here probably ought to indeed be 
"wait forever".)

In the here and now ... would it be acceptable to change the launch() 
method to add a timeout parameter? It's still a little awkward, because 
conceptually it's a timeout for just QMP and not for the actual duration 
of the entire launch process.

But, I guess, it's *closer* to the truth.

If you wanted to route it that way, I take back what I said about not 
wanting to pass around variables to event loop hooks.

If we defined the timeout as something that applies exclusively to the 
launching process, then it'd be appropriate to route that to the 
launch-related functions ... and subclasses would have to be adjusted to 
be made aware that they're expected to operate within those parameters, 
which is good.

Sorry for my waffling back and forth on this. Let me know what the 
actual requirements are if you figure out which timeouts you need / 
don't need and I'll give you some review priority.

If you attack this series again, can you please split out the python/* 
patches into its own little series and CC me?

--js

>               self._qmp_timer = None
>           self._post_launch()
>   
> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
> index a2e8604674..94597433fa 100644
> --- a/tests/qemu-iotests/iotests.py
> +++ b/tests/qemu-iotests/iotests.py
> @@ -489,7 +489,7 @@ def log(msg: Msg,
>   
>   class Timeout:
>       def __init__(self, seconds, errmsg="Timeout"):
> -        if qemu_gdb:
> +        if qemu_gdb or qemu_valgrind:
>               self.seconds = 3000
>           else:
>               self.seconds = seconds
> @@ -700,7 +700,7 @@ def qmp_to_opts(self, obj):
>           return ','.join(output_list)
>   
>       def get_qmp_events(self, wait: bool = False) -> List[QMPMessage]:
> -        if qemu_gdb:
> +        if qemu_gdb or qemu_valgrind:
>               wait = 0.0
>           return super().get_qmp_events(wait=wait)
>   
> 