[PATCH v2 0/5] Python: minor fixes

qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed

* [PATCH v2 0/5] Python: minor fixes
@ 2022-01-19 19:39 John Snow
  2022-01-19 19:39 ` [PATCH v2 1/5] python/aqmp: Fix negotiation with pre-"oob" QEMU John Snow
                   ` (4 more replies)
  0 siblings, 5 replies; 10+ messages in thread
From: John Snow @ 2022-01-19 19:39 UTC (permalink / raw)
  To: qemu-devel
  Cc: Kevin Wolf, Eduardo Habkost, Beraldo Leal, qemu-block,
	Peter Maydell, Markus Armbruster, Hanna Reitz, Cleber Rosa,
	John Snow

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

Fix a couple AQMP bugs and improve some minor irritants.

V2:
 - Hack-fix a race condition inherent
   between machine.py and aqmp/legacy.py
 - Improve error reporting from QEMUMachine.launch()

John Snow (5):
  python/aqmp: Fix negotiation with pre-"oob" QEMU
  python: use avocado's "new" runner
  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/avocado.cfg                        |  2 +-
 python/qemu/aqmp/legacy.py                |  3 ++
 python/qemu/aqmp/protocol.py              | 41 ++++++++++++--
 python/qemu/aqmp/qmp_client.py            |  4 +-
 python/qemu/machine/machine.py            | 44 ++++++++++++---
 python/setup.cfg                          |  2 +-
 tests/qemu-iotests/tests/mirror-top-perms |  3 +-
 8 files changed, 123 insertions(+), 42 deletions(-)

-- 
2.31.1




^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH v2 1/5] python/aqmp: Fix negotiation with pre-"oob" QEMU
  2022-01-19 19:39 [PATCH v2 0/5] Python: minor fixes John Snow
@ 2022-01-19 19:39 ` John Snow
  2022-01-19 19:39 ` [PATCH v2 2/5] python: use avocado's "new" runner John Snow
                   ` (3 subsequent siblings)
  4 siblings, 0 replies; 10+ messages in thread
From: John Snow @ 2022-01-19 19:39 UTC (permalink / raw)
  To: qemu-devel
  Cc: Kevin Wolf, Eduardo Habkost, Beraldo Leal, qemu-block,
	Peter Maydell, Markus Armbruster, Hanna Reitz, Cleber Rosa,
	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 8105e29fa8..6b43e1dbbe 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



^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH v2 2/5] python: use avocado's "new" runner
  2022-01-19 19:39 [PATCH v2 0/5] Python: minor fixes John Snow
  2022-01-19 19:39 ` [PATCH v2 1/5] python/aqmp: Fix negotiation with pre-"oob" QEMU John Snow
@ 2022-01-19 19:39 ` John Snow
  2022-01-20 13:06   ` Beraldo Leal
  2022-01-19 19:39 ` [PATCH v2 3/5] python/machine: raise VMLaunchFailure exception from launch() John Snow
                   ` (2 subsequent siblings)
  4 siblings, 1 reply; 10+ messages in thread
From: John Snow @ 2022-01-19 19:39 UTC (permalink / raw)
  To: qemu-devel
  Cc: Kevin Wolf, Eduardo Habkost, Beraldo Leal, qemu-block,
	Peter Maydell, Markus Armbruster, Hanna Reitz, Cleber Rosa,
	John Snow

The old legacy runner no longer seems to work with output logging, so we
can't see failure logs when a test case fails. The new runner doesn't
(seem to) support Coverage.py yet, but seeing error output is a more
important feature.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 python/avocado.cfg | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/python/avocado.cfg b/python/avocado.cfg
index c7722e7ecd..a460420059 100644
--- a/python/avocado.cfg
+++ b/python/avocado.cfg
@@ -1,5 +1,5 @@
 [run]
-test_runner = runner
+test_runner = nrunner
 
 [simpletests]
 # Don't show stdout/stderr in the test *summary*
-- 
2.31.1



^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH v2 2/5] python: use avocado's "new" runner
  2022-01-19 19:39 ` [PATCH v2 2/5] python: use avocado's "new" runner John Snow
@ 2022-01-20 13:06   ` Beraldo Leal
  2022-01-20 21:44     ` John Snow
  0 siblings, 1 reply; 10+ messages in thread
From: Beraldo Leal @ 2022-01-20 13:06 UTC (permalink / raw)
  To: John Snow
  Cc: Kevin Wolf, Eduardo Habkost, qemu-block, Peter Maydell,
	qemu-devel, Markus Armbruster, Hanna Reitz, Cleber Rosa

On Wed, Jan 19, 2022 at 02:39:13PM -0500, John Snow wrote:
> The old legacy runner no longer seems to work with output logging, so we
> can't see failure logs when a test case fails. The new runner doesn't
> (seem to) support Coverage.py yet, but seeing error output is a more
> important feature.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  python/avocado.cfg | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/python/avocado.cfg b/python/avocado.cfg
> index c7722e7ecd..a460420059 100644
> --- a/python/avocado.cfg
> +++ b/python/avocado.cfg
> @@ -1,5 +1,5 @@
>  [run]
> -test_runner = runner
> +test_runner = nrunner
>  
>  [simpletests]
>  # Don't show stdout/stderr in the test *summary*

Since Avocado 82, the new one is the default. So, you could remove the
the "[run]" section.

In any case:

Reviewed-by: Beraldo Leal <bleal@redhat.com>

--
Beraldo



^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2 2/5] python: use avocado's "new" runner
  2022-01-20 13:06   ` Beraldo Leal
@ 2022-01-20 21:44     ` John Snow
  0 siblings, 0 replies; 10+ messages in thread
From: John Snow @ 2022-01-20 21:44 UTC (permalink / raw)
  To: Beraldo Leal
  Cc: Kevin Wolf, Eduardo Habkost, Qemu-block, Peter Maydell,
	qemu-devel, Markus Armbruster, Hanna Reitz, Cleber Rosa

On Thu, Jan 20, 2022 at 8:08 AM Beraldo Leal <bleal@redhat.com> wrote:
>
> On Wed, Jan 19, 2022 at 02:39:13PM -0500, John Snow wrote:
> > The old legacy runner no longer seems to work with output logging, so we
> > can't see failure logs when a test case fails. The new runner doesn't
> > (seem to) support Coverage.py yet, but seeing error output is a more
> > important feature.
> >
> > Signed-off-by: John Snow <jsnow@redhat.com>
> > ---
> >  python/avocado.cfg | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/python/avocado.cfg b/python/avocado.cfg
> > index c7722e7ecd..a460420059 100644
> > --- a/python/avocado.cfg
> > +++ b/python/avocado.cfg
> > @@ -1,5 +1,5 @@
> >  [run]
> > -test_runner = runner
> > +test_runner = nrunner
> >
> >  [simpletests]
> >  # Don't show stdout/stderr in the test *summary*
>
> Since Avocado 82, the new one is the default. So, you could remove the
> the "[run]" section.
>

I think it was actually since 91.0, but I figured it was more obvious
to reviewers to see the explicit change. Less to explain, and it will
explode a little more if you use an avocado old enough that doesn't
have the nrunner.

> In any case:
>
> Reviewed-by: Beraldo Leal <bleal@redhat.com>
>

Thanks,
--js



^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH v2 3/5] python/machine: raise VMLaunchFailure exception from launch()
  2022-01-19 19:39 [PATCH v2 0/5] Python: minor fixes John Snow
  2022-01-19 19:39 ` [PATCH v2 1/5] python/aqmp: Fix negotiation with pre-"oob" QEMU John Snow
  2022-01-19 19:39 ` [PATCH v2 2/5] python: use avocado's "new" runner John Snow
@ 2022-01-19 19:39 ` John Snow
  2022-01-19 19:39 ` [PATCH v2 4/5] python: upgrade mypy to 0.780 John Snow
  2022-01-19 19:39 ` [PATCH v2 5/5] python/aqmp: add socket bind step to legacy.py John Snow
  4 siblings, 0 replies; 10+ messages in thread
From: John Snow @ 2022-01-19 19:39 UTC (permalink / raw)
  To: qemu-devel
  Cc: Kevin Wolf, Eduardo Habkost, Beraldo Leal, qemu-block,
	Peter Maydell, Markus Armbruster, Hanna Reitz, Cleber Rosa,
	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).

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

diff --git a/python/qemu/machine/machine.py b/python/qemu/machine/machine.py
index 67ab06ca2b..5b76ee9a36 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 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,14 @@ 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
+
+            # Leave BaseException priority exceptions alone.
             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



^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH v2 4/5] python: upgrade mypy to 0.780
  2022-01-19 19:39 [PATCH v2 0/5] Python: minor fixes John Snow
                   ` (2 preceding siblings ...)
  2022-01-19 19:39 ` [PATCH v2 3/5] python/machine: raise VMLaunchFailure exception from launch() John Snow
@ 2022-01-19 19:39 ` John Snow
  2022-01-19 19:39 ` [PATCH v2 5/5] python/aqmp: add socket bind step to legacy.py John Snow
  4 siblings, 0 replies; 10+ messages in thread
From: John Snow @ 2022-01-19 19:39 UTC (permalink / raw)
  To: qemu-devel
  Cc: Kevin Wolf, Eduardo Habkost, Beraldo Leal, qemu-block,
	Peter Maydell, Markus Armbruster, Hanna Reitz, Cleber Rosa,
	John Snow

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

(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.)

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 417e937839..4f4f20571f 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



^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH v2 5/5] python/aqmp: add socket bind step to legacy.py
  2022-01-19 19:39 [PATCH v2 0/5] Python: minor fixes John Snow
                   ` (3 preceding siblings ...)
  2022-01-19 19:39 ` [PATCH v2 4/5] python: upgrade mypy to 0.780 John Snow
@ 2022-01-19 19:39 ` John Snow
  2022-01-20  9:13   ` Daniel P. Berrangé
  4 siblings, 1 reply; 10+ messages in thread
From: John Snow @ 2022-01-19 19:39 UTC (permalink / raw)
  To: qemu-devel
  Cc: Kevin Wolf, Eduardo Habkost, Beraldo Leal, qemu-block,
	Peter Maydell, Markus Armbruster, Hanna Reitz, Cleber Rosa,
	John Snow

The old QMP library would actually 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 ugly; fixing it more nicely will unfortunately have to wait
until I can stipulate Python 3.7+ as our minimum version. Python 3.6 is
EOL as of the beginning of this year, but I haven't checked if all of
our supported build platforms have a properly modern Python available
yet.)

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 9e7b9fb80b..cf7634ee95 100644
--- a/python/qemu/aqmp/legacy.py
+++ b/python/qemu/aqmp/legacy.py
@@ -33,6 +33,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 c4fbe35a0e..eb740a5452 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,
@@ -234,6 +235,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 = []
@@ -423,6 +427,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: Union[str, Tuple[str, int]],
                          ssl: Optional[SSLContext] = None) -> None:
@@ -460,24 +492,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



^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH v2 5/5] python/aqmp: add socket bind step to legacy.py
  2022-01-19 19:39 ` [PATCH v2 5/5] python/aqmp: add socket bind step to legacy.py John Snow
@ 2022-01-20  9:13   ` Daniel P. Berrangé
  2022-01-20 17:11     ` John Snow
  0 siblings, 1 reply; 10+ messages in thread
From: Daniel P. Berrangé @ 2022-01-20  9:13 UTC (permalink / raw)
  To: John Snow
  Cc: Kevin Wolf, Eduardo Habkost, Beraldo Leal, qemu-block,
	Peter Maydell, qemu-devel, Markus Armbruster, Hanna Reitz,
	Cleber Rosa

On Wed, Jan 19, 2022 at 02:39:16PM -0500, John Snow wrote:
> The old QMP library would actually 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 ugly; fixing it more nicely will unfortunately have to wait
> until I can stipulate Python 3.7+ as our minimum version. Python 3.6 is
> EOL as of the beginning of this year, but I haven't checked if all of
> our supported build platforms have a properly modern Python available
> yet.)

RHEL-8 system python will remain 3.6 for the life of RHEL-8.

While you can bring in newer python versions in parallel,
IMHO it is highly desirable to remain compatible with the
system python as that's the one you can guarantee users
actually have available by default.


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 :|



^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2 5/5] python/aqmp: add socket bind step to legacy.py
  2022-01-20  9:13   ` Daniel P. Berrangé
@ 2022-01-20 17:11     ` John Snow
  0 siblings, 0 replies; 10+ messages in thread
From: John Snow @ 2022-01-20 17:11 UTC (permalink / raw)
  To: Daniel P. Berrangé
  Cc: Kevin Wolf, Eduardo Habkost, Beraldo Leal, Qemu-block,
	Peter Maydell, qemu-devel, Markus Armbruster, Hanna Reitz,
	Cleber Rosa

[-- Attachment #1: Type: text/plain, Size: 2576 bytes --]

On Thu, Jan 20, 2022, 4:13 AM Daniel P. Berrangé <berrange@redhat.com>
wrote:

> On Wed, Jan 19, 2022 at 02:39:16PM -0500, John Snow wrote:
> > The old QMP library would actually 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 ugly; fixing it more nicely will unfortunately have to wait
> > until I can stipulate Python 3.7+ as our minimum version. Python 3.6 is
> > EOL as of the beginning of this year, but I haven't checked if all of
> > our supported build platforms have a properly modern Python available
> > yet.)
>
> RHEL-8 system python will remain 3.6 for the life of RHEL-8.
>
> While you can bring in newer python versions in parallel,
> IMHO it is highly desirable to remain compatible with the
> system python as that's the one you can guarantee users
> actually have available by default.
>

I agree, but over time my hand will be forced. Libraries are beginning to
drop support for Python 3.6 upstream, and it's only a matter of time before
it becomes implausible to support an EOL python version.

I actually go out of my way to ensure compatibility with the very oldest
versions I possibly can - *extremely* out of my way - but there's only so
much I can reasonably do. Supporting 3.6 and 3.11 simultaneously may prove
challenging.

Either way, I'm not bumping the version here in this series. I'm just
stating that this hack is kind of the best I can (quickly and easily) do
until 3.7.

(3.7 adds start_server=False to start_unix_server which allows the
separation of steps without needing to muck around with the socket object.)


>
>
> 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 :
>

[-- Attachment #2: Type: text/html, Size: 4118 bytes --]

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2022-01-21  0:43 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-01-19 19:39 [PATCH v2 0/5] Python: minor fixes John Snow
2022-01-19 19:39 ` [PATCH v2 1/5] python/aqmp: Fix negotiation with pre-"oob" QEMU John Snow
2022-01-19 19:39 ` [PATCH v2 2/5] python: use avocado's "new" runner John Snow
2022-01-20 13:06   ` Beraldo Leal
2022-01-20 21:44     ` John Snow
2022-01-19 19:39 ` [PATCH v2 3/5] python/machine: raise VMLaunchFailure exception from launch() John Snow
2022-01-19 19:39 ` [PATCH v2 4/5] python: upgrade mypy to 0.780 John Snow
2022-01-19 19:39 ` [PATCH v2 5/5] python/aqmp: add socket bind step to legacy.py John Snow
2022-01-20  9:13   ` Daniel P. Berrangé
2022-01-20 17:11     ` John Snow

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).