From 14661d93d787846833b0e62d5995195c8851f741 Mon Sep 17 00:00:00 2001 From: John Snow Date: Fri, 10 Jul 2020 01:06:38 -0400 Subject: python/machine.py: consolidate _post_shutdown() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Move more cleanup actions into _post_shutdown. As a change, if QEMU should so happen to be terminated during a call to wait(), that event will now be logged. This is not likely to occur during normative use. Signed-off-by: John Snow Reviewed-by: Cleber Rosa Tested-by: Cleber Rosa Reviewed-by: Philippe Mathieu-Daudé Message-Id: <20200710050649.32434-2-jsnow@redhat.com> Signed-off-by: Philippe Mathieu-Daudé --- python/qemu/machine.py | 27 +++++++++++++-------------- 1 file changed, 13 insertions(+), 14 deletions(-) (limited to 'python/qemu') diff --git a/python/qemu/machine.py b/python/qemu/machine.py index c25f0b4..ca1f211 100644 --- a/python/qemu/machine.py +++ b/python/qemu/machine.py @@ -294,6 +294,8 @@ class QEMUMachine: self._qmp.accept() def _post_shutdown(self): + self._load_io_log() + if self._qemu_log_file is not None: self._qemu_log_file.close() self._qemu_log_file = None @@ -307,6 +309,17 @@ class QEMUMachine: while len(self._remove_files) > 0: self._remove_if_exists(self._remove_files.pop()) + exitcode = self.exitcode() + if exitcode is not None and exitcode < 0: + msg = 'qemu received signal %i; command: "%s"' + if self._qemu_full_args: + command = ' '.join(self._qemu_full_args) + else: + command = '' + LOG.warning(msg, -int(exitcode), command) + + self._launched = False + def launch(self): """ Launch the VM and make sure we cleanup and expose the @@ -355,7 +368,6 @@ class QEMUMachine: self._popen.wait() if self._qmp: self._qmp.close() - self._load_io_log() self._post_shutdown() def shutdown(self, has_quit=False, hard=False): @@ -382,21 +394,8 @@ class QEMUMachine: self._popen.kill() self._popen.wait() - self._load_io_log() self._post_shutdown() - exitcode = self.exitcode() - if exitcode is not None and exitcode < 0 and \ - not (exitcode == -9 and hard): - msg = 'qemu received signal %i: %s' - if self._qemu_full_args: - command = ' '.join(self._qemu_full_args) - else: - command = '' - LOG.warning(msg, -int(exitcode), command) - - self._launched = False - def kill(self): self.shutdown(hard=True) -- cgit v1.1 From 671940e633b83ac489e0b4bb407749723ff8a879 Mon Sep 17 00:00:00 2001 From: John Snow Date: Fri, 10 Jul 2020 01:06:39 -0400 Subject: python/machine.py: Close QMP socket in cleanup MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit It's not important to do this before waiting for the process to exit, so it can be done during generic post-shutdown cleanup. Signed-off-by: John Snow Reviewed-by: Philippe Mathieu-Daudé Reviewed-by: Cleber Rosa Tested-by: Cleber Rosa Message-Id: <20200710050649.32434-3-jsnow@redhat.com> Signed-off-by: Philippe Mathieu-Daudé --- python/qemu/machine.py | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) (limited to 'python/qemu') diff --git a/python/qemu/machine.py b/python/qemu/machine.py index ca1f211..d3faa9a 100644 --- a/python/qemu/machine.py +++ b/python/qemu/machine.py @@ -294,6 +294,10 @@ class QEMUMachine: self._qmp.accept() def _post_shutdown(self): + if self._qmp: + self._qmp.close() + self._qmp = None + self._load_io_log() if self._qemu_log_file is not None: @@ -366,8 +370,6 @@ class QEMUMachine: Wait for the VM to power off """ self._popen.wait() - if self._qmp: - self._qmp.close() self._post_shutdown() def shutdown(self, has_quit=False, hard=False): @@ -388,7 +390,6 @@ class QEMUMachine: try: if not has_quit: self._qmp.cmd('quit') - self._qmp.close() self._popen.wait(timeout=3) except: self._popen.kill() -- cgit v1.1 From e2c97f161294c702ee4a2dd08532d5df67f6bff4 Mon Sep 17 00:00:00 2001 From: John Snow Date: Fri, 10 Jul 2020 01:06:40 -0400 Subject: python/machine.py: Add _early_cleanup hook MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Some parts of cleanup need to occur prior to shutdown, otherwise shutdown might break. Move this into a suitably named method/callback. Signed-off-by: John Snow Reviewed-by: Philippe Mathieu-Daudé Reviewed-by: Cleber Rosa Tested-by: Cleber Rosa Message-Id: <20200710050649.32434-4-jsnow@redhat.com> Signed-off-by: Philippe Mathieu-Daudé --- python/qemu/machine.py | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) (limited to 'python/qemu') diff --git a/python/qemu/machine.py b/python/qemu/machine.py index d3faa9a..127926b 100644 --- a/python/qemu/machine.py +++ b/python/qemu/machine.py @@ -365,6 +365,17 @@ class QEMUMachine: close_fds=False) self._post_launch() + def _early_cleanup(self) -> None: + """ + Perform any cleanup that needs to happen before the VM exits. + """ + # If we keep the console socket open, we may deadlock waiting + # for QEMU to exit, while QEMU is waiting for the socket to + # become writeable. + if self._console_socket is not None: + self._console_socket.close() + self._console_socket = None + def wait(self): """ Wait for the VM to power off @@ -376,12 +387,7 @@ class QEMUMachine: """ Terminate the VM and clean up """ - # If we keep the console socket open, we may deadlock waiting - # for QEMU to exit, while QEMU is waiting for the socket to - # become writeable. - if self._console_socket is not None: - self._console_socket.close() - self._console_socket = None + self._early_cleanup() if self.is_running(): if hard: -- cgit v1.1 From 3a7d64b6fc8ddce3987005e0ee6eadbe2cbba5c8 Mon Sep 17 00:00:00 2001 From: John Snow Date: Fri, 10 Jul 2020 01:06:41 -0400 Subject: python/machine.py: Perform early cleanup for wait() calls, too MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This is primarily for consistency, and is a step towards wait() and shutdown() sharing the same implementation so that the two cleanup paths cannot diverge. Signed-off-by: John Snow Reviewed-by: Philippe Mathieu-Daudé Reviewed-by: Cleber Rosa Tested-by: Cleber Rosa Message-Id: <20200710050649.32434-5-jsnow@redhat.com> Signed-off-by: Philippe Mathieu-Daudé --- python/qemu/machine.py | 1 + 1 file changed, 1 insertion(+) (limited to 'python/qemu') diff --git a/python/qemu/machine.py b/python/qemu/machine.py index 127926b..63e4087 100644 --- a/python/qemu/machine.py +++ b/python/qemu/machine.py @@ -380,6 +380,7 @@ class QEMUMachine: """ Wait for the VM to power off """ + self._early_cleanup() self._popen.wait() self._post_shutdown() -- cgit v1.1 From a3842cb078a195db0715b00edd7812adcb8b077f Mon Sep 17 00:00:00 2001 From: John Snow Date: Fri, 10 Jul 2020 01:06:42 -0400 Subject: python/machine.py: Prohibit multiple shutdown() calls MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit If the VM is not launched, don't try to shut it down. As a change, _post_shutdown now unconditionally also calls _early_cleanup in order to offer comprehensive object cleanup in failure cases. As a courtesy, treat it as a NOP instead of rejecting it as an error. This is slightly nicer for acceptance tests where vm.shutdown() is issued unconditionally in tearDown callbacks. Signed-off-by: John Snow Reviewed-by: Philippe Mathieu-Daudé Message-Id: <20200710050649.32434-6-jsnow@redhat.com> Signed-off-by: Philippe Mathieu-Daudé --- python/qemu/machine.py | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) (limited to 'python/qemu') diff --git a/python/qemu/machine.py b/python/qemu/machine.py index 63e4087..c28957e 100644 --- a/python/qemu/machine.py +++ b/python/qemu/machine.py @@ -294,6 +294,13 @@ class QEMUMachine: self._qmp.accept() def _post_shutdown(self): + """ + Called to cleanup the VM instance after the process has exited. + May also be called after a failed launch. + """ + # Comprehensive reset for the failed launch case: + self._early_cleanup() + if self._qmp: self._qmp.close() self._qmp = None @@ -339,7 +346,7 @@ class QEMUMachine: self._launch() self._launched = True except: - self.shutdown() + self._post_shutdown() LOG.debug('Error launching VM') if self._qemu_full_args: @@ -368,6 +375,8 @@ class QEMUMachine: def _early_cleanup(self) -> None: """ Perform any cleanup that needs to happen before the VM exits. + + Called additionally by _post_shutdown for comprehensive cleanup. """ # If we keep the console socket open, we may deadlock waiting # for QEMU to exit, while QEMU is waiting for the socket to @@ -388,6 +397,9 @@ class QEMUMachine: """ Terminate the VM and clean up """ + if not self._launched: + return + self._early_cleanup() if self.is_running(): -- cgit v1.1 From c9b3045bc2f52aca8825b6a04e9367b87d64d1cf Mon Sep 17 00:00:00 2001 From: John Snow Date: Fri, 10 Jul 2020 01:06:43 -0400 Subject: python/machine.py: Add a configurable timeout to shutdown() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three seconds is hardcoded. Use it as a default parameter instead, and use that value for both waits that may occur in the function. Signed-off-by: John Snow Reviewed-by: Philippe Mathieu-Daudé Reviewed-by: Cleber Rosa Message-Id: <20200710050649.32434-7-jsnow@redhat.com> Signed-off-by: Philippe Mathieu-Daudé --- python/qemu/machine.py | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) (limited to 'python/qemu') diff --git a/python/qemu/machine.py b/python/qemu/machine.py index c28957e..e825f0b 100644 --- a/python/qemu/machine.py +++ b/python/qemu/machine.py @@ -393,7 +393,9 @@ class QEMUMachine: self._popen.wait() self._post_shutdown() - def shutdown(self, has_quit=False, hard=False): + def shutdown(self, has_quit: bool = False, + hard: bool = False, + timeout: Optional[int] = 3) -> None: """ Terminate the VM and clean up """ @@ -409,10 +411,10 @@ class QEMUMachine: try: if not has_quit: self._qmp.cmd('quit') - self._popen.wait(timeout=3) + self._popen.wait(timeout=timeout) except: self._popen.kill() - self._popen.wait() + self._popen.wait(timeout=timeout) self._post_shutdown() -- cgit v1.1 From 895280593139a1c34e59526835ba8fda903f8aaa Mon Sep 17 00:00:00 2001 From: John Snow Date: Fri, 10 Jul 2020 01:06:44 -0400 Subject: python/machine.py: Make wait() call shutdown() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit At this point, shutdown(has_quit=True) and wait() do essentially the same thing; they perform cleanup without actually instructing QEMU to quit. Define one in terms of the other. Signed-off-by: John Snow Reviewed-by: Philippe Mathieu-Daudé Reviewed-by: Cleber Rosa Tested-by: Cleber Rosa Message-Id: <20200710050649.32434-8-jsnow@redhat.com> Signed-off-by: Philippe Mathieu-Daudé --- python/qemu/machine.py | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) (limited to 'python/qemu') diff --git a/python/qemu/machine.py b/python/qemu/machine.py index e825f0b..3f0b873 100644 --- a/python/qemu/machine.py +++ b/python/qemu/machine.py @@ -385,14 +385,6 @@ class QEMUMachine: self._console_socket.close() self._console_socket = None - def wait(self): - """ - Wait for the VM to power off - """ - self._early_cleanup() - self._popen.wait() - self._post_shutdown() - def shutdown(self, has_quit: bool = False, hard: bool = False, timeout: Optional[int] = 3) -> None: @@ -421,6 +413,15 @@ class QEMUMachine: def kill(self): self.shutdown(hard=True) + def wait(self, timeout: Optional[int] = None) -> None: + """ + Wait for the VM to power off and perform post-shutdown cleanup. + + :param timeout: Optional timeout in seconds. + Default None, an infinite wait. + """ + self.shutdown(has_quit=True, timeout=timeout) + def set_qmp_monitor(self, enabled=True): """ Set the QMP monitor. -- cgit v1.1 From 193bf1c061ce0bb078ebc153facb9f31fe139d72 Mon Sep 17 00:00:00 2001 From: John Snow Date: Fri, 10 Jul 2020 01:06:47 -0400 Subject: python/machine.py: split shutdown into hard and soft flavors MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This is done primarily to avoid the 'bare except' pattern, which suppresses all exceptions during shutdown and can obscure errors. Replace this with a pattern that isolates the different kind of shutdown paradigms (_hard_shutdown and _soft_shutdown), and a new fallback shutdown handler (_do_shutdown) that gracefully attempts one before the other. This split now also ensures that no matter what happens, _post_shutdown() is always invoked. shutdown() changes in behavior such that if it attempts to do a graceful shutdown and is unable to, it will now always raise an exception to indicate this. This can be avoided by the test writer in three ways: 1. If the VM is expected to have already exited or is in the process of exiting, wait() can be used instead of shutdown() to clean up resources instead. This helps avoid race conditions in shutdown. 2. If a test writer is expecting graceful shutdown to fail, shutdown should be called in a try...except block. 3. If the test writer has no interest in performing a graceful shutdown at all, kill() can be used instead. Handling shutdown in this way makes it much more explicit which type of shutdown we want and allows the library to report problems with this process. Signed-off-by: John Snow Reviewed-by: Philippe Mathieu-Daudé Reviewed-by: Cleber Rosa Tested-by: Cleber Rosa Message-Id: <20200710050649.32434-11-jsnow@redhat.com> Signed-off-by: Philippe Mathieu-Daudé --- python/qemu/machine.py | 98 ++++++++++++++++++++++++++++++++++++++++++-------- 1 file changed, 83 insertions(+), 15 deletions(-) (limited to 'python/qemu') diff --git a/python/qemu/machine.py b/python/qemu/machine.py index 3f0b873..a955e3f 100644 --- a/python/qemu/machine.py +++ b/python/qemu/machine.py @@ -49,6 +49,12 @@ class QEMUMachineAddDeviceError(QEMUMachineError): """ +class AbnormalShutdown(QEMUMachineError): + """ + Exception raised when a graceful shutdown was requested, but not performed. + """ + + class MonitorResponseError(qmp.QMPError): """ Represents erroneous QMP monitor reply @@ -376,6 +382,7 @@ class QEMUMachine: """ Perform any cleanup that needs to happen before the VM exits. + May be invoked by both soft and hard shutdown in failover scenarios. Called additionally by _post_shutdown for comprehensive cleanup. """ # If we keep the console socket open, we may deadlock waiting @@ -385,32 +392,93 @@ class QEMUMachine: self._console_socket.close() self._console_socket = None + def _hard_shutdown(self) -> None: + """ + Perform early cleanup, kill the VM, and wait for it to terminate. + + :raise subprocess.Timeout: When timeout is exceeds 60 seconds + waiting for the QEMU process to terminate. + """ + self._early_cleanup() + self._popen.kill() + self._popen.wait(timeout=60) + + def _soft_shutdown(self, has_quit: bool = False, + timeout: Optional[int] = 3) -> None: + """ + Perform early cleanup, attempt to gracefully shut down the VM, and wait + for it to terminate. + + :param has_quit: When True, don't attempt to issue 'quit' QMP command + :param timeout: Optional timeout in seconds for graceful shutdown. + Default 3 seconds, A value of None is an infinite wait. + + :raise ConnectionReset: On QMP communication errors + :raise subprocess.TimeoutExpired: When timeout is exceeded waiting for + the QEMU process to terminate. + """ + self._early_cleanup() + + if self._qmp is not None: + if not has_quit: + # Might raise ConnectionReset + self._qmp.cmd('quit') + + # May raise subprocess.TimeoutExpired + self._popen.wait(timeout=timeout) + + def _do_shutdown(self, has_quit: bool = False, + timeout: Optional[int] = 3) -> None: + """ + Attempt to shutdown the VM gracefully; fallback to a hard shutdown. + + :param has_quit: When True, don't attempt to issue 'quit' QMP command + :param timeout: Optional timeout in seconds for graceful shutdown. + Default 3 seconds, A value of None is an infinite wait. + + :raise AbnormalShutdown: When the VM could not be shut down gracefully. + The inner exception will likely be ConnectionReset or + subprocess.TimeoutExpired. In rare cases, non-graceful termination + may result in its own exceptions, likely subprocess.TimeoutExpired. + """ + try: + self._soft_shutdown(has_quit, timeout) + except Exception as exc: + self._hard_shutdown() + raise AbnormalShutdown("Could not perform graceful shutdown") \ + from exc + def shutdown(self, has_quit: bool = False, hard: bool = False, timeout: Optional[int] = 3) -> None: """ - Terminate the VM and clean up + Terminate the VM (gracefully if possible) and perform cleanup. + Cleanup will always be performed. + + 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. + Default 3 seconds, A value of None is an infinite wait. """ if not self._launched: return - self._early_cleanup() - - if self.is_running(): + try: if hard: - self._popen.kill() - elif self._qmp: - try: - if not has_quit: - self._qmp.cmd('quit') - self._popen.wait(timeout=timeout) - except: - self._popen.kill() - self._popen.wait(timeout=timeout) - - self._post_shutdown() + self._hard_shutdown() + else: + self._do_shutdown(has_quit, timeout=timeout) + finally: + self._post_shutdown() def kill(self): + """ + Terminate the VM forcefully, wait for it to exit, and perform cleanup. + """ self.shutdown(hard=True) def wait(self, timeout: Optional[int] = None) -> None: -- cgit v1.1 From de6e08b5b987afbaf22e37e7f9e34421fb76ef3f Mon Sep 17 00:00:00 2001 From: John Snow Date: Fri, 10 Jul 2020 01:06:48 -0400 Subject: python/machine.py: re-add sigkill warning suppression MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit If the user kills QEMU on purpose, we don't need to warn them about that having happened: they know already. Signed-off-by: John Snow Reviewed-by: Philippe Mathieu-Daudé Reviewed-by: Cleber Rosa Message-Id: <20200710050649.32434-12-jsnow@redhat.com> Signed-off-by: Philippe Mathieu-Daudé --- python/qemu/machine.py | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) (limited to 'python/qemu') diff --git a/python/qemu/machine.py b/python/qemu/machine.py index a955e3f..736a3c9 100644 --- a/python/qemu/machine.py +++ b/python/qemu/machine.py @@ -22,6 +22,7 @@ import logging import os import subprocess import shutil +import signal import socket import tempfile from typing import Optional, Type @@ -133,6 +134,7 @@ class QEMUMachine: self._console_address = None self._console_socket = None self._remove_files = [] + self._user_killed = False self._console_log_path = console_log if self._console_log_path: # In order to log the console, buffering needs to be enabled. @@ -327,7 +329,8 @@ class QEMUMachine: self._remove_if_exists(self._remove_files.pop()) exitcode = self.exitcode() - if exitcode is not None and exitcode < 0: + if (exitcode is not None and exitcode < 0 + and not (self._user_killed and exitcode == -signal.SIGKILL)): msg = 'qemu received signal %i; command: "%s"' if self._qemu_full_args: command = ' '.join(self._qemu_full_args) @@ -335,6 +338,7 @@ class QEMUMachine: command = '' LOG.warning(msg, -int(exitcode), command) + self._user_killed = False self._launched = False def launch(self): @@ -469,6 +473,7 @@ class QEMUMachine: try: if hard: + self._user_killed = True self._hard_shutdown() else: self._do_shutdown(has_quit, timeout=timeout) -- cgit v1.1 From 04f0e36eba7b1a06e413a0690d4b1a24994d99fe Mon Sep 17 00:00:00 2001 From: John Snow Date: Fri, 10 Jul 2020 01:06:49 -0400 Subject: python/machine.py: change default wait timeout to 3 seconds MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Machine.wait() does not appear to be used except in the acceptance tests, and an infinite timeout by default in a test suite is not the most helpful. Change it to 3 seconds, like the default shutdown timeout. Signed-off-by: John Snow Reviewed-by: Philippe Mathieu-Daudé Reviewed-by: Cleber Rosa Tested-by: Cleber Rosa Message-Id: <20200710050649.32434-13-jsnow@redhat.com> Signed-off-by: Philippe Mathieu-Daudé --- python/qemu/machine.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'python/qemu') diff --git a/python/qemu/machine.py b/python/qemu/machine.py index 736a3c9..6905518 100644 --- a/python/qemu/machine.py +++ b/python/qemu/machine.py @@ -486,12 +486,12 @@ class QEMUMachine: """ self.shutdown(hard=True) - def wait(self, timeout: Optional[int] = None) -> None: + def wait(self, timeout: Optional[int] = 3) -> None: """ Wait for the VM to power off and perform post-shutdown cleanup. :param timeout: Optional timeout in seconds. - Default None, an infinite wait. + Default 3 seconds, A value of None is an infinite wait. """ self.shutdown(has_quit=True, timeout=timeout) -- cgit v1.1 From a5d76376d65d8777f28bb064412a8d72fa2c7953 Mon Sep 17 00:00:00 2001 From: John Snow Date: Fri, 10 Jul 2020 01:22:05 -0400 Subject: python/qmp.py: Define common types MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Define some common types that we'll need to annotate a lot of other functions going forward. Signed-off-by: John Snow Reviewed-by: Kevin Wolf Message-Id: <20200710052220.3306-2-jsnow@redhat.com> Signed-off-by: Philippe Mathieu-Daudé --- python/qemu/qmp.py | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) (limited to 'python/qemu') diff --git a/python/qemu/qmp.py b/python/qemu/qmp.py index e64b6b5..8388c7b 100644 --- a/python/qemu/qmp.py +++ b/python/qemu/qmp.py @@ -12,13 +12,31 @@ import errno import socket import logging from typing import ( + Any, + Dict, Optional, TextIO, Type, + Tuple, + Union, ) from types import TracebackType +# QMPMessage is a QMP Message of any kind. +# e.g. {'yee': 'haw'} +# +# QMPReturnValue is the inner value of return values only. +# {'return': {}} is the QMPMessage, +# {} is the QMPReturnValue. +QMPMessage = Dict[str, Any] +QMPReturnValue = Dict[str, Any] + +InternetAddrT = Tuple[str, str] +UnixAddrT = str +SocketAddrT = Union[InternetAddrT, UnixAddrT] + + class QMPError(Exception): """ QMP base exception -- cgit v1.1 From e3a23b4803a3939c7e24e8946880f5ef369ef781 Mon Sep 17 00:00:00 2001 From: John Snow Date: Fri, 10 Jul 2020 01:22:07 -0400 Subject: python/qmp.py: re-absorb MonitorResponseError MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When I initially split this out, I considered this more of a machine error than a QMP protocol error, but I think that's misguided. Move this back to qmp.py and name it QMPResponseError. Convert qmp.command() to use this exception type. Signed-off-by: John Snow Reviewed-by: Philippe Mathieu-Daudé Reviewed-by: Kevin Wolf Message-Id: <20200710052220.3306-4-jsnow@redhat.com> Signed-off-by: Philippe Mathieu-Daudé --- python/qemu/machine.py | 15 +-------------- python/qemu/qmp.py | 17 +++++++++++++++-- 2 files changed, 16 insertions(+), 16 deletions(-) (limited to 'python/qemu') diff --git a/python/qemu/machine.py b/python/qemu/machine.py index 6905518..80c4d4a 100644 --- a/python/qemu/machine.py +++ b/python/qemu/machine.py @@ -56,19 +56,6 @@ class AbnormalShutdown(QEMUMachineError): """ -class MonitorResponseError(qmp.QMPError): - """ - Represents erroneous QMP monitor reply - """ - def __init__(self, reply): - try: - desc = reply["error"]["desc"] - except KeyError: - desc = reply - super().__init__(desc) - self.reply = reply - - class QEMUMachine: """ A QEMU VM @@ -533,7 +520,7 @@ class QEMUMachine: if reply is None: raise qmp.QMPError("Monitor is closed") if "error" in reply: - raise MonitorResponseError(reply) + raise qmp.QMPResponseError(reply) return reply["return"] def get_qmp_event(self, wait=False): diff --git a/python/qemu/qmp.py b/python/qemu/qmp.py index 8388c7b..aa8a666 100644 --- a/python/qemu/qmp.py +++ b/python/qemu/qmp.py @@ -61,6 +61,19 @@ class QMPTimeoutError(QMPError): """ +class QMPResponseError(QMPError): + """ + Represents erroneous QMP monitor reply + """ + def __init__(self, reply: QMPMessage): + try: + desc = reply['error']['desc'] + except KeyError: + desc = reply + super().__init__(desc) + self.reply = reply + + class QEMUMonitorProtocol: """ Provide an API to connect to QEMU via QEMU Monitor Protocol (QMP) and then @@ -251,8 +264,8 @@ class QEMUMonitorProtocol: Build and send a QMP command to the monitor, report errors if any """ ret = self.cmd(cmd, kwds) - if "error" in ret: - raise Exception(ret['error']['desc']) + if 'error' in ret: + raise QMPResponseError(ret) return ret['return'] def pull_event(self, wait=False): -- cgit v1.1 From ef5d474472426eda6abf8128cdb1d026af94862b Mon Sep 17 00:00:00 2001 From: John Snow Date: Fri, 10 Jul 2020 01:22:08 -0400 Subject: python/qmp.py: Do not return None from cmd_obj MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This makes typing the qmp library difficult, as it necessitates wrapping Optional[] around the type for every return type up the stack. At some point, it becomes difficult to discern or remember why it's None instead of the expected object. Use the python exception system to tell us exactly why we didn't get an object. Remove this special-cased return. Signed-off-by: John Snow Reviewed-by: Kevin Wolf Message-Id: <20200710052220.3306-5-jsnow@redhat.com> Signed-off-by: Philippe Mathieu-Daudé --- python/qemu/qmp.py | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) (limited to 'python/qemu') diff --git a/python/qemu/qmp.py b/python/qemu/qmp.py index aa8a666..ef3c919 100644 --- a/python/qemu/qmp.py +++ b/python/qemu/qmp.py @@ -225,22 +225,18 @@ class QEMUMonitorProtocol: self.__sockfile = self.__sock.makefile(mode='r') return self.__negotiate_capabilities() - def cmd_obj(self, qmp_cmd): + def cmd_obj(self, qmp_cmd: QMPMessage) -> QMPMessage: """ Send a QMP command to the QMP Monitor. @param qmp_cmd: QMP command to be sent as a Python dict - @return QMP response as a Python dict or None if the connection has - been closed + @return QMP response as a Python dict """ self.logger.debug(">>> %s", qmp_cmd) - try: - self.__sock.sendall(json.dumps(qmp_cmd).encode('utf-8')) - except OSError as err: - if err.errno == errno.EPIPE: - return None - raise err + self.__sock.sendall(json.dumps(qmp_cmd).encode('utf-8')) resp = self.__json_read() + if resp is None: + raise QMPConnectError("Unexpected empty reply from server") self.logger.debug("<<< %s", resp) return resp -- cgit v1.1 From 2e2d93051753067fc5b888fdc18831127a4a900e Mon Sep 17 00:00:00 2001 From: John Snow Date: Fri, 10 Jul 2020 01:22:09 -0400 Subject: python/qmp.py: add casts to JSON deserialization MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit mypy and python type hints are not powerful enough to properly describe JSON messages in Python 3.6. The best we can do, generally, is describe them as Dict[str, Any]. Add casts to coerce this type for static analysis; but do NOT enforce this type at runtime in any way. Note: Python 3.8 adds a TypedDict construct which allows for the description of more arbitrary Dictionary shapes. There is a third-party module, "Pydantic", which is compatible with 3.6 that can be used instead of the JSON library that parses JSON messages to fully-typed Python objects, and may be preferable in some cases. (That is well beyond the scope of this commit or series.) Signed-off-by: John Snow Reviewed-by: Kevin Wolf Message-Id: <20200710052220.3306-6-jsnow@redhat.com> Signed-off-by: Philippe Mathieu-Daudé --- python/qemu/qmp.py | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) (limited to 'python/qemu') diff --git a/python/qemu/qmp.py b/python/qemu/qmp.py index ef3c919..1ae3605 100644 --- a/python/qemu/qmp.py +++ b/python/qemu/qmp.py @@ -13,6 +13,7 @@ import socket import logging from typing import ( Any, + cast, Dict, Optional, TextIO, @@ -130,7 +131,10 @@ class QEMUMonitorProtocol: data = self.__sockfile.readline() if not data: return None - resp = json.loads(data) + # By definition, any JSON received from QMP is a QMPMessage, + # and we are asserting only at static analysis time that it + # has a particular shape. + resp: QMPMessage = json.loads(data) if 'event' in resp: self.logger.debug("<<< %s", resp) self.__events.append(resp) @@ -262,7 +266,7 @@ class QEMUMonitorProtocol: ret = self.cmd(cmd, kwds) if 'error' in ret: raise QMPResponseError(ret) - return ret['return'] + return cast(QMPReturnValue, ret['return']) def pull_event(self, wait=False): """ -- cgit v1.1 From 84dcdf0887cdaaba7300442482c99e5064865a2d Mon Sep 17 00:00:00 2001 From: John Snow Date: Fri, 10 Jul 2020 01:22:10 -0400 Subject: python/qmp.py: add QMPProtocolError MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In the case that we receive a reply but are unable to understand it, use this exception name to indicate that case. Signed-off-by: John Snow Reviewed-by: Philippe Mathieu-Daudé Reviewed-by: Kevin Wolf Message-Id: <20200710052220.3306-7-jsnow@redhat.com> Signed-off-by: Philippe Mathieu-Daudé --- python/qemu/qmp.py | 10 ++++++++++ 1 file changed, 10 insertions(+) (limited to 'python/qemu') diff --git a/python/qemu/qmp.py b/python/qemu/qmp.py index 1ae3605..7935dab 100644 --- a/python/qemu/qmp.py +++ b/python/qemu/qmp.py @@ -62,6 +62,12 @@ class QMPTimeoutError(QMPError): """ +class QMPProtocolError(QMPError): + """ + QMP protocol error; unexpected response + """ + + class QMPResponseError(QMPError): """ Represents erroneous QMP monitor reply @@ -266,6 +272,10 @@ class QEMUMonitorProtocol: ret = self.cmd(cmd, kwds) if 'error' in ret: raise QMPResponseError(ret) + if 'return' not in ret: + raise QMPProtocolError( + "'return' key not found in QMP response '{}'".format(str(ret)) + ) return cast(QMPReturnValue, ret['return']) def pull_event(self, wait=False): -- cgit v1.1