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

From: John Snow <jsnow@redhat.com>
To: Hanna Reitz <hreitz@redhat.com>
Cc: Kevin Wolf <kwolf@redhat.com>,
	Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>,
	Eduardo Habkost <ehabkost@redhat.com>,
	qemu-block@nongnu.org, qemu-devel <qemu-devel@nongnu.org>,
	Cleber Rosa <crosa@redhat.com>
Subject: Re: [PATCH 09/15] python/machine: remove has_quit argument
Date: Fri, 17 Sep 2021 19:12:51 -0400	[thread overview]
Message-ID: <CAFn=p-Yo3=D5uCEE4_mN9BAuO_8ALe4rSOzZmU6Lt85okUk+8A@mail.gmail.com> (raw)
In-Reply-To: <f39831de-afc1-3152-777d-ec4af76c087e@redhat.com>

[-- Attachment #1: Type: text/plain, Size: 3368 bytes --]

On Fri, Sep 17, 2021 at 9:59 AM Hanna Reitz <hreitz@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 <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.
>
>
"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:
> >           """
>
>

[-- Attachment #2: Type: text/html, Size: 4553 bytes --]