From fa73e6e4ca1a93c5bbf9d05fb2a25736ab810b35 Mon Sep 17 00:00:00 2001 From: John Snow Date: Mon, 31 Jan 2022 23:11:31 -0500 Subject: [PATCH 1/4] python/aqmp: Fix negotiation with pre-"oob" QEMU 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 Reviewed-by: Hanna Reitz Reviewed-by: Kevin Wolf Message-id: 20220201041134.1237016-2-jsnow@redhat.com Signed-off-by: John Snow --- 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 @@ class QMPClient(AsyncProtocol[Message], Events): """ 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 From 50465f94d211beabfbfc80e4f85ec4fad0757570 Mon Sep 17 00:00:00 2001 From: John Snow Date: Mon, 31 Jan 2022 23:11:32 -0500 Subject: [PATCH 2/4] python/machine: raise VMLaunchFailure exception from launch() 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 in _post_shutdown() 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 catches a broader array of Exception events, including SystemExit and KeyboardInterrupt. We do not want to "wrap" such exceptions as a VMLaunchFailure, because that will 'downgrade' the exception from a BaseException to a regular Exception. We do, however, want to perform cleanup in either case, so catch on the broadest scope and wrap-and-re-raise only in the more targeted scope.) Signed-off-by: John Snow Reviewed-by: Hanna Reitz Reviewed-by: Kevin Wolf Message-id: 20220201041134.1237016-3-jsnow@redhat.com Signed-off-by: John Snow --- 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 @@ class QEMUMachine: 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 @@ class QEMUMachine: 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', From 74a1505d279897d2a448c876820a33cbe1f0f22e Mon Sep 17 00:00:00 2001 From: John Snow Date: Mon, 31 Jan 2022 23:11:33 -0500 Subject: [PATCH 3/4] python: upgrade mypy to 0.780 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 Reviewed-by: Kevin Wolf Message-id: 20220201041134.1237016-4-jsnow@redhat.com Signed-off-by: John Snow --- 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 From b0b662bb2b340d63529672b5bdae596a6243c4d0 Mon Sep 17 00:00:00 2001 From: John Snow Date: Mon, 31 Jan 2022 23:11:34 -0500 Subject: [PATCH 4/4] python/aqmp: add socket bind step to legacy.py 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 Reviewed-by: Kevin Wolf Message-id: 20220201041134.1237016-5-jsnow@redhat.com Signed-off-by: John Snow --- 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 @@ class QEMUMonitorProtocol(qemu.qmp.QEMUMonitorProtocol): 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 asyncio import StreamReader, StreamWriter from enum import Enum from functools import wraps import logging +import socket from ssl import SSLContext from typing import ( Any, @@ -238,6 +239,9 @@ class AsyncProtocol(Generic[T]): 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 @@ class AsyncProtocol(Generic[T]): 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 @@ class AsyncProtocol(Generic[T]): 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.")