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

From: Emanuele Giuseppe Esposito <eesposit@redhat.com>
To: John Snow <jsnow@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: Tue, 18 May 2021 20:20:01 +0200	[thread overview]
Message-ID: <8311eff0-c5f7-8689-78d2-141966964896@redhat.com> (raw)
In-Reply-To: <7004fc54-d456-32f6-1c35-4cce1de00a6e@redhat.com>

On 18/05/2021 16:26, John Snow wrote:
> On 5/18/21 9:58 AM, Emanuele Giuseppe Esposito wrote:
>>
>>>> So the current plan I have for _qmp_timer is:
>>>>
>>>> - As Max suggested, move it in __init__ and check there for the 
>>>> wrapper contents. If we need to block forever (gdb, valgrind), we 
>>>> set it to None. Otherwise to 15 seconds. I think setting it always 
>>>> to None is not ideal, because if you are testing something that 
>>>> deadlocks (see my attempts to remove/add locks in QEMU multiqueue) 
>>>> and the socket is set to block forever, you don't know if the test 
>>>> is super slow or it just deadlocked.
>>>>
>>>
>>> I agree with your concern on rational defaults, let's focus on that 
>>> briefly:
>>>
>>> Let's have QEMUMachine default to *no timeouts* moving forward, and 
>>> have the timeouts be *opt-in*. This keeps the Machine class somewhat 
>>> pure and free of opinions. The separation of mechanism and policy.
>>>
>>> Next, instead of modifying hundreds of tests to opt-in to the 
>>> timeout, let's modify the VM class in iotests.py to opt-in to that 
>>> timeout, restoring the current "safe" behavior of iotests.
>>>
>>> The above items can happen in a single commit, preserving behavior in 
>>> the bisect.
>>>
>>> Finally, we can add a non-private property that individual tests can 
>>> re-override to opt BACK out of the default.
>>>
>>> Something as simple as:
>>>
>>> vm.qmp_timeout = None
>>>
>>> would be just fine.
>>>
>>
>> I applied these suggested changes, will send v4 and we'll see what 
>> comes out of it.
>>
>>>> Well, one can argue that in both cases this is not the expected 
>>>> behavior, but I think having an upper bound on each QMP command 
>>>> execution would be good.
>>>>
>>>> - pass _qmp_timer directly to self._qmp.accept() in _post launch, 
>>>> leaving _launch() intact. I think this makes sense because as you 
>>>> also mentioned, changing _post_launch() into taking a parameter 
>>>> requires changing also all subclasses and pass values around.
>>>>
>>>
>>> Sounds OK. If we do change the defaults back to "No Timeout" in a way 
>>> that allows an override by an opinionated class, we'll already have 
>>> the public property, though, so a parameter might not be needed.
>>>
>>> (Yes, this is the THIRD time I've changed my mind in 48 hours.)
>>>
>>>> Any opinion on this is very welcome.
>>>>
>>>
>>> Brave words!
>>>
>>> My last thought here is that I still don't like the idea of 
>>> QEMUMachine class changing its timeout behavior based on the 
>>> introspection of wrapper args.
>>>
>>> It feels much more like the case that a caller who is knowingly 
>>> wrapping it with a program that delays its execution should change 
>>> its parameters accordingly based on what the caller knows about what 
>>> they're trying to accomplish.
>>>
>>> Does that make the code too messy? I understand you probably want to 
>>> ensure that adding a GDB wrapper is painless and simple, so it might 
>>> not be great to always ask a caller to remember to set some timeout 
>>> value to use it.
>>>
>> I am not sure I follow you here, where do you want to move this logic? 
>> Can you please elaborate more, I did not understand what you mean.
>>
>> I understand that tweaking the timers in iotests.py with checks like
>>
>> if not (qemu_gdb or qemu_valgrind):
>>      normal timer
>>
>> may not be the most beautiful piece of code, but as you said it keeps 
>> things as simple as they can.
>>
> 
> What I mean is that of the two patterns:
> 
> caller.py:
>      vm = machine(..., wrapper_args=['gdb', ...])
> 
> machine.py:
>      def __init__(...):
>          if 'gdb' in wrapper_args:
>              self.timer = None
> 
> vs this one:
> 
> caller.py:
>      vm = machine(..., wrapper_args=['gdb', ...], timer=None)
> 
> machine.py:
>      def __init__(...):
>          ... # No introspection of wrapper_args
> 
> 
> I prefer the second. I would assume it's possible to localize the logic 
> that creates a GDB-wrapped machine alongside the knowledge that it needs 
> the timeout turned off *outside* of the machine class.
> 
> I could be *very wrong* about that assumption though. The reason I 
> prefer the second pattern is because it avoids having to deal with what 
> happens when a caller specifies both a timeout and a gdb-wrapper. In the 
> second case, the caller explicitly requested the timeout to be None, so 
> anything that occurs afterwards is the fault of the caller, not machine.py.
> 
> To me, that's "simpler". (I could be wrong, I don't have a great overall 
> view of your series, just the bits that I have seen that touch machine.py.)

I think this can be done almost effortless. With your suggested changes 
on qmp_timer, we can have:

machine.py
def __init__(self, ..., wrapper, timer: None)
	self._qmp_timer = timer

def _post_launch(self)
	self._qmp.accept(self._qmp_timer)

iotests.py
	timer = None if qemu_gdb or qemu_valgrind else 15.0
	wrapper = qemu_gdb or qemu_valgrind # did not know about this OR trick btw
	vm = machine(..., wrapper, timer)

Thank you,
Emanuele
> 
> --js
> 
>>> I figure that the right place to do this, though, is wherever the 
>>> boilerplate code gets written that knows how to set up the right gdb 
>>> args and so on, and not in machine.py. It sounds like iotests.py code 
>>> to me, maybe in the VM class.
>>
>> After the changes suggested on qmp_timeout, iotests.py already 
>> contains the only code to perform the setup right for gdb and 
>> valgrind, and machine.py is not touched (except for qmp_timeout). 
>> iotests.py will essentially contain a couple of ifs like the one 
>> above, changing the timer when gdb and valgring are *not* needed.
>>
>> Emanuele
>>
> 