On Fri, Sep 17, 2021 at 9:59 AM Hanna Reitz <hre...@redhat.com> wrote:
> On 17.09.21 07:40, John Snow wrote: > > If we spy on the QMP commands instead, we don't need callers to remember > > to pass it. Seems like a fair trade-off. > > > > The one slightly weird bit is overloading this instance variable for > > wait(), where we use it to mean "don't issue the qmp 'quit' > > command". This means that wait() will "fail" if the QEMU process does > > not terminate of its own accord. > > > > In most cases, we probably did already actually issue quit -- some > > iotests do this -- but in some others, we may be waiting for QEMU to > > terminate for some other reason. > > > > Signed-off-by: John Snow <js...@redhat.com> > > --- > > python/qemu/machine/machine.py | 35 +++++++++++++++++++--------------- > > tests/qemu-iotests/040 | 7 +------ > > tests/qemu-iotests/218 | 2 +- > > tests/qemu-iotests/255 | 2 +- > > 4 files changed, 23 insertions(+), 23 deletions(-) > > Looks good overall, some bikeshedding below. > > > diff --git a/python/qemu/machine/machine.py > b/python/qemu/machine/machine.py > > index 056d340e35..6e58d2f951 100644 > > --- a/python/qemu/machine/machine.py > > +++ b/python/qemu/machine/machine.py > > @@ -170,6 +170,7 @@ def __init__(self, > > self._console_socket: Optional[socket.socket] = None > > self._remove_files: List[str] = [] > > self._user_killed = False > > + self._has_quit = False > > Sounds a bit weird, because evidently it has quit. > > I mean, it was always more of a has_sent_quit or quit_issued, but now it > really seems a bit wrong. > > "quit_issued" seems like it might help overall, I'll do that. > [...] > > > @@ -529,7 +526,9 @@ def wait(self, timeout: Optional[int] = 30) -> None: > > :param timeout: Optional timeout in seconds. Default 30 > seconds. > > A value of `None` is an infinite wait. > > """ > > - self.shutdown(has_quit=True, timeout=timeout) > > + # In case QEMU was stopped without issuing 'quit': > > This comment confused me more than anything and only looking at the > function’s name and doc string was I able to understand why we have > has_quit = True here, and only by scratching my head asking myself why > you’d write the comment did I understand the comment’s purpose. > > What isn’t quite clear in the comment is that we kind of expect > _has_quit to already be True, because the VM was probably exited with > `quit`. But we don’t know for sure, so we have to force _has_quit to True. > > But it’s also not necessary to explain, I think. The idea is simply > that this function should do nothing to make the VM quit, so it’s > absolutely natural that we’d set _has_quit to True, because the caller > guarantees that they already made the VM quit. No need to explain why > this might already be True, and why it’s still OK. > > So I’d just say something like “Do not send a `quit` to the VM, just > wait for it to exit”. > > I'll just drop the comment, then. > Hanna > > > + self._has_quit = True > > + self.shutdown(timeout=timeout) > > > > def set_qmp_monitor(self, enabled: bool = True) -> None: > > """ > >
