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

[PATCH v4 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.

V4:
 - Just commit message changes, and applying Hanna's RBs.

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 v4 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>
Reviewed-by: Hanna Reitz <hreitz@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 v4 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 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 <jsnow@redhat.com>
Reviewed-by: Hanna Reitz <hreitz@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 v4 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 v4 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 v4 1/4] python/aqmp: Fix negotiation with pre-"oob" QEMU

[Kevin Wolf]

Am 01.02.2022 um 05:11 hat John Snow geschrieben:
> 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>
> Reviewed-by: Hanna Reitz <hreitz@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)

In case you have some interest in bike sheds:

As long as we only ever append a single capability, it doesn't really
make a difference and an explicit setdefault() when adding it is fine.
But if we had more than one, maybe making arguments a defaultdict(list)
would be nicer.

Not worth respinning, of course, if you don't for another reason.

Kevin

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

[Kevin Wolf]

Am 01.02.2022 um 05:11 hat John Snow geschrieben:
> 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

I feel that this is the only part that really makes it ugly. Do you
really think this way is so bad that we can't make it an official public
interface in the library?

Kevin

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

[Kevin Wolf]

Am 01.02.2022 um 05:11 hat John Snow geschrieben:
> 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.

Thanks, the new output when QEMU fails to start looks really useful!

The only thing we could probably still improve is detecting that the
QEMU process has exited instead of waiting for the socket connection to
time out. But since it only happens in case of failure, the additional
seconds of waiting are probably only a bit annoying for debugging, but
not a big problem.

Reviewed-by: Kevin Wolf <kwolf@redhat.com>

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

[John Snow]

On Tue, Feb 1, 2022 at 8:28 AM Kevin Wolf <kwolf@redhat.com> wrote:
>
> Am 01.02.2022 um 05:11 hat John Snow geschrieben:
> > 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.
>
> Thanks, the new output when QEMU fails to start looks really useful!
>
> The only thing we could probably still improve is detecting that the
> QEMU process has exited instead of waiting for the socket connection to
> time out. But since it only happens in case of failure, the additional
> seconds of waiting are probably only a bit annoying for debugging, but
> not a big problem.

That's absolutely on my radar, I assure you!

It's something that is easy to solve with asyncio:

async def launch(self, ...):
    task1 = asyncio.create_task(self.qmp.accept(...))
    task2 = asyncio.create_subprocess_exec(...)
    ret = asyncio.wait_for((task1, task2))
    ...

If either task raises, then the wait_for will also end prematurely
(and cancel the other task). I'm sure it won't actually be this easy,
but that's the general idea.

Though, that's a pretty big upheaval to the Python code again, so it's
not something I can jam in quickly. I have some light sketches that
examine a "what-if" for an asyncio-native machine.py using the asyncio
QMP code, but there are some design concerns there -- if I go through
the effort of doing this, I will want to publish that python package
upstream as well, and if I do that, it needs to be carefully thought
through in terms of a supportable interface.

Well, I'm thinking about it, anyway. For right now I am interested in
getting the qemu.qmp project out the door and onto PyPI.org, and then
I'll worry about machine improvements. If in the meantime you have a
good idea for how to fix up the machine.py code we already have, I'm
happy to take patches. Otherwise, I'll try to get to it "soon".

>
> Reviewed-by: Kevin Wolf <kwolf@redhat.com>
>

Thanks!

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

[John Snow]

On Tue, Feb 1, 2022 at 8:21 AM Kevin Wolf <kwolf@redhat.com> wrote:
>
> Am 01.02.2022 um 05:11 hat John Snow geschrieben:
> > 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
>
> I feel that this is the only part that really makes it ugly. Do you
> really think this way is so bad that we can't make it an official public
> interface in the library?
>
> Kevin
>

Good question.

I felt like I'd rather use the 'start_serving' parameter of
loop.create_server(...), added in python 3.7; see
https://docs.python.org/3/library/asyncio-eventloop.html#asyncio.loop.create_server
Python 3.6 is already EOL, but we still depend on it for our build and
I wasn't prepared to write the series that forces us on to 3.7,
because RHEL uses 3.6 as its native python. I'll have to update the
docker files, etc -- and I'm sure people will be kind of unhappy with
this, so I am putting it off. People were unhappy enough with the move
to Python 3.6.

I also felt as though the async version has no real need for a
separate bind step -- you can just start the server in a coroutine and
wait until it yields, then proceed to launch QEMU. It's easy in that
paradigm. If this bind step is only for the benefit of the legacy.py
interface, I thought maybe it wasn't wise to commit to supporting it
if it was something I wanted to get rid of soon anyway. There's also
the ugliness that if you use the early bind step, the arguments passed
to accept() are now ignored, which is kind of ugly, too. It's not a
*great* interface. It doesn't spark joy.

I have some patches that are building a "sync.py" module that's meant
to replace "legacy.py", and it's in the development of that module
that I expect to either remove the bits I am unhappy with, or commit
to supporting necessary infrastructure that's just simply required for
a functional synchronous interface to work. I planned to start
versioning the "qemu.qmp" package at 0.0.1, and the version that drops
legacy.py I intend to version at 0.1.0.

All that said, I'm fairly wishy-washy on it, so if you have some
strong feelings, lemme know. There may be some real utility in just
doubling down on always creating our own socket object. I just haven't
thought through everything here, admittedly.

--js

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

[John Snow]

On Tue, Feb 1, 2022 at 7:59 AM Kevin Wolf <kwolf@redhat.com> wrote:
>
> Am 01.02.2022 um 05:11 hat John Snow geschrieben:
> > 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>
> > Reviewed-by: Hanna Reitz <hreitz@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)
>
> In case you have some interest in bike sheds:
>
> As long as we only ever append a single capability, it doesn't really
> make a difference and an explicit setdefault() when adding it is fine.
> But if we had more than one, maybe making arguments a defaultdict(list)
> would be nicer.
>
> Not worth respinning, of course, if you don't for another reason.
>
> Kevin
>

Nope, no reason. I just forget that there's a fancier doodad. I'll add
a patch to a less-important series to shine this up.

--js

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

[Kevin Wolf]

Am 01.02.2022 um 19:32 hat John Snow geschrieben:
> On Tue, Feb 1, 2022 at 8:21 AM Kevin Wolf <kwolf@redhat.com> wrote:
> >
> > Am 01.02.2022 um 05:11 hat John Snow geschrieben:
> > > 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
> >
> > I feel that this is the only part that really makes it ugly. Do you
> > really think this way is so bad that we can't make it an official public
> > interface in the library?
> >
> > Kevin
> >
> 
> Good question.
> 
> I felt like I'd rather use the 'start_serving' parameter of
> loop.create_server(...), added in python 3.7; see
> https://docs.python.org/3/library/asyncio-eventloop.html#asyncio.loop.create_server
> Python 3.6 is already EOL, but we still depend on it for our build and
> I wasn't prepared to write the series that forces us on to 3.7,
> because RHEL uses 3.6 as its native python. I'll have to update the
> docker files, etc -- and I'm sure people will be kind of unhappy with
> this, so I am putting it off. People were unhappy enough with the move
> to Python 3.6.

Yes, I understand. In theory - I haven't really thought much about it -
it feels like whether you create a separate socket in a first step and
pass it to the server that is created in the second step (for 3.6) or
you start an idle server in the first step and then let it start serving
in the second step (for 3.7+) should be an implementation detail if we
get the external API right.

> I also felt as though the async version has no real need for a
> separate bind step -- you can just start the server in a coroutine and
> wait until it yields, then proceed to launch QEMU. It's easy in that
> paradigm. If this bind step is only for the benefit of the legacy.py
> interface, I thought maybe it wasn't wise to commit to supporting it
> if it was something I wanted to get rid of soon anyway. There's also
> the ugliness that if you use the early bind step, the arguments passed
> to accept() are now ignored, which is kind of ugly, too. It's not a
> *great* interface. It doesn't spark joy.

Right, that's probably a sign that the interfaces aren't completely
right. I'm not sure which interfaces. As long as it's just _bind_hack()
and it's only used in an API that is going away in the future, that's no
problem. But it could also be a sign that the public interfaces aren't
flexible enough.

> I have some patches that are building a "sync.py" module that's meant
> to replace "legacy.py", and it's in the development of that module
> that I expect to either remove the bits I am unhappy with, or commit
> to supporting necessary infrastructure that's just simply required for
> a functional synchronous interface to work. I planned to start
> versioning the "qemu.qmp" package at 0.0.1, and the version that drops
> legacy.py I intend to version at 0.1.0.

If I understand these version numbers correctly, this also implies that
there is no compatibility promise yet. So maybe what to make a public
interface isn't even a big concern yet.

I guess the relevant question in the context of this patch is whether
sync.py will need a similar two-phase setup as legacy.py. Do you think
you can do without it when you have to reintroduce this behaviour here
to fix bugs?

> All that said, I'm fairly wishy-washy on it, so if you have some
> strong feelings, lemme know. There may be some real utility in just
> doubling down on always creating our own socket object. I just haven't
> thought through everything here, admittedly.

No, I don't have strong feelings about this. I don't even really know
the code. I just saw that you were unhappy with the solution and thought
I could try to be the rubber duck that makes you figure out something
that you're happy with. ;-)

Kevin

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

[John Snow]

On Tue, Feb 1, 2022 at 2:46 PM Kevin Wolf <kwolf@redhat.com> wrote:
>
> Am 01.02.2022 um 19:32 hat John Snow geschrieben:
> > On Tue, Feb 1, 2022 at 8:21 AM Kevin Wolf <kwolf@redhat.com> wrote:
> > >
> > > Am 01.02.2022 um 05:11 hat John Snow geschrieben:
> > > > 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
> > >
> > > I feel that this is the only part that really makes it ugly. Do you
> > > really think this way is so bad that we can't make it an official public
> > > interface in the library?
> > >
> > > Kevin
> > >
> >
> > Good question.
> >
> > I felt like I'd rather use the 'start_serving' parameter of
> > loop.create_server(...), added in python 3.7; see
> > https://docs.python.org/3/library/asyncio-eventloop.html#asyncio.loop.create_server
> > Python 3.6 is already EOL, but we still depend on it for our build and
> > I wasn't prepared to write the series that forces us on to 3.7,
> > because RHEL uses 3.6 as its native python. I'll have to update the
> > docker files, etc -- and I'm sure people will be kind of unhappy with
> > this, so I am putting it off. People were unhappy enough with the move
> > to Python 3.6.
>
> Yes, I understand. In theory - I haven't really thought much about it -
> it feels like whether you create a separate socket in a first step and
> pass it to the server that is created in the second step (for 3.6) or
> you start an idle server in the first step and then let it start serving
> in the second step (for 3.7+) should be an implementation detail if we
> get the external API right.
>
> > I also felt as though the async version has no real need for a
> > separate bind step -- you can just start the server in a coroutine and
> > wait until it yields, then proceed to launch QEMU. It's easy in that
> > paradigm. If this bind step is only for the benefit of the legacy.py
> > interface, I thought maybe it wasn't wise to commit to supporting it
> > if it was something I wanted to get rid of soon anyway. There's also
> > the ugliness that if you use the early bind step, the arguments passed
> > to accept() are now ignored, which is kind of ugly, too. It's not a
> > *great* interface. It doesn't spark joy.
>
> Right, that's probably a sign that the interfaces aren't completely
> right. I'm not sure which interfaces. As long as it's just _bind_hack()
> and it's only used in an API that is going away in the future, that's no
> problem. But it could also be a sign that the public interfaces aren't
> flexible enough.
>

Yeah, I agree. It's not flexible enough. I think the sync.py
development will force me to understand where I have come to rest on
the conveniences of asyncio and force the design to be more flexible
overall.

> > I have some patches that are building a "sync.py" module that's meant
> > to replace "legacy.py", and it's in the development of that module
> > that I expect to either remove the bits I am unhappy with, or commit
> > to supporting necessary infrastructure that's just simply required for
> > a functional synchronous interface to work. I planned to start
> > versioning the "qemu.qmp" package at 0.0.1, and the version that drops
> > legacy.py I intend to version at 0.1.0.
>
> If I understand these version numbers correctly, this also implies that
> there is no compatibility promise yet. So maybe what to make a public
> interface isn't even a big concern yet.

Right. (Still, I didn't want to upload anything to PyPI I just
outwardly had no intention of supporting at all. It felt polite, even
in "alpha".)

>
> I guess the relevant question in the context of this patch is whether
> sync.py will need a similar two-phase setup as legacy.py. Do you think
> you can do without it when you have to reintroduce this behaviour here
> to fix bugs?
>

Hm, honestly, no. I think you'll still need the two-phase in the sync
version no matter what -- if you expect to be able to launch QMP and
QEMU from the same process, anyway. I suppose it's just a matter of
what you *call* it and what/where the arguments are.

I could just call it bind(), and it does whatever it needs to (Either
creating a socket or, in 3.7+, instantiating more of the asyncio loop
machinery).
The signature for accept() would need to change to (perhaps)
optionally accepting no arguments; i.e. you can do either of the
following:

(1) qmp.bind('/tmp/sock')
    qmp.accept()

(2) qmp.accept('/tmp/sock')

The risk is that the interface is just a touch more complex, the docs
get a bit more cluttered, there's a slight loss of clarity, the
accept() method would need to check to make sure you didn't give it an
address argument if you've already given it one, etc. Necessary
trade-off for the flexibility, though.

I've got some ideas, thanks.

> > All that said, I'm fairly wishy-washy on it, so if you have some
> > strong feelings, lemme know. There may be some real utility in just
> > doubling down on always creating our own socket object. I just haven't
> > thought through everything here, admittedly.
>
> No, I don't have strong feelings about this. I don't even really know
> the code. I just saw that you were unhappy with the solution and thought
> I could try to be the rubber duck that makes you figure out something
> that you're happy with. ;-)

I appreciate it! There's some improvements I can make, but I think
there's still time to polish it yet, and for right now I'll just push
ahead with the hack to get Peter's tests green.

>
> Kevin
>

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

[Kevin Wolf]

Am 02.02.2022 um 20:08 hat John Snow geschrieben:
> > I guess the relevant question in the context of this patch is whether
> > sync.py will need a similar two-phase setup as legacy.py. Do you think
> > you can do without it when you have to reintroduce this behaviour here
> > to fix bugs?
> >
> 
> Hm, honestly, no. I think you'll still need the two-phase in the sync
> version no matter what -- if you expect to be able to launch QMP and
> QEMU from the same process, anyway. I suppose it's just a matter of
> what you *call* it and what/where the arguments are.
> 
> I could just call it bind(), and it does whatever it needs to (Either
> creating a socket or, in 3.7+, instantiating more of the asyncio loop
> machinery).
> The signature for accept() would need to change to (perhaps)
> optionally accepting no arguments; i.e. you can do either of the
> following:
> 
> (1) qmp.bind('/tmp/sock')
>     qmp.accept()
> 
> (2) qmp.accept('/tmp/sock')
> 
> The risk is that the interface is just a touch more complex, the docs
> get a bit more cluttered, there's a slight loss of clarity, the
> accept() method would need to check to make sure you didn't give it an
> address argument if you've already given it one, etc. Necessary
> trade-off for the flexibility, though.

Hm, how about copying the create_server() interface instead? So it
would become:

(1) qmp.create_server('/tmp/sock', start_serving=False)
    qmp.start_serving()

(2) qmp.create_server('/tmp/sock')

Then you get the connection details only in a single place. (The names
should probably be changed because we're still a QMP client even though
we're technically the server on the socket level, but you get the idea.)

Kevin

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

[Daniel P. Berrangé]

On Wed, Feb 02, 2022 at 02:08:59PM -0500, John Snow wrote:
> On Tue, Feb 1, 2022 at 2:46 PM Kevin Wolf <kwolf@redhat.com> wrote:
> >
> > Am 01.02.2022 um 19:32 hat John Snow geschrieben:
> > > On Tue, Feb 1, 2022 at 8:21 AM Kevin Wolf <kwolf@redhat.com> wrote:
> > > >
> > > > Am 01.02.2022 um 05:11 hat John Snow geschrieben:
> > > > > 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 guess the relevant question in the context of this patch is whether
> > sync.py will need a similar two-phase setup as legacy.py. Do you think
> > you can do without it when you have to reintroduce this behaviour here
> > to fix bugs?
> >
> 
> Hm, honestly, no. I think you'll still need the two-phase in the sync
> version no matter what -- if you expect to be able to launch QMP and
> QEMU from the same process, anyway. I suppose it's just a matter of
> what you *call* it and what/where the arguments are.
> 
> I could just call it bind(), and it does whatever it needs to (Either
> creating a socket or, in 3.7+, instantiating more of the asyncio loop
> machinery).
> The signature for accept() would need to change to (perhaps)
> optionally accepting no arguments; i.e. you can do either of the
> following:
> 
> (1) qmp.bind('/tmp/sock')
>     qmp.accept()
> 
> (2) qmp.accept('/tmp/sock')
> 
> The risk is that the interface is just a touch more complex, the docs
> get a bit more cluttered, there's a slight loss of clarity, the
> accept() method would need to check to make sure you didn't give it an
> address argument if you've already given it one, etc. Necessary
> trade-off for the flexibility, though.

Doing a bind() inside an accept() call is really strange
design IMHO.

bind() is a one-time-only initialization operation for startup,
where as accept() is a runtime operation invoked repeatedly.

bind() is also an op that is reasonably likely to fail
due to something already holding the socket address, and thus
an error condition that you want to report to an application
during its early startup phase.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

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

[John Snow]

On Thu, Feb 3, 2022 at 4:38 AM Daniel P. Berrangé <berrange@redhat.com> wrote:
>
> On Wed, Feb 02, 2022 at 02:08:59PM -0500, John Snow wrote:
> > On Tue, Feb 1, 2022 at 2:46 PM Kevin Wolf <kwolf@redhat.com> wrote:
> > >
> > > Am 01.02.2022 um 19:32 hat John Snow geschrieben:
> > > > On Tue, Feb 1, 2022 at 8:21 AM Kevin Wolf <kwolf@redhat.com> wrote:
> > > > >
> > > > > Am 01.02.2022 um 05:11 hat John Snow geschrieben:
> > > > > > 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 guess the relevant question in the context of this patch is whether
> > > sync.py will need a similar two-phase setup as legacy.py. Do you think
> > > you can do without it when you have to reintroduce this behaviour here
> > > to fix bugs?
> > >
> >
> > Hm, honestly, no. I think you'll still need the two-phase in the sync
> > version no matter what -- if you expect to be able to launch QMP and
> > QEMU from the same process, anyway. I suppose it's just a matter of
> > what you *call* it and what/where the arguments are.
> >
> > I could just call it bind(), and it does whatever it needs to (Either
> > creating a socket or, in 3.7+, instantiating more of the asyncio loop
> > machinery).
> > The signature for accept() would need to change to (perhaps)
> > optionally accepting no arguments; i.e. you can do either of the
> > following:
> >
> > (1) qmp.bind('/tmp/sock')
> >     qmp.accept()
> >
> > (2) qmp.accept('/tmp/sock')
> >
> > The risk is that the interface is just a touch more complex, the docs
> > get a bit more cluttered, there's a slight loss of clarity, the
> > accept() method would need to check to make sure you didn't give it an
> > address argument if you've already given it one, etc. Necessary
> > trade-off for the flexibility, though.
>
> Doing a bind() inside an accept() call is really strange
> design IMHO.
>

Yeah, it was possibly a mistake to use the same names as the very
famous and widely known POSIX calls. I didn't intend for them to be
precisely 1:1.

What I intended was that each QMPClient() object could either receive
an incoming connection or dial out and make one. I happened to name
these accept() and connect(). Under the hood, we do "whatever we have
to" in order to make those methods work. This was the simplest
possible interface I could imagine; and it also mimics the (sync) qmp
code in python/qemu/qmp. That code doesn't have distinct
bind-listen-accept steps; it performs bind and listen immediately
during __init__, then socket.accept() is called during the accept()
method call.

However, In python asyncio, we're encouraged to use the
asyncio.start_server() and asyncio.start_unix_server() functions
instead. These are async calls, and they cannot occur during
__init__(), because __init__() isn't an async coroutine. This compels
us to move the bind step out of init and into an async method. In the
interest of keeping the new interface looking very similar to the old
one, I pushed the entirety of the bind-listen-accept sequence into the
async accept(...) method.

A little weird, but that's why it happened that way. (The concept of a
QMP client being the socket server is also a little weird and defies
the traditional expectation, too, IMO. Oh well, we're here now.)

For comparison, the sync interface:

qmp = QEMUMonitorProtocol('qemu.sock', server=True)  # bind(), listen()
qmp.accept()  # accept()

And the async interface:

qmp = QMPClient()
await qmp.accept('qemu.sock')  # bind, listen, and accept

That said, I could probably make this interface a lot more traditional
for actual /servers/. I have patches to add a QMPServer() class, but
it adopted the "exactly one peer" limitation that QMPClient() has. If
you think there is some value in developing the idea further, I could
refactor the code to be a *lot* more traditional and have a listening
server that acts as a factory for generating connected client
instances. It just isn't something that I think I'll have merge ready
in less than a week.

> bind() is a one-time-only initialization operation for startup,
> where as accept() is a runtime operation invoked repeatedly.
>
> bind() is also an op that is reasonably likely to fail
> due to something already holding the socket address, and thus
> an error condition that you want to report to an application
> during its early startup phase.

Yeah, oops. I was focused on keeping the number of public calls low,
but that didn't help me out here.

Lemme see what I can do.

--js

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

[John Snow]

On Thu, Feb 3, 2022 at 4:19 AM Kevin Wolf <kwolf@redhat.com> wrote:
>
> Am 02.02.2022 um 20:08 hat John Snow geschrieben:
> > > I guess the relevant question in the context of this patch is whether
> > > sync.py will need a similar two-phase setup as legacy.py. Do you think
> > > you can do without it when you have to reintroduce this behaviour here
> > > to fix bugs?
> > >
> >
> > Hm, honestly, no. I think you'll still need the two-phase in the sync
> > version no matter what -- if you expect to be able to launch QMP and
> > QEMU from the same process, anyway. I suppose it's just a matter of
> > what you *call* it and what/where the arguments are.
> >
> > I could just call it bind(), and it does whatever it needs to (Either
> > creating a socket or, in 3.7+, instantiating more of the asyncio loop
> > machinery).
> > The signature for accept() would need to change to (perhaps)
> > optionally accepting no arguments; i.e. you can do either of the
> > following:
> >
> > (1) qmp.bind('/tmp/sock')
> >     qmp.accept()
> >
> > (2) qmp.accept('/tmp/sock')
> >
> > The risk is that the interface is just a touch more complex, the docs
> > get a bit more cluttered, there's a slight loss of clarity, the
> > accept() method would need to check to make sure you didn't give it an
> > address argument if you've already given it one, etc. Necessary
> > trade-off for the flexibility, though.
>
> Hm, how about copying the create_server() interface instead? So it
> would become:
>
> (1) qmp.create_server('/tmp/sock', start_serving=False)
>     qmp.start_serving()
>
> (2) qmp.create_server('/tmp/sock')
>
> Then you get the connection details only in a single place. (The names
> should probably be changed because we're still a QMP client even though
> we're technically the server on the socket level, but you get the idea.)

Good idea. I'll experiment.

(I'm worried that the way I initially factored the code to begin with
might make this ugly, but maybe I'm just dreading nothing. I'll have
to see.)

Thanks for taking a look!

--js