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
