[PATCH v3 0/4] Python: Improvements for iotest 040,041

[PATCH v3 0/4] Python: Improvements for iotest 040,041

[John Snow]

GitLab: https://gitlab.com/jsnow/qemu/-/commits/python-aqmp-fixes
CI: https://gitlab.com/jsnow/qemu/-/pipelines/455146881

Fixes and improvements all relating to "iotest 040,041, intermittent
failure in netbsd VM"
https://lists.gnu.org/archive/html/qemu-devel/2022-01/msg01975.html

See each patch for details.

V3:
 - Retitled series
 - Dropped patch that was already merged
 - Reworded some comments, docstrings, etc.

John Snow (4):
  python/aqmp: Fix negotiation with pre-"oob" QEMU
  python/machine: raise VMLaunchFailure exception from launch()
  python: upgrade mypy to 0.780
  python/aqmp: add socket bind step to legacy.py

 python/Pipfile.lock                       | 66 +++++++++++++----------
 python/qemu/aqmp/legacy.py                |  3 ++
 python/qemu/aqmp/protocol.py              | 41 ++++++++++++--
 python/qemu/aqmp/qmp_client.py            |  4 +-
 python/qemu/machine/machine.py            | 45 +++++++++++++---
 python/setup.cfg                          |  2 +-
 tests/qemu-iotests/tests/mirror-top-perms |  3 +-
 7 files changed, 123 insertions(+), 41 deletions(-)

-- 
2.31.1

[PATCH v3 1/4] python/aqmp: Fix negotiation with pre-"oob" QEMU

[John Snow]

QEMU versions prior to the "oob" capability *also* can't accept the
"enable" keyword argument at all. Fix the handshake process with older
QEMU versions.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 python/qemu/aqmp/qmp_client.py | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/python/qemu/aqmp/qmp_client.py b/python/qemu/aqmp/qmp_client.py
index f1a845cc82..90a8737f03 100644
--- a/python/qemu/aqmp/qmp_client.py
+++ b/python/qemu/aqmp/qmp_client.py
@@ -292,9 +292,9 @@ async def _negotiate(self) -> None:
         """
         self.logger.debug("Negotiating capabilities ...")
 
-        arguments: Dict[str, List[str]] = {'enable': []}
+        arguments: Dict[str, List[str]] = {}
         if self._greeting and 'oob' in self._greeting.QMP.capabilities:
-            arguments['enable'].append('oob')
+            arguments.setdefault('enable', []).append('oob')
         msg = self.make_execute_msg('qmp_capabilities', arguments=arguments)
 
         # It's not safe to use execute() here, because the reader/writers
-- 
2.31.1

[PATCH v3 2/4] python/machine: raise VMLaunchFailure exception from launch()

[John Snow]

This allows us to pack in some extra information about the failure,
which guarantees that if the caller did not *intentionally* cause a
failure (by capturing this Exception), some pretty good clues will be
printed at the bottom of the traceback information.

This will help make failures in the event of a non-negative return code
more obvious when they go unhandled; the current behavior is to print a
warning message only in the event of signal-based terminations (for
negative return codes).

(Note: In Python, catching BaseException instead of Exception is like
installing a signal handler that will run as long as Python itself
doesn't crash. KeyboardInterrupt and several other "strong" events in
Python are a BaseException. These events should generally not be
suppressed, but occasionally we want to perform some cleanup in response
to one.)

Signed-off-by: John Snow <jsnow@redhat.com>
---
 python/qemu/machine/machine.py            | 45 ++++++++++++++++++++---
 tests/qemu-iotests/tests/mirror-top-perms |  3 +-
 2 files changed, 40 insertions(+), 8 deletions(-)

diff --git a/python/qemu/machine/machine.py b/python/qemu/machine/machine.py
index 67ab06ca2b..a5972fab4d 100644
--- a/python/qemu/machine/machine.py
+++ b/python/qemu/machine/machine.py
@@ -74,6 +74,35 @@ class QEMUMachineAddDeviceError(QEMUMachineError):
     """
 
 
+class VMLaunchFailure(QEMUMachineError):
+    """
+    Exception raised when a VM launch was attempted, but failed.
+    """
+    def __init__(self, exitcode: Optional[int],
+                 command: str, output: Optional[str]):
+        super().__init__(exitcode, command, output)
+        self.exitcode = exitcode
+        self.command = command
+        self.output = output
+
+    def __str__(self) -> str:
+        ret = ''
+        if self.__cause__ is not None:
+            name = type(self.__cause__).__name__
+            reason = str(self.__cause__)
+            if reason:
+                ret += f"{name}: {reason}"
+            else:
+                ret += f"{name}"
+        ret += '\n'
+
+        if self.exitcode is not None:
+            ret += f"\tExit code: {self.exitcode}\n"
+        ret += f"\tCommand: {self.command}\n"
+        ret += f"\tOutput: {self.output}\n"
+        return ret
+
+
 class AbnormalShutdown(QEMUMachineError):
     """
     Exception raised when a graceful shutdown was requested, but not performed.
@@ -397,7 +426,7 @@ def launch(self) -> None:
 
         try:
             self._launch()
-        except:
+        except BaseException as exc:
             # We may have launched the process but it may
             # have exited before we could connect via QMP.
             # Assume the VM didn't launch or is exiting.
@@ -408,11 +437,15 @@ def launch(self) -> None:
             else:
                 self._post_shutdown()
 
-            LOG.debug('Error launching VM')
-            if self._qemu_full_args:
-                LOG.debug('Command: %r', ' '.join(self._qemu_full_args))
-            if self._iolog:
-                LOG.debug('Output: %r', self._iolog)
+            if isinstance(exc, Exception):
+                raise VMLaunchFailure(
+                    exitcode=self.exitcode(),
+                    command=' '.join(self._qemu_full_args),
+                    output=self._iolog
+                ) from exc
+
+            # Don't wrap 'BaseException'; doing so would downgrade
+            # that exception. However, we still want to clean up.
             raise
 
     def _launch(self) -> None:
diff --git a/tests/qemu-iotests/tests/mirror-top-perms b/tests/qemu-iotests/tests/mirror-top-perms
index 0a51a613f3..b5849978c4 100755
--- a/tests/qemu-iotests/tests/mirror-top-perms
+++ b/tests/qemu-iotests/tests/mirror-top-perms
@@ -21,7 +21,6 @@
 
 import os
 
-from qemu.aqmp import ConnectError
 from qemu.machine import machine
 from qemu.qmp import QMPConnectError
 
@@ -107,7 +106,7 @@ class TestMirrorTopPerms(iotests.QMPTestCase):
                 self.vm_b.launch()
                 print('ERROR: VM B launched successfully, '
                       'this should not have happened')
-        except (QMPConnectError, ConnectError):
+        except (QMPConnectError, machine.VMLaunchFailure):
             assert 'Is another process using the image' in self.vm_b.get_log()
 
         result = self.vm.qmp('block-job-cancel',
-- 
2.31.1

[PATCH v3 3/4] python: upgrade mypy to 0.780

[John Snow]

We need a slightly newer version of mypy in order to use some features
of the asyncio server functions in the next commit.

(Note: pipenv is not really suited to upgrading individual packages; I
need to replace this tool with something better for the task. For now,
the miscellaneous updates not related to the mypy upgrade are simply
beyond my control. It's on my list to take care of soon.)

Signed-off-by: John Snow <jsnow@redhat.com>
---
 python/Pipfile.lock | 66 ++++++++++++++++++++++++++-------------------
 python/setup.cfg    |  2 +-
 2 files changed, 40 insertions(+), 28 deletions(-)

diff --git a/python/Pipfile.lock b/python/Pipfile.lock
index d2a7dbd88b..ce46404ce0 100644
--- a/python/Pipfile.lock
+++ b/python/Pipfile.lock
@@ -1,7 +1,7 @@
 {
     "_meta": {
         "hash": {
-            "sha256": "784b327272db32403d5a488507853b5afba850ba26a5948e5b6a90c1baef2d9c"
+            "sha256": "f1a25654d884a5b450e38d78b1f2e3ebb9073e421cc4358d4bbb83ac251a5670"
         },
         "pipfile-spec": 6,
         "requires": {
@@ -34,7 +34,7 @@
                 "sha256:09bdb456e02564731f8b5957cdd0c98a7f01d2db5e90eb1d794c353c28bfd705",
                 "sha256:6a8a51f64dae307f6e0c9db752b66a7951e282389d8362cc1d39a56f3feeb31d"
             ],
-            "markers": "python_version ~= '3.6'",
+            "index": "pypi",
             "version": "==2.6.0"
         },
         "avocado-framework": {
@@ -50,6 +50,7 @@
                 "sha256:106fef6dc37dd8c0e2c0a60d3fca3e77460a48907f335fa28420463a6f799736",
                 "sha256:23e223426b28491b1ced97dc3bbe183027419dfc7982b4fa2f05d5f3ff10711c"
             ],
+            "index": "pypi",
             "version": "==0.3.2"
         },
         "filelock": {
@@ -57,6 +58,7 @@
                 "sha256:18d82244ee114f543149c66a6e0c14e9c4f8a1044b5cdaadd0f82159d6a6ff59",
                 "sha256:929b7d63ec5b7d6b71b0fa5ac14e030b3f70b75747cef1b10da9b879fef15836"
             ],
+            "index": "pypi",
             "version": "==3.0.12"
         },
         "flake8": {
@@ -88,7 +90,7 @@
                 "sha256:54161657e8ffc76596c4ede7080ca68cb02962a2e074a2586b695a93a925d36e",
                 "sha256:e962bff7440364183203d179d7ae9ad90cb1f2b74dcb84300e88ecc42dca3351"
             ],
-            "markers": "python_version < '3.7'",
+            "index": "pypi",
             "version": "==5.1.4"
         },
         "isort": {
@@ -124,7 +126,7 @@
                 "sha256:ed361bb83436f117f9917d282a456f9e5009ea12fd6de8742d1a4752c3017e93",
                 "sha256:f5144c75445ae3ca2057faac03fda5a902eff196702b0a24daf1d6ce0650514b"
             ],
-            "markers": "python_version >= '2.7' and python_version not in '3.0, 3.1, 3.2, 3.3, 3.4, 3.5'",
+            "index": "pypi",
             "version": "==1.6.0"
         },
         "mccabe": {
@@ -136,23 +138,23 @@
         },
         "mypy": {
             "hashes": [
-                "sha256:15b948e1302682e3682f11f50208b726a246ab4e6c1b39f9264a8796bb416aa2",
-                "sha256:219a3116ecd015f8dca7b5d2c366c973509dfb9a8fc97ef044a36e3da66144a1",
-                "sha256:3b1fc683fb204c6b4403a1ef23f0b1fac8e4477091585e0c8c54cbdf7d7bb164",
-                "sha256:3beff56b453b6ef94ecb2996bea101a08f1f8a9771d3cbf4988a61e4d9973761",
-                "sha256:7687f6455ec3ed7649d1ae574136835a4272b65b3ddcf01ab8704ac65616c5ce",
-                "sha256:7ec45a70d40ede1ec7ad7f95b3c94c9cf4c186a32f6bacb1795b60abd2f9ef27",
-                "sha256:86c857510a9b7c3104cf4cde1568f4921762c8f9842e987bc03ed4f160925754",
-                "sha256:8a627507ef9b307b46a1fea9513d5c98680ba09591253082b4c48697ba05a4ae",
-                "sha256:8dfb69fbf9f3aeed18afffb15e319ca7f8da9642336348ddd6cab2713ddcf8f9",
-                "sha256:a34b577cdf6313bf24755f7a0e3f3c326d5c1f4fe7422d1d06498eb25ad0c600",
-                "sha256:a8ffcd53cb5dfc131850851cc09f1c44689c2812d0beb954d8138d4f5fc17f65",
-                "sha256:b90928f2d9eb2f33162405f32dde9f6dcead63a0971ca8a1b50eb4ca3e35ceb8",
-                "sha256:c56ffe22faa2e51054c5f7a3bc70a370939c2ed4de308c690e7949230c995913",
-                "sha256:f91c7ae919bbc3f96cd5e5b2e786b2b108343d1d7972ea130f7de27fdd547cf3"
+                "sha256:00cb1964a7476e871d6108341ac9c1a857d6bd20bf5877f4773ac5e9d92cd3cd",
+                "sha256:127de5a9b817a03a98c5ae8a0c46a20dc44442af6dcfa2ae7f96cb519b312efa",
+                "sha256:1f3976a945ad7f0a0727aafdc5651c2d3278e3c88dee94e2bf75cd3386b7b2f4",
+                "sha256:2f8c098f12b402c19b735aec724cc9105cc1a9eea405d08814eb4b14a6fb1a41",
+                "sha256:4ef13b619a289aa025f2273e05e755f8049bb4eaba6d703a425de37d495d178d",
+                "sha256:5d142f219bf8c7894dfa79ebfb7d352c4c63a325e75f10dfb4c3db9417dcd135",
+                "sha256:62eb5dd4ea86bda8ce386f26684f7f26e4bfe6283c9f2b6ca6d17faf704dcfad",
+                "sha256:64c36eb0936d0bfb7d8da49f92c18e312ad2e3ed46e5548ae4ca997b0d33bd59",
+                "sha256:75eed74d2faf2759f79c5f56f17388defd2fc994222312ec54ee921e37b31ad4",
+                "sha256:974bebe3699b9b46278a7f076635d219183da26e1a675c1f8243a69221758273",
+                "sha256:a5e5bb12b7982b179af513dddb06fca12285f0316d74f3964078acbfcf4c68f2",
+                "sha256:d31291df31bafb997952dc0a17ebb2737f802c754aed31dd155a8bfe75112c57",
+                "sha256:d3b4941de44341227ece1caaf5b08b23e42ad4eeb8b603219afb11e9d4cfb437",
+                "sha256:eadb865126da4e3c4c95bdb47fe1bb087a3e3ea14d39a3b13224b8a4d9f9a102"
             ],
             "index": "pypi",
-            "version": "==0.770"
+            "version": "==0.780"
         },
         "mypy-extensions": {
             "hashes": [
@@ -166,7 +168,7 @@
                 "sha256:5b327ac1320dc863dca72f4514ecc086f31186744b84a230374cc1fd776feae5",
                 "sha256:67714da7f7bc052e064859c05c595155bd1ee9f69f76557e21f051443c20947a"
             ],
-            "markers": "python_version >= '2.7' and python_version not in '3.0, 3.1, 3.2, 3.3'",
+            "index": "pypi",
             "version": "==20.9"
         },
         "pluggy": {
@@ -174,7 +176,7 @@
                 "sha256:15b2acde666561e1298d71b523007ed7364de07029219b604cf808bfa1c765b0",
                 "sha256:966c145cd83c96502c3c3868f50408687b38434af77734af1e9ca461a4081d2d"
             ],
-            "markers": "python_version >= '2.7' and python_version not in '3.0, 3.1, 3.2, 3.3'",
+            "index": "pypi",
             "version": "==0.13.1"
         },
         "py": {
@@ -182,7 +184,7 @@
                 "sha256:21b81bda15b66ef5e1a777a21c4dcd9c20ad3efd0b3f817e7a809035269e1bd3",
                 "sha256:3b80836aa6d1feeaa108e046da6423ab8f6ceda6468545ae8d02d9d58d18818a"
             ],
-            "markers": "python_version >= '2.7' and python_version not in '3.0, 3.1, 3.2, 3.3'",
+            "index": "pypi",
             "version": "==1.10.0"
         },
         "pycodestyle": {
@@ -205,7 +207,7 @@
                 "sha256:a18f47b506a429f6f4b9df81bb02beab9ca21d0a5fee38ed15aef65f0545519f",
                 "sha256:d66e804411278594d764fc69ec36ec13d9ae9147193a1740cd34d272ca383b8e"
             ],
-            "markers": "python_version >= '3.5'",
+            "index": "pypi",
             "version": "==2.9.0"
         },
         "pylint": {
@@ -221,13 +223,21 @@
                 "sha256:c203ec8783bf771a155b207279b9bccb8dea02d8f0c9e5f8ead507bc3246ecc1",
                 "sha256:ef9d7589ef3c200abe66653d3f1ab1033c3c419ae9b9bdb1240a85b024efc88b"
             ],
-            "markers": "python_version >= '2.6' and python_version not in '3.0, 3.1, 3.2, 3.3'",
+            "index": "pypi",
             "version": "==2.4.7"
         },
         "qemu": {
             "editable": true,
             "path": "."
         },
+        "setuptools": {
+            "hashes": [
+                "sha256:22c7348c6d2976a52632c67f7ab0cdf40147db7789f9aed18734643fe9cf3373",
+                "sha256:4ce92f1e1f8f01233ee9952c04f6b81d1e02939d6e1b488428154974a4d0783e"
+            ],
+            "markers": "python_version >= '3.6'",
+            "version": "==59.6.0"
+        },
         "six": {
             "hashes": [
                 "sha256:1e61c37477a1626458e36f7b1d82aa5c9b094fa4802892072e49de9c60c4c926",
@@ -294,19 +304,21 @@
                 "sha256:50b6f157849174217d0656f99dc82fe932884fb250826c18350e159ec6cdf342",
                 "sha256:779383f6086d90c99ae41cf0ff39aac8a7937a9283ce0a414e5dd782f4c94a84"
             ],
-            "markers": "python_version < '3.8'",
+            "index": "pypi",
             "version": "==3.10.0.0"
         },
         "urwid": {
             "hashes": [
                 "sha256:588bee9c1cb208d0906a9f73c613d2bd32c3ed3702012f51efe318a3f2127eae"
             ],
+            "index": "pypi",
             "version": "==2.1.2"
         },
         "urwid-readline": {
             "hashes": [
                 "sha256:018020cbc864bb5ed87be17dc26b069eae2755cb29f3a9c569aac3bded1efaf4"
             ],
+            "index": "pypi",
             "version": "==0.13"
         },
         "virtualenv": {
@@ -314,7 +326,7 @@
                 "sha256:14fdf849f80dbb29a4eb6caa9875d476ee2a5cf76a5f5415fa2f1606010ab467",
                 "sha256:2b0126166ea7c9c3661f5b8e06773d28f83322de7a3ff7d06f0aed18c9de6a76"
             ],
-            "markers": "python_version >= '2.7' and python_version not in '3.0, 3.1, 3.2, 3.3'",
+            "index": "pypi",
             "version": "==20.4.7"
         },
         "wrapt": {
@@ -328,7 +340,7 @@
                 "sha256:3607921face881ba3e026887d8150cca609d517579abe052ac81fc5aeffdbd76",
                 "sha256:51cb66cc54621609dd593d1787f286ee42a5c0adbb4b29abea5a63edc3e03098"
             ],
-            "markers": "python_version < '3.10'",
+            "index": "pypi",
             "version": "==3.4.1"
         }
     }
diff --git a/python/setup.cfg b/python/setup.cfg
index 3fb18f845d..18aea2bab3 100644
--- a/python/setup.cfg
+++ b/python/setup.cfg
@@ -41,7 +41,7 @@ devel =
     flake8 >= 3.6.0
     fusepy >= 2.0.4
     isort >= 5.1.2
-    mypy >= 0.770
+    mypy >= 0.780
     pylint >= 2.8.0
     tox >= 3.18.0
     urwid >= 2.1.2
-- 
2.31.1

[PATCH v3 4/4] python/aqmp: add socket bind step to legacy.py

[John Snow]

The synchronous QMP library would bind to the server address during
__init__(). The new library delays this to the accept() call, because
binding occurs inside of the call to start_[unix_]server(), which is an
async method -- so it cannot happen during __init__ anymore.

Python 3.7+ adds the ability to create the server (and thus the bind()
call) and begin the active listening in separate steps, but we don't
have that functionality in 3.6, our current minimum.

Therefore ... Add a temporary workaround that allows the synchronous
version of the client to bind the socket in advance, guaranteeing that
there will be a UNIX socket in the filesystem ready for the QEMU client
to connect to without a race condition.

(Yes, it's a bit ugly. Fixing it more nicely will have to wait until our
minimum Python version is 3.7+.)

Signed-off-by: John Snow <jsnow@redhat.com>
---
 python/qemu/aqmp/legacy.py   |  3 +++
 python/qemu/aqmp/protocol.py | 41 +++++++++++++++++++++++++++++++++---
 2 files changed, 41 insertions(+), 3 deletions(-)

diff --git a/python/qemu/aqmp/legacy.py b/python/qemu/aqmp/legacy.py
index 0890f95b16..6baa5f3409 100644
--- a/python/qemu/aqmp/legacy.py
+++ b/python/qemu/aqmp/legacy.py
@@ -56,6 +56,9 @@ def __init__(self, address: SocketAddrT,
         self._address = address
         self._timeout: Optional[float] = None
 
+        if server:
+            self._aqmp._bind_hack(address)  # pylint: disable=protected-access
+
     _T = TypeVar('_T')
 
     def _sync(
diff --git a/python/qemu/aqmp/protocol.py b/python/qemu/aqmp/protocol.py
index 50e973c2f2..33358f5cd7 100644
--- a/python/qemu/aqmp/protocol.py
+++ b/python/qemu/aqmp/protocol.py
@@ -15,6 +15,7 @@
 from enum import Enum
 from functools import wraps
 import logging
+import socket
 from ssl import SSLContext
 from typing import (
     Any,
@@ -238,6 +239,9 @@ def __init__(self, name: Optional[str] = None) -> None:
         self._runstate = Runstate.IDLE
         self._runstate_changed: Optional[asyncio.Event] = None
 
+        # Workaround for bind()
+        self._sock: Optional[socket.socket] = None
+
     def __repr__(self) -> str:
         cls_name = type(self).__name__
         tokens = []
@@ -427,6 +431,34 @@ async def _establish_connection(
         else:
             await self._do_connect(address, ssl)
 
+    def _bind_hack(self, address: Union[str, Tuple[str, int]]) -> None:
+        """
+        Used to create a socket in advance of accept().
+
+        This is a workaround to ensure that we can guarantee timing of
+        precisely when a socket exists to avoid a connection attempt
+        bouncing off of nothing.
+
+        Python 3.7+ adds a feature to separate the server creation and
+        listening phases instead, and should be used instead of this
+        hack.
+        """
+        if isinstance(address, tuple):
+            family = socket.AF_INET
+        else:
+            family = socket.AF_UNIX
+
+        sock = socket.socket(family, socket.SOCK_STREAM)
+        sock.setsockopt(socket.SOL_SOCKET, socket.SO_REUSEADDR, 1)
+
+        try:
+            sock.bind(address)
+        except:
+            sock.close()
+            raise
+
+        self._sock = sock
+
     @upper_half
     async def _do_accept(self, address: SocketAddrT,
                          ssl: Optional[SSLContext] = None) -> None:
@@ -464,24 +496,27 @@ async def _client_connected_cb(reader: asyncio.StreamReader,
         if isinstance(address, tuple):
             coro = asyncio.start_server(
                 _client_connected_cb,
-                host=address[0],
-                port=address[1],
+                host=None if self._sock else address[0],
+                port=None if self._sock else address[1],
                 ssl=ssl,
                 backlog=1,
                 limit=self._limit,
+                sock=self._sock,
             )
         else:
             coro = asyncio.start_unix_server(
                 _client_connected_cb,
-                path=address,
+                path=None if self._sock else address,
                 ssl=ssl,
                 backlog=1,
                 limit=self._limit,
+                sock=self._sock,
             )
 
         server = await coro     # Starts listening
         await connected.wait()  # Waits for the callback to fire (and finish)
         assert server is None
+        self._sock = None
 
         self.logger.debug("Connection accepted.")
 
-- 
2.31.1

Re: [PATCH v3 2/4] python/machine: raise VMLaunchFailure exception from launch()

[Hanna Reitz]

On 24.01.22 19:06, John Snow wrote:
> This allows us to pack in some extra information about the failure,
> which guarantees that if the caller did not *intentionally* cause a
> failure (by capturing this Exception), some pretty good clues will be
> printed at the bottom of the traceback information.

OK, I presume in contrast to unconditionally logging this on debug 
level, which is less than ideal because on that level it’s most likely 
hidden, but that was exactly the point, because we don’t know whether 
the caller will catch the exception, so we mustn’t log it on a more 
urgent level.

But by creating a new exception class, we get a reasonable log output 
exactly when the caller won’t catch it.

> This will help make failures in the event of a non-negative return code
> more obvious when they go unhandled; the current behavior is to print a
> warning message only in the event of signal-based terminations (for
> negative return codes).

I assume you mean the one in _post_shutdown()...?

Confused me a bit, because for a while I interpreted this to mean “We 
don’t output anything in case of a positive return code”, but it means 
“We don’t print any details in that case, because the exception we 
re-raise in launch() doesn’t contain valuable information”.  Makes sense.

> (Note: In Python, catching BaseException instead of Exception is like
> installing a signal handler that will run as long as Python itself
> doesn't crash.

This really confused me, because I can’t really understand this at all.

But I guess what I took from googling was that every exception object 
must be derived from BaseException eventually, and so we continue to 
catch all exceptions here, we just give them a name. (And then we create 
a VMLaunchFailure only for Exception exceptions, because the others 
don’t have much to do with launching the VM.)

> KeyboardInterrupt and several other "strong" events in
> Python are a BaseException. These events should generally not be
> suppressed, but occasionally we want to perform some cleanup in response
> to one.)
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>   python/qemu/machine/machine.py            | 45 ++++++++++++++++++++---
>   tests/qemu-iotests/tests/mirror-top-perms |  3 +-
>   2 files changed, 40 insertions(+), 8 deletions(-)

Reviewed-by: Hanna Reitz <hreitz@redhat.com>

(Looked at `except` and `ConnectError` usage outside of 
mirror-top-perms, but couldn’t find anything else that looked like it 
caught VM launch exceptions.)

Re: [PATCH v3 1/4] python/aqmp: Fix negotiation with pre-"oob" QEMU

[Hanna Reitz]

On 24.01.22 19:06, John Snow wrote:
> QEMU versions prior to the "oob" capability *also* can't accept the
> "enable" keyword argument at all. Fix the handshake process with older
> QEMU versions.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>   python/qemu/aqmp/qmp_client.py | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)

Reviewed-by: Hanna Reitz <hreitz@redhat.com>

Re: [PATCH v3 4/4] python/aqmp: add socket bind step to legacy.py

[Hanna Reitz]

On 24.01.22 19:06, John Snow wrote:
> The synchronous QMP library would bind to the server address during
> __init__(). The new library delays this to the accept() call, because
> binding occurs inside of the call to start_[unix_]server(), which is an
> async method -- so it cannot happen during __init__ anymore.
>
> Python 3.7+ adds the ability to create the server (and thus the bind()
> call) and begin the active listening in separate steps, but we don't
> have that functionality in 3.6, our current minimum.
>
> Therefore ... Add a temporary workaround that allows the synchronous
> version of the client to bind the socket in advance, guaranteeing that
> there will be a UNIX socket in the filesystem ready for the QEMU client
> to connect to without a race condition.
>
> (Yes, it's a bit ugly. Fixing it more nicely will have to wait until our
> minimum Python version is 3.7+.)

I mean.  Looks good to me?  Not quite enough for an R-b, I’d say, and 
you don’t really need an A-b from me on this, but looks good to me! O:)

> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>   python/qemu/aqmp/legacy.py   |  3 +++
>   python/qemu/aqmp/protocol.py | 41 +++++++++++++++++++++++++++++++++---
>   2 files changed, 41 insertions(+), 3 deletions(-)

Re: [PATCH v3 2/4] python/machine: raise VMLaunchFailure exception from launch()

[John Snow]

On Thu, Jan 27, 2022 at 9:22 AM Hanna Reitz <hreitz@redhat.com> wrote:
>
> On 24.01.22 19:06, John Snow wrote:
> > This allows us to pack in some extra information about the failure,
> > which guarantees that if the caller did not *intentionally* cause a
> > failure (by capturing this Exception), some pretty good clues will be
> > printed at the bottom of the traceback information.
>
> OK, I presume in contrast to unconditionally logging this on debug
> level, which is less than ideal because on that level it’s most likely
> hidden, but that was exactly the point, because we don’t know whether
> the caller will catch the exception, so we mustn’t log it on a more
> urgent level.
>

Exactly. More urgent logging interferes with tests where we
intentionally give a bad configuration. device-crash-test is another
example.

> But by creating a new exception class, we get a reasonable log output
> exactly when the caller won’t catch it.
>

That's the intent. By stuffing this info into the Exception, we'll
always see it printed if the error went unhandled. It seemed like the
best way to make sure the error messages were more apparent more often
without requiring the use of debug mode -- so that errors in e.g.
GitLab CI would print good diagnostic info by default.

> > This will help make failures in the event of a non-negative return code
> > more obvious when they go unhandled; the current behavior is to print a
> > warning message only in the event of signal-based terminations (for
> > negative return codes).
>
> I assume you mean the one in _post_shutdown()...?
>

Yes.

> Confused me a bit, because for a while I interpreted this to mean “We
> don’t output anything in case of a positive return code”, but it means
> “We don’t print any details in that case, because the exception we
> re-raise in launch() doesn’t contain valuable information”.  Makes sense.
>

Sorry, I'll improve the commit message.

> > (Note: In Python, catching BaseException instead of Exception is like
> > installing a signal handler that will run as long as Python itself
> > doesn't crash.
>
> This really confused me, because I can’t really understand this at all.
>
> But I guess what I took from googling was that every exception object
> must be derived from BaseException eventually, and so we continue to
> catch all exceptions here, we just give them a name. (And then we create
> a VMLaunchFailure only for Exception exceptions, because the others
> don’t have much to do with launching the VM.)
>

Apologies for not being more clear. ("It made sense to me at the time
...") What I mean to say here is: there are several ways to catch all
exceptions.

"except:" will catch everything.
"except BaseException" catches everything, too. This is equivalent to the above.
"except Exception" won't catch anything that inherits directly from
BaseException, only Exception and children.

What I wanted to convey here is that:

(1) If the exception is a BaseException, it's probably something like
KeyboardInterrupt (SIGINT) or SystemExit, we don't want to wrap the
exception and instead we want to re-raise it as-is. We are functioning
more or less like a signal handler here, performing some cleanup and
then yielding back control.
(2) If the exception is merely an Exception, it's OK to wrap it with
the custom exception and re-raise.

Wrapping a BaseException would be a problem because it would
'downgrade' the severity of the exception (so to speak) and may cause
issues.
I'll try to improve the commit message.

> > KeyboardInterrupt and several other "strong" events in
> > Python are a BaseException. These events should generally not be
> > suppressed, but occasionally we want to perform some cleanup in response
> > to one.)
> >
> > Signed-off-by: John Snow <jsnow@redhat.com>
> > ---
> >   python/qemu/machine/machine.py            | 45 ++++++++++++++++++++---
> >   tests/qemu-iotests/tests/mirror-top-perms |  3 +-
> >   2 files changed, 40 insertions(+), 8 deletions(-)
>
> Reviewed-by: Hanna Reitz <hreitz@redhat.com>
>
> (Looked at `except` and `ConnectError` usage outside of
> mirror-top-perms, but couldn’t find anything else that looked like it
> caught VM launch exceptions.)
>

Thanks!

Re: [PATCH v3 4/4] python/aqmp: add socket bind step to legacy.py

[John Snow]

On Thu, Jan 27, 2022 at 10:50 AM Hanna Reitz <hreitz@redhat.com> wrote:
>
> On 24.01.22 19:06, John Snow wrote:
> > The synchronous QMP library would bind to the server address during
> > __init__(). The new library delays this to the accept() call, because
> > binding occurs inside of the call to start_[unix_]server(), which is an
> > async method -- so it cannot happen during __init__ anymore.
> >
> > Python 3.7+ adds the ability to create the server (and thus the bind()
> > call) and begin the active listening in separate steps, but we don't
> > have that functionality in 3.6, our current minimum.
> >
> > Therefore ... Add a temporary workaround that allows the synchronous
> > version of the client to bind the socket in advance, guaranteeing that
> > there will be a UNIX socket in the filesystem ready for the QEMU client
> > to connect to without a race condition.
> >
> > (Yes, it's a bit ugly. Fixing it more nicely will have to wait until our
> > minimum Python version is 3.7+.)
>
> I mean.  Looks good to me?  Not quite enough for an R-b, I’d say, and
> you don’t really need an A-b from me on this, but looks good to me! O:)
>

Works for me, thanks!