diff mbox series

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

Message ID 20210917054047.2042843-10-jsnow@redhat.com (mailing list archive)
State New, archived
Headers show
Series Switch iotests to using Async QMP | expand

Commit Message

John Snow Sept. 17, 2021, 5:40 a.m. UTC
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(-)

Comments

Hanna Reitz Sept. 17, 2021, 1:59 p.m. UTC | #1
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:
>           """
John Snow Sept. 17, 2021, 11:12 p.m. UTC | #2
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:
> >           """
>
>
diff mbox series

Patch

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
 
     def __enter__(self: _T) -> _T:
         return self
@@ -368,6 +369,7 @@  def _post_shutdown(self) -> None:
                 command = ''
             LOG.warning(msg, -int(exitcode), command)
 
+        self._has_quit = False
         self._user_killed = False
         self._launched = False
 
@@ -443,15 +445,13 @@  def _hard_shutdown(self) -> None:
         self._subp.kill()
         self._subp.wait(timeout=60)
 
-    def _soft_shutdown(self, timeout: Optional[int],
-                       has_quit: bool = False) -> None:
+    def _soft_shutdown(self, timeout: Optional[int]) -> None:
         """
         Perform early cleanup, attempt to gracefully shut down the VM, and wait
         for it to terminate.
 
         :param timeout: Timeout in seconds for graceful shutdown.
                         A value of None is an infinite wait.
-        :param has_quit: When True, don't attempt to issue 'quit' QMP command
 
         :raise ConnectionReset: On QMP communication errors
         :raise subprocess.TimeoutExpired: When timeout is exceeded waiting for
@@ -460,21 +460,19 @@  def _soft_shutdown(self, timeout: Optional[int],
         self._early_cleanup()
 
         if self._qmp_connection:
-            if not has_quit:
+            if not self._has_quit:
                 # Might raise ConnectionReset
-                self._qmp.cmd('quit')
+                self.qmp('quit')
 
         # May raise subprocess.TimeoutExpired
         self._subp.wait(timeout=timeout)
 
-    def _do_shutdown(self, timeout: Optional[int],
-                     has_quit: bool = False) -> None:
+    def _do_shutdown(self, timeout: Optional[int]) -> None:
         """
         Attempt to shutdown the VM gracefully; fallback to a hard shutdown.
 
         :param timeout: Timeout in seconds for graceful shutdown.
                         A value of None is an infinite wait.
-        :param has_quit: When True, don't attempt to issue 'quit' QMP command
 
         :raise AbnormalShutdown: When the VM could not be shut down gracefully.
             The inner exception will likely be ConnectionReset or
@@ -482,13 +480,13 @@  def _do_shutdown(self, timeout: Optional[int],
             may result in its own exceptions, likely subprocess.TimeoutExpired.
         """
         try:
-            self._soft_shutdown(timeout, has_quit)
+            self._soft_shutdown(timeout)
         except Exception as exc:
             self._hard_shutdown()
             raise AbnormalShutdown("Could not perform graceful shutdown") \
                 from exc
 
-    def shutdown(self, has_quit: bool = False,
+    def shutdown(self,
                  hard: bool = False,
                  timeout: Optional[int] = 30) -> None:
         """
@@ -498,7 +496,6 @@  def shutdown(self, has_quit: bool = False,
         If the VM has not yet been launched, or shutdown(), wait(), or kill()
         have already been called, this method does nothing.
 
-        :param has_quit: When true, do not attempt to issue 'quit' QMP command.
         :param hard: When true, do not attempt graceful shutdown, and
                      suppress the SIGKILL warning log message.
         :param timeout: Optional timeout in seconds for graceful shutdown.
@@ -512,7 +509,7 @@  def shutdown(self, has_quit: bool = False,
                 self._user_killed = True
                 self._hard_shutdown()
             else:
-                self._do_shutdown(timeout, has_quit)
+                self._do_shutdown(timeout)
         finally:
             self._post_shutdown()
 
@@ -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':
+        self._has_quit = True
+        self.shutdown(timeout=timeout)
 
     def set_qmp_monitor(self, enabled: bool = True) -> None:
         """
@@ -574,7 +573,10 @@  def qmp(self, cmd: str,
             conv_keys = True
 
         qmp_args = self._qmp_args(conv_keys, args)
-        return self._qmp.cmd(cmd, args=qmp_args)
+        ret = self._qmp.cmd(cmd, args=qmp_args)
+        if cmd == 'quit' and 'error' not in ret and 'return' in ret:
+            self._has_quit = True
+        return ret
 
     def command(self, cmd: str,
                 conv_keys: bool = True,
@@ -585,7 +587,10 @@  def command(self, cmd: str,
         On failure raise an exception.
         """
         qmp_args = self._qmp_args(conv_keys, args)
-        return self._qmp.command(cmd, **qmp_args)
+        ret = self._qmp.command(cmd, **qmp_args)
+        if cmd == 'quit':
+            self._has_quit = True
+        return ret
 
     def get_qmp_event(self, wait: bool = False) -> Optional[QMPMessage]:
         """
diff --git a/tests/qemu-iotests/040 b/tests/qemu-iotests/040
index f3677de9df..6af5ab9e76 100755
--- a/tests/qemu-iotests/040
+++ b/tests/qemu-iotests/040
@@ -92,10 +92,9 @@  class TestSingleDrive(ImageCommitTestCase):
         self.vm.add_device('virtio-scsi')
         self.vm.add_device("scsi-hd,id=scsi0,drive=drive0")
         self.vm.launch()
-        self.has_quit = False
 
     def tearDown(self):
-        self.vm.shutdown(has_quit=self.has_quit)
+        self.vm.shutdown()
         os.remove(test_img)
         os.remove(mid_img)
         os.remove(backing_img)
@@ -127,8 +126,6 @@  class TestSingleDrive(ImageCommitTestCase):
         result = self.vm.qmp('quit')
         self.assert_qmp(result, 'return', {})
 
-        self.has_quit = True
-
     # Same as above, but this time we add the filter after starting the job
     @iotests.skip_if_unsupported(['throttle'])
     def test_commit_plus_filter_and_quit(self):
@@ -147,8 +144,6 @@  class TestSingleDrive(ImageCommitTestCase):
         result = self.vm.qmp('quit')
         self.assert_qmp(result, 'return', {})
 
-        self.has_quit = True
-
     def test_device_not_found(self):
         result = self.vm.qmp('block-commit', device='nonexistent', top='%s' % mid_img)
         self.assert_qmp(result, 'error/class', 'DeviceNotFound')
diff --git a/tests/qemu-iotests/218 b/tests/qemu-iotests/218
index 325d8244fb..4922b4d3b6 100755
--- a/tests/qemu-iotests/218
+++ b/tests/qemu-iotests/218
@@ -187,4 +187,4 @@  with iotests.VM() as vm, \
     log(vm.qmp('quit'))
 
     with iotests.Timeout(5, 'Timeout waiting for VM to quit'):
-        vm.shutdown(has_quit=True)
+        vm.shutdown()
diff --git a/tests/qemu-iotests/255 b/tests/qemu-iotests/255
index c43aa9c67a..3d6d0e80cb 100755
--- a/tests/qemu-iotests/255
+++ b/tests/qemu-iotests/255
@@ -123,4 +123,4 @@  with iotests.FilePath('src.qcow2') as src_path, \
     vm.qmp_log('block-job-cancel', device='job0')
     vm.qmp_log('quit')
 
-    vm.shutdown(has_quit=True)
+    vm.shutdown()