Re: [PATCH 09/15] python/machine: remove has_quit argument

[Hanna Reitz]

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 <jsnow@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.

[...]

> @@ -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”.

Hanna

> +        self._has_quit = True
> +        self.shutdown(timeout=timeout)
>   
>       def set_qmp_monitor(self, enabled: bool = True) -> None:
>           """