[PATCH v2 00/25] Python: delete synchronous qemu.qmp package

[PATCH v2 00/25] Python: delete synchronous qemu.qmp package

[John Snow]

GitLab: https://gitlab.com/jsnow/qemu/-/commits/python-qmp-legacy-switch
CI: https://gitlab.com/jsnow/qemu/-/pipelines/430491195

Hi, this series is part of an effort to publish the qemu.qmp package on
PyPI. It is the first of three series to complete this work:

--> (1) Switch the new Async QMP library in to python/qemu/qmp
    (2) Fork python/qemu/qmp out into its own repository,
        with updated GitLab CI/CD targets to build packages.
    (3) Update qemu.git to install qemu.qmp from PyPI,
        and then delete python/qemu/qmp.

This series swaps out qemu.qmp for qemu.aqmp permanently, instead of
hiding it behind an environment variable toggle. This leaves us with
just one QMP library to worry about. It also implements the rename of
"qemu.aqmp" to "qemu.qmp".

Who should review what?
  - iotests/block maintainers (Hanna, Vladimir, and Kevin):
    please look at patches 15-18.
  - Dan, please glance at patches 13-14.
  - Anyone who cares about Python: whatever you're willing to
    stomach. We're recently down Willian and Eduardo, so there's not
    many dedicated Python reviewers...

I suspect the most potential disruption to iotest and avocado
maintainers, as those two subsystems rely on the QMP features the
most. Would appreciate at least an ACK from each of those camps if
you're willing to give benefit-of-the-doubt on the actual Python code.

V2:
 - Integrate the renaming of qemu.aqmp to qemu.qmp in this series
 - Minor bits and pieces.

John Snow (25):
  python/aqmp: add __del__ method to legacy interface
  python/aqmp: handle asyncio.TimeoutError on execute()
  python/aqmp: copy type definitions from qmp
  python/aqmp: add SocketAddrT to package root
  python/aqmp: rename AQMPError to QMPError
  python/qemu-ga-client: update instructions to newer CLI syntax
  python/qmp: switch qemu-ga-client to AQMP
  python/qmp: switch qom tools to AQMP
  python/qmp: switch qmp-shell to AQMP
  python: move qmp utilities to python/qemu/utils
  python: move qmp-shell under the AQMP package
  python/machine: permanently switch to AQMP
  scripts/cpu-x86-uarch-abi: fix CLI parsing
  scripts/cpu-x86-uarch-abi: switch to AQMP
  scripts/render-block-graph: switch to AQMP
  scripts/bench-block-job: switch to AQMP
  iotests/mirror-top-perms: switch to AQMP
  iotests: switch to AQMP
  python: temporarily silence pylint duplicate-code warnings
  python/aqmp: take QMPBadPortError and parse_address from qemu.qmp
  python/aqmp: fully separate from qmp.QEMUMonitorProtocol
  python/aqmp: copy qmp docstrings to qemu.aqmp.legacy
  python: remove the old QMP package
  python: re-enable pylint duplicate-code warnings
  python: rename qemu.aqmp to qemu.qmp

 python/qemu/qmp/README.rst                   |   9 -
 python/qemu/aqmp/__init__.py                 |  51 ---
 python/qemu/aqmp/legacy.py                   | 138 ------
 python/qemu/aqmp/py.typed                    |   0
 python/qemu/machine/machine.py               |  18 +-
 python/qemu/machine/qtest.py                 |   2 +-
 python/qemu/qmp/__init__.py                  | 441 ++-----------------
 python/qemu/{aqmp => qmp}/aqmp_tui.py        |   2 +-
 python/qemu/{aqmp => qmp}/error.py           |  12 +-
 python/qemu/{aqmp => qmp}/events.py          |   6 +-
 python/qemu/qmp/legacy.py                    | 319 ++++++++++++++
 python/qemu/{aqmp => qmp}/message.py         |   0
 python/qemu/{aqmp => qmp}/models.py          |   0
 python/qemu/{aqmp => qmp}/protocol.py        |  28 +-
 python/qemu/{aqmp => qmp}/qmp_client.py      |  32 +-
 python/qemu/qmp/qmp_shell.py                 |  31 +-
 python/qemu/{aqmp => qmp}/util.py            |   0
 python/qemu/{qmp => utils}/qemu_ga_client.py |  24 +-
 python/qemu/{qmp => utils}/qom.py            |   5 +-
 python/qemu/{qmp => utils}/qom_common.py     |   3 +-
 python/qemu/{qmp => utils}/qom_fuse.py       |  11 +-
 python/setup.cfg                             |  21 +-
 python/tests/protocol.py                     |  14 +-
 scripts/cpu-x86-uarch-abi.py                 |   7 +-
 scripts/device-crash-test                    |   4 +-
 scripts/qmp/qemu-ga-client                   |   2 +-
 scripts/qmp/qom-fuse                         |   2 +-
 scripts/qmp/qom-get                          |   2 +-
 scripts/qmp/qom-list                         |   2 +-
 scripts/qmp/qom-set                          |   2 +-
 scripts/qmp/qom-tree                         |   2 +-
 scripts/render_block_graph.py                |   8 +-
 scripts/simplebench/bench_block_job.py       |   5 +-
 tests/qemu-iotests/iotests.py                |   2 +-
 tests/qemu-iotests/tests/mirror-top-perms    |  13 +-
 35 files changed, 490 insertions(+), 728 deletions(-)
 delete mode 100644 python/qemu/qmp/README.rst
 delete mode 100644 python/qemu/aqmp/__init__.py
 delete mode 100644 python/qemu/aqmp/legacy.py
 delete mode 100644 python/qemu/aqmp/py.typed
 rename python/qemu/{aqmp => qmp}/aqmp_tui.py (99%)
 rename python/qemu/{aqmp => qmp}/error.py (87%)
 rename python/qemu/{aqmp => qmp}/events.py (99%)
 create mode 100644 python/qemu/qmp/legacy.py
 rename python/qemu/{aqmp => qmp}/message.py (100%)
 rename python/qemu/{aqmp => qmp}/models.py (100%)
 rename python/qemu/{aqmp => qmp}/protocol.py (97%)
 rename python/qemu/{aqmp => qmp}/qmp_client.py (96%)
 rename python/qemu/{aqmp => qmp}/util.py (100%)
 rename python/qemu/{qmp => utils}/qemu_ga_client.py (94%)
 rename python/qemu/{qmp => utils}/qom.py (99%)
 rename python/qemu/{qmp => utils}/qom_common.py (98%)
 rename python/qemu/{qmp => utils}/qom_fuse.py (97%)

-- 
2.31.1

[PATCH v2 01/25] python/aqmp: add __del__ method to legacy interface

[John Snow]

asyncio can complain *very* loudly if you forget to back out of things
gracefully before the garbage collector starts destroying objects that
contain live references to asyncio Tasks.

The usual fix is just to remember to call aqmp.disconnect(), but for the
sake of the legacy wrapper and quick, one-off scripts where a graceful
shutdown is not necessarily of paramount imporance, add a courtesy
cleanup that will trigger prior to seeing screenfuls of confusing
asyncio tracebacks.

Note that we can't *always* save you from yourself; depending on when
the GC runs, you might just seriously be out of luck. The best we can do
in this case is to gently remind you to clean up after yourself.

(Still much better than multiple pages of incomprehensible python
warnings for the crime of forgetting to put your toys away.)

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

diff --git a/python/qemu/aqmp/legacy.py b/python/qemu/aqmp/legacy.py
index 9e7b9fb80b..2ccb136b02 100644
--- a/python/qemu/aqmp/legacy.py
+++ b/python/qemu/aqmp/legacy.py
@@ -16,6 +16,8 @@
 import qemu.qmp
 from qemu.qmp import QMPMessage, QMPReturnValue, SocketAddrT
 
+from .error import AQMPError
+from .protocol import Runstate
 from .qmp_client import QMPClient
 
 
@@ -136,3 +138,19 @@ def settimeout(self, timeout: Optional[float]) -> None:
 
     def send_fd_scm(self, fd: int) -> None:
         self._aqmp.send_fd_scm(fd)
+
+    def __del__(self) -> None:
+        if self._aqmp.runstate == Runstate.IDLE:
+            return
+
+        if not self._aloop.is_running():
+            self.close()
+        else:
+            # Garbage collection ran while the event loop was running.
+            # Nothing we can do about it now, but if we don't raise our
+            # own error, the user will be treated to a lot of traceback
+            # they might not understand.
+            raise AQMPError(
+                "QEMUMonitorProtocol.close()"
+                " was not called before object was garbage collected"
+            )
-- 
2.31.1

[PATCH v2 02/25] python/aqmp: handle asyncio.TimeoutError on execute()

[John Snow]

This exception can be injected into any await statement. If we are
canceled via timeout, we want to clear the pending execution record on
our way out.

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

diff --git a/python/qemu/aqmp/qmp_client.py b/python/qemu/aqmp/qmp_client.py
index 8105e29fa8..6a985ffe30 100644
--- a/python/qemu/aqmp/qmp_client.py
+++ b/python/qemu/aqmp/qmp_client.py
@@ -435,7 +435,11 @@ async def _issue(self, msg: Message) -> Union[None, str]:
             msg_id = msg['id']
 
         self._pending[msg_id] = asyncio.Queue(maxsize=1)
-        await self._outgoing.put(msg)
+        try:
+            await self._outgoing.put(msg)
+        except:
+            del self._pending[msg_id]
+            raise
 
         return msg_id
 
@@ -452,9 +456,9 @@ async def _reply(self, msg_id: Union[str, None]) -> Message:
             was lost, or some other problem.
         """
         queue = self._pending[msg_id]
-        result = await queue.get()
 
         try:
+            result = await queue.get()
             if isinstance(result, ExecInterruptedError):
                 raise result
             return result
-- 
2.31.1

[PATCH v2 03/25] python/aqmp: copy type definitions from qmp

[John Snow]

Copy the remaining type definitions from QMP into the qemu.aqmp.legacy
module. Now, most users don't need to import anything else but
qemu.aqmp.legacy.

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

diff --git a/python/qemu/aqmp/legacy.py b/python/qemu/aqmp/legacy.py
index 2ccb136b02..9431fe9330 100644
--- a/python/qemu/aqmp/legacy.py
+++ b/python/qemu/aqmp/legacy.py
@@ -6,7 +6,9 @@
 
 import asyncio
 from typing import (
+    Any,
     Awaitable,
+    Dict,
     List,
     Optional,
     TypeVar,
@@ -14,13 +16,29 @@
 )
 
 import qemu.qmp
-from qemu.qmp import QMPMessage, QMPReturnValue, SocketAddrT
 
 from .error import AQMPError
-from .protocol import Runstate
+from .protocol import Runstate, SocketAddrT
 from .qmp_client import QMPClient
 
 
+#: QMPMessage is an entire QMP message of any kind.
+QMPMessage = Dict[str, Any]
+
+#: QMPReturnValue is the 'return' value of a command.
+QMPReturnValue = object
+
+#: QMPObject is any object in a QMP message.
+QMPObject = Dict[str, object]
+
+# QMPMessage can be outgoing commands or incoming events/returns.
+# QMPReturnValue is usually a dict/json object, but due to QAPI's
+# 'returns-whitelist', it can actually be anything.
+#
+# {'return': {}} is a QMPMessage,
+# {} is the QMPReturnValue.
+
+
 # pylint: disable=missing-docstring
 
 
diff --git a/python/qemu/aqmp/protocol.py b/python/qemu/aqmp/protocol.py
index 5190b33b13..42a897e2fe 100644
--- a/python/qemu/aqmp/protocol.py
+++ b/python/qemu/aqmp/protocol.py
@@ -46,6 +46,10 @@
 _TaskFN = Callable[[], Awaitable[None]]  # aka ``async def func() -> None``
 _FutureT = TypeVar('_FutureT', bound=Optional['asyncio.Future[Any]'])
 
+InternetAddrT = Tuple[str, int]
+UnixAddrT = str
+SocketAddrT = Union[UnixAddrT, InternetAddrT]
+
 
 class Runstate(Enum):
     """Protocol session runstate."""
@@ -257,7 +261,7 @@ async def runstate_changed(self) -> Runstate:
 
     @upper_half
     @require(Runstate.IDLE)
-    async def accept(self, address: Union[str, Tuple[str, int]],
+    async def accept(self, address: SocketAddrT,
                      ssl: Optional[SSLContext] = None) -> None:
         """
         Accept a connection and begin processing message queues.
@@ -275,7 +279,7 @@ async def accept(self, address: Union[str, Tuple[str, int]],
 
     @upper_half
     @require(Runstate.IDLE)
-    async def connect(self, address: Union[str, Tuple[str, int]],
+    async def connect(self, address: SocketAddrT,
                       ssl: Optional[SSLContext] = None) -> None:
         """
         Connect to the server and begin processing message queues.
@@ -337,7 +341,7 @@ def _set_state(self, state: Runstate) -> None:
 
     @upper_half
     async def _new_session(self,
-                           address: Union[str, Tuple[str, int]],
+                           address: SocketAddrT,
                            ssl: Optional[SSLContext] = None,
                            accept: bool = False) -> None:
         """
@@ -397,7 +401,7 @@ async def _new_session(self,
     @upper_half
     async def _establish_connection(
             self,
-            address: Union[str, Tuple[str, int]],
+            address: SocketAddrT,
             ssl: Optional[SSLContext] = None,
             accept: bool = False
     ) -> None:
@@ -424,7 +428,7 @@ async def _establish_connection(
             await self._do_connect(address, ssl)
 
     @upper_half
-    async def _do_accept(self, address: Union[str, Tuple[str, int]],
+    async def _do_accept(self, address: SocketAddrT,
                          ssl: Optional[SSLContext] = None) -> None:
         """
         Acting as the transport server, accept a single connection.
@@ -482,7 +486,7 @@ async def _client_connected_cb(reader: asyncio.StreamReader,
         self.logger.debug("Connection accepted.")
 
     @upper_half
-    async def _do_connect(self, address: Union[str, Tuple[str, int]],
+    async def _do_connect(self, address: SocketAddrT,
                           ssl: Optional[SSLContext] = None) -> None:
         """
         Acting as the transport client, initiate a connection to a server.
-- 
2.31.1

[PATCH v2 04/25] python/aqmp: add SocketAddrT to package root

[John Snow]

It's a commonly needed definition, it can be re-exported by the root.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 python/qemu/aqmp/__init__.py | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/python/qemu/aqmp/__init__.py b/python/qemu/aqmp/__init__.py
index 880d5b6fa7..c6fa2dda58 100644
--- a/python/qemu/aqmp/__init__.py
+++ b/python/qemu/aqmp/__init__.py
@@ -26,7 +26,12 @@
 from .error import AQMPError
 from .events import EventListener
 from .message import Message
-from .protocol import ConnectError, Runstate, StateError
+from .protocol import (
+    ConnectError,
+    Runstate,
+    SocketAddrT,
+    StateError,
+)
 from .qmp_client import ExecInterruptedError, ExecuteError, QMPClient
 
 
@@ -48,4 +53,7 @@
     'ConnectError',
     'ExecuteError',
     'ExecInterruptedError',
+
+    # Type aliases
+    'SocketAddrT',
 )
-- 
2.31.1

[PATCH v2 05/25] python/aqmp: rename AQMPError to QMPError

[John Snow]

This is in preparation for renaming qemu.aqmp to qemu.qmp. I should have
done this from this from the very beginning, but it's a convenient time
to make sure this churn is taken care of.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 python/qemu/aqmp/__init__.py   |  6 +++---
 python/qemu/aqmp/error.py      | 12 ++++++------
 python/qemu/aqmp/events.py     |  4 ++--
 python/qemu/aqmp/legacy.py     |  4 ++--
 python/qemu/aqmp/protocol.py   |  8 ++++----
 python/qemu/aqmp/qmp_client.py |  8 ++++----
 6 files changed, 21 insertions(+), 21 deletions(-)

diff --git a/python/qemu/aqmp/__init__.py b/python/qemu/aqmp/__init__.py
index c6fa2dda58..e1efab00af 100644
--- a/python/qemu/aqmp/__init__.py
+++ b/python/qemu/aqmp/__init__.py
@@ -6,7 +6,7 @@
 QEMU Guest Agent, and the QEMU Storage Daemon.
 
 `QMPClient` provides the main functionality of this package. All errors
-raised by this library dervive from `AQMPError`, see `aqmp.error` for
+raised by this library dervive from `QMPError`, see `aqmp.error` for
 additional detail. See `aqmp.events` for an in-depth tutorial on
 managing QMP events.
 """
@@ -23,7 +23,7 @@
 
 import logging
 
-from .error import AQMPError
+from .error import QMPError
 from .events import EventListener
 from .message import Message
 from .protocol import (
@@ -48,7 +48,7 @@
     'Runstate',
 
     # Exceptions, most generic to most explicit
-    'AQMPError',
+    'QMPError',
     'StateError',
     'ConnectError',
     'ExecuteError',
diff --git a/python/qemu/aqmp/error.py b/python/qemu/aqmp/error.py
index 781f49b008..24ba4d5054 100644
--- a/python/qemu/aqmp/error.py
+++ b/python/qemu/aqmp/error.py
@@ -1,21 +1,21 @@
 """
-AQMP Error Classes
+QMP Error Classes
 
 This package seeks to provide semantic error classes that are intended
 to be used directly by clients when they would like to handle particular
 semantic failures (e.g. "failed to connect") without needing to know the
 enumeration of possible reasons for that failure.
 
-AQMPError serves as the ancestor for all exceptions raised by this
+QMPError serves as the ancestor for all exceptions raised by this
 package, and is suitable for use in handling semantic errors from this
 library. In most cases, individual public methods will attempt to catch
 and re-encapsulate various exceptions to provide a semantic
 error-handling interface.
 
-.. admonition:: AQMP Exception Hierarchy Reference
+.. admonition:: QMP Exception Hierarchy Reference
 
  |   `Exception`
- |    +-- `AQMPError`
+ |    +-- `QMPError`
  |         +-- `ConnectError`
  |         +-- `StateError`
  |         +-- `ExecInterruptedError`
@@ -31,11 +31,11 @@
 """
 
 
-class AQMPError(Exception):
+class QMPError(Exception):
     """Abstract error class for all errors originating from this package."""
 
 
-class ProtocolError(AQMPError):
+class ProtocolError(QMPError):
     """
     Abstract error class for protocol failures.
 
diff --git a/python/qemu/aqmp/events.py b/python/qemu/aqmp/events.py
index 5f7150c78d..f3d4e2b5e8 100644
--- a/python/qemu/aqmp/events.py
+++ b/python/qemu/aqmp/events.py
@@ -443,7 +443,7 @@ def accept(self, event) -> bool:
     Union,
 )
 
-from .error import AQMPError
+from .error import QMPError
 from .message import Message
 
 
@@ -451,7 +451,7 @@ def accept(self, event) -> bool:
 EventFilter = Callable[[Message], bool]
 
 
-class ListenerError(AQMPError):
+class ListenerError(QMPError):
     """
     Generic error class for `EventListener`-related problems.
     """
diff --git a/python/qemu/aqmp/legacy.py b/python/qemu/aqmp/legacy.py
index 9431fe9330..27df22818a 100644
--- a/python/qemu/aqmp/legacy.py
+++ b/python/qemu/aqmp/legacy.py
@@ -17,7 +17,7 @@
 
 import qemu.qmp
 
-from .error import AQMPError
+from .error import QMPError
 from .protocol import Runstate, SocketAddrT
 from .qmp_client import QMPClient
 
@@ -168,7 +168,7 @@ def __del__(self) -> None:
             # Nothing we can do about it now, but if we don't raise our
             # own error, the user will be treated to a lot of traceback
             # they might not understand.
-            raise AQMPError(
+            raise QMPError(
                 "QEMUMonitorProtocol.close()"
                 " was not called before object was garbage collected"
             )
diff --git a/python/qemu/aqmp/protocol.py b/python/qemu/aqmp/protocol.py
index 42a897e2fe..9ee6fe4ae2 100644
--- a/python/qemu/aqmp/protocol.py
+++ b/python/qemu/aqmp/protocol.py
@@ -29,7 +29,7 @@
     cast,
 )
 
-from .error import AQMPError
+from .error import QMPError
 from .util import (
     bottom_half,
     create_task,
@@ -65,7 +65,7 @@ class Runstate(Enum):
     DISCONNECTING = 3
 
 
-class ConnectError(AQMPError):
+class ConnectError(QMPError):
     """
     Raised when the initial connection process has failed.
 
@@ -90,7 +90,7 @@ def __str__(self) -> str:
         return f"{self.error_message}: {cause}"
 
 
-class StateError(AQMPError):
+class StateError(QMPError):
     """
     An API command (connect, execute, etc) was issued at an inappropriate time.
 
@@ -363,7 +363,7 @@ async def _new_session(self,
             This exception will wrap a more concrete one. In most cases,
             the wrapped exception will be `OSError` or `EOFError`. If a
             protocol-level failure occurs while establishing a new
-            session, the wrapped error may also be an `AQMPError`.
+            session, the wrapped error may also be an `QMPError`.
         """
         assert self.runstate == Runstate.IDLE
 
diff --git a/python/qemu/aqmp/qmp_client.py b/python/qemu/aqmp/qmp_client.py
index 6a985ffe30..f1a845cc82 100644
--- a/python/qemu/aqmp/qmp_client.py
+++ b/python/qemu/aqmp/qmp_client.py
@@ -20,7 +20,7 @@
     cast,
 )
 
-from .error import AQMPError, ProtocolError
+from .error import ProtocolError, QMPError
 from .events import Events
 from .message import Message
 from .models import ErrorResponse, Greeting
@@ -66,7 +66,7 @@ class NegotiationError(_WrappedProtocolError):
     """
 
 
-class ExecuteError(AQMPError):
+class ExecuteError(QMPError):
     """
     Exception raised by `QMPClient.execute()` on RPC failure.
 
@@ -87,7 +87,7 @@ def __init__(self, error_response: ErrorResponse,
         self.error_class: str = error_response.error.class_
 
 
-class ExecInterruptedError(AQMPError):
+class ExecInterruptedError(QMPError):
     """
     Exception raised by `execute()` (et al) when an RPC is interrupted.
 
@@ -641,7 +641,7 @@ def send_fd_scm(self, fd: int) -> None:
         sock = self._writer.transport.get_extra_info('socket')
 
         if sock.family != socket.AF_UNIX:
-            raise AQMPError("Sending file descriptors requires a UNIX socket.")
+            raise QMPError("Sending file descriptors requires a UNIX socket.")
 
         if not hasattr(sock, 'sendmsg'):
             # We need to void the warranty sticker.
-- 
2.31.1

[PATCH v2 06/25] python/qemu-ga-client: update instructions to newer CLI syntax

[John Snow]

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

diff --git a/python/qemu/qmp/qemu_ga_client.py b/python/qemu/qmp/qemu_ga_client.py
index 67ac0b4211..b3e1d98c9e 100644
--- a/python/qemu/qmp/qemu_ga_client.py
+++ b/python/qemu/qmp/qemu_ga_client.py
@@ -5,7 +5,7 @@
 
 Start QEMU with:
 
-# qemu [...] -chardev socket,path=/tmp/qga.sock,server,wait=off,id=qga0 \
+# qemu [...] -chardev socket,path=/tmp/qga.sock,server=on,wait=off,id=qga0 \
   -device virtio-serial \
   -device virtserialport,chardev=qga0,name=org.qemu.guest_agent.0
 
-- 
2.31.1

[PATCH v2 07/25] python/qmp: switch qemu-ga-client to AQMP

[John Snow]

Signed-off-by: John Snow <jsnow@redhat.com>
---
 python/qemu/qmp/qemu_ga_client.py | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/python/qemu/qmp/qemu_ga_client.py b/python/qemu/qmp/qemu_ga_client.py
index b3e1d98c9e..15ed430c61 100644
--- a/python/qemu/qmp/qemu_ga_client.py
+++ b/python/qemu/qmp/qemu_ga_client.py
@@ -37,8 +37,8 @@
 # the COPYING file in the top-level directory.
 
 import argparse
+import asyncio
 import base64
-import errno
 import os
 import random
 import sys
@@ -50,8 +50,8 @@
     Sequence,
 )
 
-from qemu import qmp
-from qemu.qmp import SocketAddrT
+from qemu.aqmp import ConnectError, SocketAddrT
+from qemu.aqmp.legacy import QEMUMonitorProtocol
 
 
 # This script has not seen many patches or careful attention in quite
@@ -61,7 +61,7 @@
 # pylint: disable=missing-docstring
 
 
-class QemuGuestAgent(qmp.QEMUMonitorProtocol):
+class QemuGuestAgent(QEMUMonitorProtocol):
     def __getattr__(self, name: str) -> Callable[..., Any]:
         def wrapper(**kwds: object) -> object:
             return self.command('guest-' + name.replace('_', '-'), **kwds)
@@ -149,7 +149,7 @@ def ping(self, timeout: Optional[float]) -> bool:
         self.qga.settimeout(timeout)
         try:
             self.qga.ping()
-        except TimeoutError:
+        except asyncio.TimeoutError:
             return False
         return True
 
@@ -172,7 +172,7 @@ def suspend(self, mode: str) -> None:
         try:
             getattr(self.qga, 'suspend' + '_' + mode)()
             # On error exception will raise
-        except TimeoutError:
+        except asyncio.TimeoutError:
             # On success command will timed out
             return
 
@@ -182,7 +182,7 @@ def shutdown(self, mode: str = 'powerdown') -> None:
 
         try:
             self.qga.shutdown(mode=mode)
-        except TimeoutError:
+        except asyncio.TimeoutError:
             pass
 
 
@@ -277,7 +277,7 @@ def _cmd_reboot(client: QemuGuestAgentClient, args: Sequence[str]) -> None:
 
 def send_command(address: str, cmd: str, args: Sequence[str]) -> None:
     if not os.path.exists(address):
-        print('%s not found' % address)
+        print(f"'{address}' not found. (Is QEMU running?)")
         sys.exit(1)
 
     if cmd not in commands:
@@ -287,10 +287,10 @@ def send_command(address: str, cmd: str, args: Sequence[str]) -> None:
 
     try:
         client = QemuGuestAgentClient(address)
-    except OSError as err:
+    except ConnectError as err:
         print(err)
-        if err.errno == errno.ECONNREFUSED:
-            print('Hint: qemu is not running?')
+        if isinstance(err.exc, ConnectionError):
+            print('(Is QEMU running?)')
         sys.exit(1)
 
     if cmd == 'fsfreeze' and args[0] == 'freeze':
-- 
2.31.1

[PATCH v2 08/25] python/qmp: switch qom tools to AQMP

[John Snow]

Signed-off-by: John Snow <jsnow@redhat.com>
---
 python/qemu/qmp/qom.py        |  5 +++--
 python/qemu/qmp/qom_common.py |  3 ++-
 python/qemu/qmp/qom_fuse.py   | 11 ++++++-----
 3 files changed, 11 insertions(+), 8 deletions(-)

diff --git a/python/qemu/qmp/qom.py b/python/qemu/qmp/qom.py
index 8ff28a8343..bb5d1a78f5 100644
--- a/python/qemu/qmp/qom.py
+++ b/python/qemu/qmp/qom.py
@@ -32,7 +32,8 @@
 
 import argparse
 
-from . import QMPResponseError
+from qemu.aqmp import ExecuteError
+
 from .qom_common import QOMCommand
 
 
@@ -233,7 +234,7 @@ def _list_node(self, path: str) -> None:
                 rsp = self.qmp.command('qom-get', path=path,
                                        property=item.name)
                 print(f"  {item.name}: {rsp} ({item.type})")
-            except QMPResponseError as err:
+            except ExecuteError as err:
                 print(f"  {item.name}: <EXCEPTION: {err!s}> ({item.type})")
         print('')
         for item in items:
diff --git a/python/qemu/qmp/qom_common.py b/python/qemu/qmp/qom_common.py
index a59ae1a2a1..6f07451dfa 100644
--- a/python/qemu/qmp/qom_common.py
+++ b/python/qemu/qmp/qom_common.py
@@ -27,7 +27,8 @@
     TypeVar,
 )
 
-from . import QEMUMonitorProtocol, QMPError
+from qemu.aqmp import QMPError
+from qemu.aqmp.legacy import QEMUMonitorProtocol
 
 
 # The following is needed only for a type alias.
diff --git a/python/qemu/qmp/qom_fuse.py b/python/qemu/qmp/qom_fuse.py
index 43f4671fdb..653a76b93b 100644
--- a/python/qemu/qmp/qom_fuse.py
+++ b/python/qemu/qmp/qom_fuse.py
@@ -48,7 +48,8 @@
 import fuse
 from fuse import FUSE, FuseOSError, Operations
 
-from . import QMPResponseError
+from qemu.aqmp import ExecuteError
+
 from .qom_common import QOMCommand
 
 
@@ -99,7 +100,7 @@ def is_object(self, path: str) -> bool:
         try:
             self.qom_list(path)
             return True
-        except QMPResponseError:
+        except ExecuteError:
             return False
 
     def is_property(self, path: str) -> bool:
@@ -112,7 +113,7 @@ def is_property(self, path: str) -> bool:
                 if item.name == prop:
                     return True
             return False
-        except QMPResponseError:
+        except ExecuteError:
             return False
 
     def is_link(self, path: str) -> bool:
@@ -125,7 +126,7 @@ def is_link(self, path: str) -> bool:
                 if item.name == prop and item.link:
                     return True
             return False
-        except QMPResponseError:
+        except ExecuteError:
             return False
 
     def read(self, path: str, size: int, offset: int, fh: IO[bytes]) -> bytes:
@@ -138,7 +139,7 @@ def read(self, path: str, size: int, offset: int, fh: IO[bytes]) -> bytes:
         try:
             data = str(self.qmp.command('qom-get', path=path, property=prop))
             data += '\n'  # make values shell friendly
-        except QMPResponseError as err:
+        except ExecuteError as err:
             raise FuseOSError(EPERM) from err
 
         if offset > len(data):
-- 
2.31.1

[PATCH v2 09/25] python/qmp: switch qmp-shell to AQMP

[John Snow]

We have a replacement for async QMP, but it doesn't have feature parity
yet. For now, then, port the old tool onto the new backend.

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

diff --git a/python/qemu/aqmp/legacy.py b/python/qemu/aqmp/legacy.py
index 27df22818a..0890f95b16 100644
--- a/python/qemu/aqmp/legacy.py
+++ b/python/qemu/aqmp/legacy.py
@@ -22,6 +22,9 @@
 from .qmp_client import QMPClient
 
 
+# (Temporarily) Re-export QMPBadPortError
+QMPBadPortError = qemu.qmp.QMPBadPortError
+
 #: QMPMessage is an entire QMP message of any kind.
 QMPMessage = Dict[str, Any]
 
diff --git a/python/qemu/qmp/qmp_shell.py b/python/qemu/qmp/qmp_shell.py
index e7d7eb18f1..d11bf54b00 100644
--- a/python/qemu/qmp/qmp_shell.py
+++ b/python/qemu/qmp/qmp_shell.py
@@ -95,8 +95,13 @@
     Sequence,
 )
 
-from qemu import qmp
-from qemu.qmp import QMPMessage
+from qemu.aqmp import ConnectError, QMPError, SocketAddrT
+from qemu.aqmp.legacy import (
+    QEMUMonitorProtocol,
+    QMPBadPortError,
+    QMPMessage,
+    QMPObject,
+)
 
 
 LOG = logging.getLogger(__name__)
@@ -125,7 +130,7 @@ def complete(self, text: str, state: int) -> Optional[str]:
         return None
 
 
-class QMPShellError(qmp.QMPError):
+class QMPShellError(QMPError):
     """
     QMP Shell Base error class.
     """
@@ -153,7 +158,7 @@ def visit_Name(cls,  # pylint: disable=invalid-name
         return node
 
 
-class QMPShell(qmp.QEMUMonitorProtocol):
+class QMPShell(QEMUMonitorProtocol):
     """
     QMPShell provides a basic readline-based QMP shell.
 
@@ -161,7 +166,7 @@ class QMPShell(qmp.QEMUMonitorProtocol):
     :param pretty: Pretty-print QMP messages.
     :param verbose: Echo outgoing QMP messages to console.
     """
-    def __init__(self, address: qmp.SocketAddrT,
+    def __init__(self, address: SocketAddrT,
                  pretty: bool = False, verbose: bool = False):
         super().__init__(address)
         self._greeting: Optional[QMPMessage] = None
@@ -237,7 +242,7 @@ def _parse_value(cls, val: str) -> object:
 
     def _cli_expr(self,
                   tokens: Sequence[str],
-                  parent: qmp.QMPObject) -> None:
+                  parent: QMPObject) -> None:
         for arg in tokens:
             (key, sep, val) = arg.partition('=')
             if sep != '=':
@@ -403,7 +408,7 @@ class HMPShell(QMPShell):
     :param pretty: Pretty-print QMP messages.
     :param verbose: Echo outgoing QMP messages to console.
     """
-    def __init__(self, address: qmp.SocketAddrT,
+    def __init__(self, address: SocketAddrT,
                  pretty: bool = False, verbose: bool = False):
         super().__init__(address, pretty, verbose)
         self._cpu_index = 0
@@ -512,19 +517,17 @@ def main() -> None:
 
     try:
         address = shell_class.parse_address(args.qmp_server)
-    except qmp.QMPBadPortError:
+    except QMPBadPortError:
         parser.error(f"Bad port number: {args.qmp_server}")
         return  # pycharm doesn't know error() is noreturn
 
     with shell_class(address, args.pretty, args.verbose) as qemu:
         try:
             qemu.connect(negotiate=not args.skip_negotiation)
-        except qmp.QMPConnectError:
-            die("Didn't get QMP greeting message")
-        except qmp.QMPCapabilitiesError:
-            die("Couldn't negotiate capabilities")
-        except OSError as err:
-            die(f"Couldn't connect to {args.qmp_server}: {err!s}")
+        except ConnectError as err:
+            if isinstance(err.exc, OSError):
+                die(f"Couldn't connect to {args.qmp_server}: {err!s}")
+            die(str(err))
 
         for _ in qemu.repl():
             pass
-- 
2.31.1

[PATCH v2 10/25] python: move qmp utilities to python/qemu/utils

[John Snow]

In order to upload a QMP package to PyPI, I want to remove any scripts
that I am not 100% confident I want to support upstream, beyond our
castle walls.

Move most of our QMP utilities into the utils package so we can split
them out from the PyPI upload.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 python/qemu/{qmp => utils}/qemu_ga_client.py |  0
 python/qemu/{qmp => utils}/qom.py            |  0
 python/qemu/{qmp => utils}/qom_common.py     |  0
 python/qemu/{qmp => utils}/qom_fuse.py       |  0
 python/setup.cfg                             | 16 ++++++++--------
 scripts/qmp/qemu-ga-client                   |  2 +-
 scripts/qmp/qom-fuse                         |  2 +-
 scripts/qmp/qom-get                          |  2 +-
 scripts/qmp/qom-list                         |  2 +-
 scripts/qmp/qom-set                          |  2 +-
 scripts/qmp/qom-tree                         |  2 +-
 11 files changed, 14 insertions(+), 14 deletions(-)
 rename python/qemu/{qmp => utils}/qemu_ga_client.py (100%)
 rename python/qemu/{qmp => utils}/qom.py (100%)
 rename python/qemu/{qmp => utils}/qom_common.py (100%)
 rename python/qemu/{qmp => utils}/qom_fuse.py (100%)

diff --git a/python/qemu/qmp/qemu_ga_client.py b/python/qemu/utils/qemu_ga_client.py
similarity index 100%
rename from python/qemu/qmp/qemu_ga_client.py
rename to python/qemu/utils/qemu_ga_client.py
diff --git a/python/qemu/qmp/qom.py b/python/qemu/utils/qom.py
similarity index 100%
rename from python/qemu/qmp/qom.py
rename to python/qemu/utils/qom.py
diff --git a/python/qemu/qmp/qom_common.py b/python/qemu/utils/qom_common.py
similarity index 100%
rename from python/qemu/qmp/qom_common.py
rename to python/qemu/utils/qom_common.py
diff --git a/python/qemu/qmp/qom_fuse.py b/python/qemu/utils/qom_fuse.py
similarity index 100%
rename from python/qemu/qmp/qom_fuse.py
rename to python/qemu/utils/qom_fuse.py
diff --git a/python/setup.cfg b/python/setup.cfg
index 417e937839..78421411d2 100644
--- a/python/setup.cfg
+++ b/python/setup.cfg
@@ -60,13 +60,13 @@ tui =
 
 [options.entry_points]
 console_scripts =
-    qom = qemu.qmp.qom:main
-    qom-set = qemu.qmp.qom:QOMSet.entry_point
-    qom-get = qemu.qmp.qom:QOMGet.entry_point
-    qom-list = qemu.qmp.qom:QOMList.entry_point
-    qom-tree = qemu.qmp.qom:QOMTree.entry_point
-    qom-fuse = qemu.qmp.qom_fuse:QOMFuse.entry_point [fuse]
-    qemu-ga-client = qemu.qmp.qemu_ga_client:main
+    qom = qemu.utils.qom:main
+    qom-set = qemu.utils.qom:QOMSet.entry_point
+    qom-get = qemu.utils.qom:QOMGet.entry_point
+    qom-list = qemu.utils.qom:QOMList.entry_point
+    qom-tree = qemu.utils.qom:QOMTree.entry_point
+    qom-fuse = qemu.utils.qom_fuse:QOMFuse.entry_point [fuse]
+    qemu-ga-client = qemu.utils.qemu_ga_client:main
     qmp-shell = qemu.qmp.qmp_shell:main
     aqmp-tui = qemu.aqmp.aqmp_tui:main [tui]
 
@@ -80,7 +80,7 @@ python_version = 3.6
 warn_unused_configs = True
 namespace_packages = True
 
-[mypy-qemu.qmp.qom_fuse]
+[mypy-qemu.utils.qom_fuse]
 # fusepy has no type stubs:
 allow_subclassing_any = True
 
diff --git a/scripts/qmp/qemu-ga-client b/scripts/qmp/qemu-ga-client
index 102fd2cad9..56edd0234a 100755
--- a/scripts/qmp/qemu-ga-client
+++ b/scripts/qmp/qemu-ga-client
@@ -4,7 +4,7 @@ import os
 import sys
 
 sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..', 'python'))
-from qemu.qmp import qemu_ga_client
+from qemu.utils import qemu_ga_client
 
 
 if __name__ == '__main__':
diff --git a/scripts/qmp/qom-fuse b/scripts/qmp/qom-fuse
index a58c8ef979..d453807b27 100755
--- a/scripts/qmp/qom-fuse
+++ b/scripts/qmp/qom-fuse
@@ -4,7 +4,7 @@ import os
 import sys
 
 sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..', 'python'))
-from qemu.qmp.qom_fuse import QOMFuse
+from qemu.utils.qom_fuse import QOMFuse
 
 
 if __name__ == '__main__':
diff --git a/scripts/qmp/qom-get b/scripts/qmp/qom-get
index e4f3e0c013..04ebe052e8 100755
--- a/scripts/qmp/qom-get
+++ b/scripts/qmp/qom-get
@@ -4,7 +4,7 @@ import os
 import sys
 
 sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..', 'python'))
-from qemu.qmp.qom import QOMGet
+from qemu.utils.qom import QOMGet
 
 
 if __name__ == '__main__':
diff --git a/scripts/qmp/qom-list b/scripts/qmp/qom-list
index 7a071a54e1..853b85a8d3 100755
--- a/scripts/qmp/qom-list
+++ b/scripts/qmp/qom-list
@@ -4,7 +4,7 @@ import os
 import sys
 
 sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..', 'python'))
-from qemu.qmp.qom import QOMList
+from qemu.utils.qom import QOMList
 
 
 if __name__ == '__main__':
diff --git a/scripts/qmp/qom-set b/scripts/qmp/qom-set
index 9ca9e2ba10..06820feec4 100755
--- a/scripts/qmp/qom-set
+++ b/scripts/qmp/qom-set
@@ -4,7 +4,7 @@ import os
 import sys
 
 sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..', 'python'))
-from qemu.qmp.qom import QOMSet
+from qemu.utils.qom import QOMSet
 
 
 if __name__ == '__main__':
diff --git a/scripts/qmp/qom-tree b/scripts/qmp/qom-tree
index 7d0ccca3a4..760e172277 100755
--- a/scripts/qmp/qom-tree
+++ b/scripts/qmp/qom-tree
@@ -4,7 +4,7 @@ import os
 import sys
 
 sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..', 'python'))
-from qemu.qmp.qom import QOMTree
+from qemu.utils.qom import QOMTree
 
 
 if __name__ == '__main__':
-- 
2.31.1

[PATCH v2 11/25] python: move qmp-shell under the AQMP package

[John Snow]

Signed-off-by: John Snow <jsnow@redhat.com>
---
 python/README.rst                      | 2 +-
 python/qemu/{qmp => aqmp}/qmp_shell.py | 0
 python/setup.cfg                       | 2 +-
 scripts/qmp/qmp-shell                  | 2 +-
 4 files changed, 3 insertions(+), 3 deletions(-)
 rename python/qemu/{qmp => aqmp}/qmp_shell.py (100%)

diff --git a/python/README.rst b/python/README.rst
index 9c1fceaee7..fcf74f69ea 100644
--- a/python/README.rst
+++ b/python/README.rst
@@ -59,7 +59,7 @@ Package installation also normally provides executable console scripts,
 so that tools like ``qmp-shell`` are always available via $PATH. To
 invoke them without installation, you can invoke e.g.:
 
-``> PYTHONPATH=~/src/qemu/python python3 -m qemu.qmp.qmp_shell``
+``> PYTHONPATH=~/src/qemu/python python3 -m qemu.aqmp.qmp_shell``
 
 The mappings between console script name and python module path can be
 found in ``setup.cfg``.
diff --git a/python/qemu/qmp/qmp_shell.py b/python/qemu/aqmp/qmp_shell.py
similarity index 100%
rename from python/qemu/qmp/qmp_shell.py
rename to python/qemu/aqmp/qmp_shell.py
diff --git a/python/setup.cfg b/python/setup.cfg
index 78421411d2..168a79c867 100644
--- a/python/setup.cfg
+++ b/python/setup.cfg
@@ -67,7 +67,7 @@ console_scripts =
     qom-tree = qemu.utils.qom:QOMTree.entry_point
     qom-fuse = qemu.utils.qom_fuse:QOMFuse.entry_point [fuse]
     qemu-ga-client = qemu.utils.qemu_ga_client:main
-    qmp-shell = qemu.qmp.qmp_shell:main
+    qmp-shell = qemu.aqmp.qmp_shell:main
     aqmp-tui = qemu.aqmp.aqmp_tui:main [tui]
 
 [flake8]
diff --git a/scripts/qmp/qmp-shell b/scripts/qmp/qmp-shell
index 4a20f97db7..31b19d73e2 100755
--- a/scripts/qmp/qmp-shell
+++ b/scripts/qmp/qmp-shell
@@ -4,7 +4,7 @@ import os
 import sys
 
 sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..', 'python'))
-from qemu.qmp import qmp_shell
+from qemu.aqmp import qmp_shell
 
 
 if __name__ == '__main__':
-- 
2.31.1

[PATCH v2 12/25] python/machine: permanently switch to AQMP

[John Snow]

Remove the QEMU_PYTHON_LEGACY_QMP environment variable, making the
switch permanent. Update Exceptions and import paths as necessary.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 python/qemu/machine/machine.py | 18 +++++++-----------
 python/qemu/machine/qtest.py   |  2 +-
 2 files changed, 8 insertions(+), 12 deletions(-)

diff --git a/python/qemu/machine/machine.py b/python/qemu/machine/machine.py
index 67ab06ca2b..21fb4a4f30 100644
--- a/python/qemu/machine/machine.py
+++ b/python/qemu/machine/machine.py
@@ -40,21 +40,16 @@
     TypeVar,
 )
 
-from qemu.qmp import (  # pylint: disable=import-error
+from qemu.aqmp import SocketAddrT
+from qemu.aqmp.legacy import (
+    QEMUMonitorProtocol,
     QMPMessage,
     QMPReturnValue,
-    SocketAddrT,
 )
 
 from . import console_socket
 
 
-if os.environ.get('QEMU_PYTHON_LEGACY_QMP'):
-    from qemu.qmp import QEMUMonitorProtocol
-else:
-    from qemu.aqmp.legacy import QEMUMonitorProtocol
-
-
 LOG = logging.getLogger(__name__)
 
 
@@ -710,8 +705,9 @@ def events_wait(self,
         :param timeout: Optional timeout, in seconds.
                         See QEMUMonitorProtocol.pull_event.
 
-        :raise QMPTimeoutError: If timeout was non-zero and no matching events
-                                were found.
+        :raise asyncio.TimeoutError:
+            If timeout was non-zero and no matching events were found.
+
         :return: A QMP event matching the filter criteria.
                  If timeout was 0 and no event matched, None.
         """
@@ -734,7 +730,7 @@ def _match(event: QMPMessage) -> bool:
             event = self._qmp.pull_event(wait=timeout)
             if event is None:
                 # NB: None is only returned when timeout is false-ish.
-                # Timeouts raise QMPTimeoutError instead!
+                # Timeouts raise asyncio.TimeoutError instead!
                 break
             if _match(event):
                 return event
diff --git a/python/qemu/machine/qtest.py b/python/qemu/machine/qtest.py
index f2f9aaa5e5..817c8a5425 100644
--- a/python/qemu/machine/qtest.py
+++ b/python/qemu/machine/qtest.py
@@ -26,7 +26,7 @@
     TextIO,
 )
 
-from qemu.qmp import SocketAddrT  # pylint: disable=import-error
+from qemu.aqmp.protocol import SocketAddrT
 
 from .machine import QEMUMachine
 
-- 
2.31.1

[PATCH v2 13/25] scripts/cpu-x86-uarch-abi: fix CLI parsing

[John Snow]

Signed-off-by: John Snow <jsnow@redhat.com>
---
 scripts/cpu-x86-uarch-abi.py | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/scripts/cpu-x86-uarch-abi.py b/scripts/cpu-x86-uarch-abi.py
index 08acc52a81..8963d90f0b 100644
--- a/scripts/cpu-x86-uarch-abi.py
+++ b/scripts/cpu-x86-uarch-abi.py
@@ -9,7 +9,7 @@
 from qemu import qmp
 import sys
 
-if len(sys.argv) != 1:
+if len(sys.argv) != 2:
     print("syntax: %s QMP-SOCK\n\n" % __file__ +
           "Where QMP-SOCK points to a QEMU process such as\n\n" +
           " # qemu-system-x86_64 -qmp unix:/tmp/qmp,server,nowait " +
@@ -66,7 +66,6 @@
 
 
 sock = sys.argv[1]
-cmd = sys.argv[2]
 shell = qmp.QEMUMonitorProtocol(sock)
 shell.connect()
 
-- 
2.31.1

[PATCH v2 14/25] scripts/cpu-x86-uarch-abi: switch to AQMP

[John Snow]

Signed-off-by: John Snow <jsnow@redhat.com>
---
 scripts/cpu-x86-uarch-abi.py | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/scripts/cpu-x86-uarch-abi.py b/scripts/cpu-x86-uarch-abi.py
index 8963d90f0b..c262d2f027 100644
--- a/scripts/cpu-x86-uarch-abi.py
+++ b/scripts/cpu-x86-uarch-abi.py
@@ -6,7 +6,7 @@
 # compatibility levels for each CPU model.
 #
 
-from qemu import qmp
+from qemu.aqmp.legacy import QEMUMonitorProtocol
 import sys
 
 if len(sys.argv) != 2:
@@ -66,7 +66,7 @@
 
 
 sock = sys.argv[1]
-shell = qmp.QEMUMonitorProtocol(sock)
+shell = QEMUMonitorProtocol(sock)
 shell.connect()
 
 models = shell.cmd("query-cpu-definitions")
-- 
2.31.1

[PATCH v2 15/25] scripts/render-block-graph: switch to AQMP

[John Snow]

Creating an instance of qemu.aqmp.ExecuteError is too involved here, so
just drop the specificity down to a generic AQMPError.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 scripts/render_block_graph.py | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/scripts/render_block_graph.py b/scripts/render_block_graph.py
index da6acf050d..97778927f3 100755
--- a/scripts/render_block_graph.py
+++ b/scripts/render_block_graph.py
@@ -25,10 +25,8 @@
 from graphviz import Digraph
 
 sys.path.append(os.path.join(os.path.dirname(__file__), '..', 'python'))
-from qemu.qmp import (
-    QEMUMonitorProtocol,
-    QMPResponseError,
-)
+from qemu.aqmp import QMPError
+from qemu.aqmp.legacy import QEMUMonitorProtocol
 
 
 def perm(arr):
@@ -105,7 +103,7 @@ def command(self, cmd):
         reply = json.loads(subprocess.check_output(ar))
 
         if 'error' in reply:
-            raise QMPResponseError(reply)
+            raise QMPError(reply)
 
         return reply['return']
 
-- 
2.31.1

[PATCH v2 16/25] scripts/bench-block-job: switch to AQMP

[John Snow]

For this commit, we only need to remove accommodations for the
synchronous QMP library.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 scripts/simplebench/bench_block_job.py | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/scripts/simplebench/bench_block_job.py b/scripts/simplebench/bench_block_job.py
index a403c35b08..af9d1646a4 100755
--- a/scripts/simplebench/bench_block_job.py
+++ b/scripts/simplebench/bench_block_job.py
@@ -27,7 +27,6 @@
 
 sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..', 'python'))
 from qemu.machine import QEMUMachine
-from qemu.qmp import QMPConnectError
 from qemu.aqmp import ConnectError
 
 
@@ -50,7 +49,7 @@ def bench_block_job(cmd, cmd_args, qemu_args):
         vm.launch()
     except OSError as e:
         return {'error': 'popen failed: ' + str(e)}
-    except (QMPConnectError, ConnectError, socket.timeout):
+    except (ConnectError, socket.timeout):
         return {'error': 'qemu failed: ' + str(vm.get_log())}
 
     try:
-- 
2.31.1

[PATCH v2 17/25] iotests/mirror-top-perms: switch to AQMP

[John Snow]

Signed-off-by: John Snow <jsnow@redhat.com>

---

Note: I still need to adjust the logging. The problem now is that the
logging messages include the PID of the test process, so they need to be
filtered out. I'll investigate that for a follow-up, or for v2.

I could just add yet another filtering function somewhere, but I think
it's getting out of hand with how many filters and loggers there are, so
I want to give it a slightly more serious treatment instead of a
hackjob.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 tests/qemu-iotests/tests/mirror-top-perms | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/tests/qemu-iotests/tests/mirror-top-perms b/tests/qemu-iotests/tests/mirror-top-perms
index 0a51a613f3..f394931a00 100755
--- a/tests/qemu-iotests/tests/mirror-top-perms
+++ b/tests/qemu-iotests/tests/mirror-top-perms
@@ -23,7 +23,6 @@ import os
 
 from qemu.aqmp import ConnectError
 from qemu.machine import machine
-from qemu.qmp import QMPConnectError
 
 import iotests
 from iotests import change_log_level, qemu_img
@@ -101,13 +100,13 @@ class TestMirrorTopPerms(iotests.QMPTestCase):
         self.vm_b.add_device('virtio-blk,drive=drive0,share-rw=on')
         try:
             # Silence AQMP errors temporarily.
-            # TODO: Remove this and just allow the errors to be logged when
-            # AQMP fully replaces QMP.
+            # TODO: Remove change_log_level and allow the errors to be logged.
+            #       This necessitates a PID filter on *all* logging output.
             with change_log_level('qemu.aqmp'):
                 self.vm_b.launch()
                 print('ERROR: VM B launched successfully, '
                       'this should not have happened')
-        except (QMPConnectError, ConnectError):
+        except ConnectError:
             assert 'Is another process using the image' in self.vm_b.get_log()
 
         result = self.vm.qmp('block-job-cancel',
-- 
2.31.1

[PATCH v2 18/25] iotests: switch to AQMP

[John Snow]

Simply import the type defition from the new location.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 tests/qemu-iotests/iotests.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 83bfedb902..cb21aebe36 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -37,7 +37,7 @@
 from contextlib import contextmanager
 
 from qemu.machine import qtest
-from qemu.qmp import QMPMessage
+from qemu.aqmp.legacy import QMPMessage
 
 # Use this logger for logging messages directly from the iotests module
 logger = logging.getLogger('qemu.iotests')
-- 
2.31.1

[PATCH v2 19/25] python: temporarily silence pylint duplicate-code warnings

[John Snow]

The next several commits copy some code from qemu.qmp to qemu.aqmp, then
delete qemu.qmp. In the interim, to prevent test failures, the duplicate
code detection needs to be silenced to prevent bisect problems with CI
testing.

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

diff --git a/python/setup.cfg b/python/setup.cfg
index 168a79c867..510df23698 100644
--- a/python/setup.cfg
+++ b/python/setup.cfg
@@ -115,6 +115,7 @@ ignore_missing_imports = True
 disable=consider-using-f-string,
         too-many-function-args,  # mypy handles this with less false positives.
         no-member,  # mypy also handles this better.
+        duplicate-code,  # To be removed by the end of this patch series.
 
 [pylint.basic]
 # Good variable names which should always be accepted, separated by a comma.
-- 
2.31.1

[PATCH v2 20/25] python/aqmp: take QMPBadPortError and parse_address from qemu.qmp

[John Snow]

Shift these definitions over from the qmp package to the async qmp
package.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 python/qemu/aqmp/aqmp_tui.py |  2 +-
 python/qemu/aqmp/legacy.py   | 30 ++++++++++++++++++++++++++----
 python/qemu/qmp/__init__.py  | 26 --------------------------
 3 files changed, 27 insertions(+), 31 deletions(-)

diff --git a/python/qemu/aqmp/aqmp_tui.py b/python/qemu/aqmp/aqmp_tui.py
index a2929f771c..184a3e4690 100644
--- a/python/qemu/aqmp/aqmp_tui.py
+++ b/python/qemu/aqmp/aqmp_tui.py
@@ -35,8 +35,8 @@
 import urwid
 import urwid_readline
 
-from ..qmp import QEMUMonitorProtocol, QMPBadPortError
 from .error import ProtocolError
+from .legacy import QEMUMonitorProtocol, QMPBadPortError
 from .message import DeserializationError, Message, UnexpectedTypeError
 from .protocol import ConnectError, Runstate
 from .qmp_client import ExecInterruptedError, QMPClient
diff --git a/python/qemu/aqmp/legacy.py b/python/qemu/aqmp/legacy.py
index 0890f95b16..76b09671cc 100644
--- a/python/qemu/aqmp/legacy.py
+++ b/python/qemu/aqmp/legacy.py
@@ -22,9 +22,6 @@
 from .qmp_client import QMPClient
 
 
-# (Temporarily) Re-export QMPBadPortError
-QMPBadPortError = qemu.qmp.QMPBadPortError
-
 #: QMPMessage is an entire QMP message of any kind.
 QMPMessage = Dict[str, Any]
 
@@ -45,6 +42,12 @@
 # pylint: disable=missing-docstring
 
 
+class QMPBadPortError(QMPError):
+    """
+    Unable to parse socket address: Port was non-numerical.
+    """
+
+
 class QEMUMonitorProtocol(qemu.qmp.QEMUMonitorProtocol):
     def __init__(self, address: SocketAddrT,
                  server: bool = False,
@@ -72,7 +75,26 @@ def _get_greeting(self) -> Optional[QMPMessage]:
         return None
 
     # __enter__ and __exit__ need no changes
-    # parse_address needs no changes
+
+    @classmethod
+    def parse_address(cls, address: str) -> SocketAddrT:
+        """
+        Parse a string into a QMP address.
+
+        Figure out if the argument is in the port:host form.
+        If it's not, it's probably a file path.
+        """
+        components = address.split(':')
+        if len(components) == 2:
+            try:
+                port = int(components[1])
+            except ValueError:
+                msg = f"Bad port: '{components[1]}' in '{address}'."
+                raise QMPBadPortError(msg) from None
+            return (components[0], port)
+
+        # Treat as filepath.
+        return address
 
     def connect(self, negotiate: bool = True) -> Optional[QMPMessage]:
         self._aqmp.await_greeting = negotiate
diff --git a/python/qemu/qmp/__init__.py b/python/qemu/qmp/__init__.py
index 358c0971d0..4e08641154 100644
--- a/python/qemu/qmp/__init__.py
+++ b/python/qemu/qmp/__init__.py
@@ -102,12 +102,6 @@ def __init__(self, reply: QMPMessage):
         self.reply = reply
 
 
-class QMPBadPortError(QMPError):
-    """
-    Unable to parse socket address: Port was non-numerical.
-    """
-
-
 class QEMUMonitorProtocol:
     """
     Provide an API to connect to QEMU via QEMU Monitor Protocol (QMP) and then
@@ -237,26 +231,6 @@ def __exit__(self,
         # Implement context manager exit function.
         self.close()
 
-    @classmethod
-    def parse_address(cls, address: str) -> SocketAddrT:
-        """
-        Parse a string into a QMP address.
-
-        Figure out if the argument is in the port:host form.
-        If it's not, it's probably a file path.
-        """
-        components = address.split(':')
-        if len(components) == 2:
-            try:
-                port = int(components[1])
-            except ValueError:
-                msg = f"Bad port: '{components[1]}' in '{address}'."
-                raise QMPBadPortError(msg) from None
-            return (components[0], port)
-
-        # Treat as filepath.
-        return address
-
     def connect(self, negotiate: bool = True) -> Optional[QMPMessage]:
         """
         Connect to the QMP Monitor and perform capabilities negotiation.
-- 
2.31.1

[PATCH v2 21/25] python/aqmp: fully separate from qmp.QEMUMonitorProtocol

[John Snow]

After this patch, qemu.aqmp.legacy.QEMUMonitorProtocol no longer
inherits from qemu.qmp.QEMUMonitorProtocol. To do this, several
inherited methods need to be explicitly re-defined.

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

diff --git a/python/qemu/aqmp/legacy.py b/python/qemu/aqmp/legacy.py
index 76b09671cc..8f38e7d912 100644
--- a/python/qemu/aqmp/legacy.py
+++ b/python/qemu/aqmp/legacy.py
@@ -5,18 +5,18 @@
 """
 
 import asyncio
+from types import TracebackType
 from typing import (
     Any,
     Awaitable,
     Dict,
     List,
     Optional,
+    Type,
     TypeVar,
     Union,
 )
 
-import qemu.qmp
-
 from .error import QMPError
 from .protocol import Runstate, SocketAddrT
 from .qmp_client import QMPClient
@@ -48,9 +48,9 @@ class QMPBadPortError(QMPError):
     """
 
 
-class QEMUMonitorProtocol(qemu.qmp.QEMUMonitorProtocol):
+class QEMUMonitorProtocol:
     def __init__(self, address: SocketAddrT,
-                 server: bool = False,
+                 server: bool = False,  # pylint: disable=unused-argument
                  nickname: Optional[str] = None):
 
         # pylint: disable=super-init-not-called
@@ -74,7 +74,18 @@ def _get_greeting(self) -> Optional[QMPMessage]:
             return self._aqmp.greeting._asdict()
         return None
 
-    # __enter__ and __exit__ need no changes
+    def __enter__(self: _T) -> _T:
+        # Implement context manager enter function.
+        return self
+
+    def __exit__(self,
+                 # pylint: disable=duplicate-code
+                 # see https://github.com/PyCQA/pylint/issues/3619
+                 exc_type: Optional[Type[BaseException]],
+                 exc_val: Optional[BaseException],
+                 exc_tb: Optional[TracebackType]) -> None:
+        # Implement context manager exit function.
+        self.close()
 
     @classmethod
     def parse_address(cls, address: str) -> SocketAddrT:
@@ -131,7 +142,22 @@ def cmd_obj(self, qmp_cmd: QMPMessage) -> QMPMessage:
             )
         )
 
-    # Default impl of cmd() delegates to cmd_obj
+    def cmd(self, name: str,
+            args: Optional[Dict[str, object]] = None,
+            cmd_id: Optional[object] = None) -> QMPMessage:
+        """
+        Build a QMP command and send it to the QMP Monitor.
+
+        @param name: command name (string)
+        @param args: command arguments (dict)
+        @param cmd_id: command id (dict, list, string or int)
+        """
+        qmp_cmd: QMPMessage = {'execute': name}
+        if args:
+            qmp_cmd['arguments'] = args
+        if cmd_id:
+            qmp_cmd['id'] = cmd_id
+        return self.cmd_obj(qmp_cmd)
 
     def command(self, cmd: str, **kwds: object) -> QMPReturnValue:
         return self._sync(
-- 
2.31.1

[PATCH v2 22/25] python/aqmp: copy qmp docstrings to qemu.aqmp.legacy

[John Snow]

Copy the docstrings out of qemu.qmp, adjusting them as necessary to
more accurately reflect the current state of this class.

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

diff --git a/python/qemu/aqmp/legacy.py b/python/qemu/aqmp/legacy.py
index 8f38e7d912..6c250cd46a 100644
--- a/python/qemu/aqmp/legacy.py
+++ b/python/qemu/aqmp/legacy.py
@@ -1,9 +1,23 @@
 """
-Sync QMP Wrapper
+(Legacy) Sync QMP Wrapper
 
-This class pretends to be qemu.qmp.QEMUMonitorProtocol.
+This module provides the `QEMUMonitorProtocol` class, which is a
+synchronous wrapper around `QMPClient`.
+
+Its design closely resembles that of the original QEMUMonitorProtocol
+class, originally written by Luiz Capitulino.
 """
 
+# Copyright (C) 2009, 2010, 2021 Red Hat Inc.
+#
+# Authors:
+#  Luiz Capitulino <lcapitulino@redhat.com>
+#  John Snow <jsnow@redhat.com>
+#
+# This work is licensed under the terms of the GNU GPL, version 2.
+# See the COPYING file in the top-level directory.
+
+
 import asyncio
 from types import TracebackType
 from typing import (
@@ -39,9 +53,6 @@
 # {} is the QMPReturnValue.
 
 
-# pylint: disable=missing-docstring
-
-
 class QMPBadPortError(QMPError):
     """
     Unable to parse socket address: Port was non-numerical.
@@ -49,6 +60,21 @@ class QMPBadPortError(QMPError):
 
 
 class QEMUMonitorProtocol:
+    """
+    Provide an API to connect to QEMU via QEMU Monitor Protocol (QMP)
+    and then allow to handle commands and events.
+
+    :param address:  QEMU address, can be either a unix socket path (string)
+                     or a tuple in the form ( address, port ) for a TCP
+                     connection
+    :param server:   Deprecated, ignored. (See 'accept')
+    :param nickname: Optional nickname used for logging.
+
+    ..note::
+        No connection is established during `__init__`, this is done by
+        the `connect()` or `accept()` methods.
+    """
+
     def __init__(self, address: SocketAddrT,
                  server: bool = False,  # pylint: disable=unused-argument
                  nickname: Optional[str] = None):
@@ -108,6 +134,12 @@ def parse_address(cls, address: str) -> SocketAddrT:
         return address
 
     def connect(self, negotiate: bool = True) -> Optional[QMPMessage]:
+        """
+        Connect to the QMP Monitor and perform capabilities negotiation.
+
+        :return: QMP greeting dict, or None if negotiate is false
+        :raise ConnectError: on connection errors
+        """
         self._aqmp.await_greeting = negotiate
         self._aqmp.negotiate = negotiate
 
@@ -117,6 +149,16 @@ def connect(self, negotiate: bool = True) -> Optional[QMPMessage]:
         return self._get_greeting()
 
     def accept(self, timeout: Optional[float] = 15.0) -> QMPMessage:
+        """
+        Await connection from QMP Monitor and perform capabilities negotiation.
+
+        :param timeout:
+            timeout in seconds (nonnegative float number, or None).
+            If None, there is no timeout, and this may block forever.
+
+        :return: QMP greeting dict
+        :raise ConnectError: on connection errors
+        """
         self._aqmp.await_greeting = True
         self._aqmp.negotiate = True
 
@@ -130,6 +172,12 @@ def accept(self, timeout: Optional[float] = 15.0) -> QMPMessage:
         return ret
 
     def cmd_obj(self, qmp_cmd: QMPMessage) -> QMPMessage:
+        """
+        Send a QMP command to the QMP Monitor.
+
+        :param qmp_cmd: QMP command to be sent as a Python dict
+        :return: QMP response as a Python dict
+        """
         return dict(
             self._sync(
                 # pylint: disable=protected-access
@@ -148,9 +196,9 @@ def cmd(self, name: str,
         """
         Build a QMP command and send it to the QMP Monitor.
 
-        @param name: command name (string)
-        @param args: command arguments (dict)
-        @param cmd_id: command id (dict, list, string or int)
+        :param name: command name (string)
+        :param args: command arguments (dict)
+        :param cmd_id: command id (dict, list, string or int)
         """
         qmp_cmd: QMPMessage = {'execute': name}
         if args:
@@ -160,6 +208,9 @@ def cmd(self, name: str,
         return self.cmd_obj(qmp_cmd)
 
     def command(self, cmd: str, **kwds: object) -> QMPReturnValue:
+        """
+        Build and send a QMP command to the monitor, report errors if any
+        """
         return self._sync(
             self._aqmp.execute(cmd, kwds),
             self._timeout
@@ -167,6 +218,19 @@ def command(self, cmd: str, **kwds: object) -> QMPReturnValue:
 
     def pull_event(self,
                    wait: Union[bool, float] = False) -> Optional[QMPMessage]:
+        """
+        Pulls a single event.
+
+        :param wait:
+            If False or 0, do not wait. Return None if no events ready.
+            If True, wait forever until the next event.
+            Otherwise, wait for the specified number of seconds.
+
+        :raise asyncio.TimeoutError:
+            When a timeout is requested and the timeout period elapses.
+
+        :return: The first available QMP event, or None.
+        """
         if not wait:
             # wait is False/0: "do not wait, do not except."
             if self._aqmp.events.empty():
@@ -187,6 +251,20 @@ def pull_event(self,
         )
 
     def get_events(self, wait: Union[bool, float] = False) -> List[QMPMessage]:
+        """
+        Get a list of QMP events and clear all pending events.
+
+        :param wait:
+            If False or 0, do not wait. Return None if no events ready.
+            If True, wait until we have at least one event.
+            Otherwise, wait for up to the specified number of seconds for at
+            least one event.
+
+        :raise asyncio.TimeoutError:
+            When a timeout is requested and the timeout period elapses.
+
+        :return: A list of QMP events.
+        """
         events = [dict(x) for x in self._aqmp.events.clear()]
         if events:
             return events
@@ -195,17 +273,33 @@ def get_events(self, wait: Union[bool, float] = False) -> List[QMPMessage]:
         return [event] if event is not None else []
 
     def clear_events(self) -> None:
+        """Clear current list of pending events."""
         self._aqmp.events.clear()
 
     def close(self) -> None:
+        """Close the connection."""
         self._sync(
             self._aqmp.disconnect()
         )
 
     def settimeout(self, timeout: Optional[float]) -> None:
+        """
+        Set the timeout for QMP RPC execution.
+
+        This timeout affects the `cmd`, `cmd_obj`, and `command` methods.
+        The `accept`, `pull_event` and `get_event` methods have their
+        own configurable timeouts.
+
+        :param timeout:
+            timeout in seconds, or None.
+            None will wait indefinitely.
+        """
         self._timeout = timeout
 
     def send_fd_scm(self, fd: int) -> None:
+        """
+        Send a file descriptor to the remote via SCM_RIGHTS.
+        """
         self._aqmp.send_fd_scm(fd)
 
     def __del__(self) -> None:
-- 
2.31.1

[PATCH v2 23/25] python: remove the old QMP package

[John Snow]

Thank you for your service!

Signed-off-by: John Snow <jsnow@redhat.com>
---
 python/PACKAGE.rst          |   4 +-
 python/README.rst           |   2 +-
 python/qemu/qmp/README.rst  |   9 -
 python/qemu/qmp/__init__.py | 396 ------------------------------------
 python/qemu/qmp/py.typed    |   0
 python/setup.cfg            |   3 +-
 6 files changed, 4 insertions(+), 410 deletions(-)
 delete mode 100644 python/qemu/qmp/README.rst
 delete mode 100644 python/qemu/qmp/__init__.py
 delete mode 100644 python/qemu/qmp/py.typed

diff --git a/python/PACKAGE.rst b/python/PACKAGE.rst
index b0b86cc4c3..ddfa9ba3f5 100644
--- a/python/PACKAGE.rst
+++ b/python/PACKAGE.rst
@@ -8,11 +8,11 @@ to change at any time.
 Usage
 -----
 
-The ``qemu.qmp`` subpackage provides a library for communicating with
+The ``qemu.aqmp`` subpackage provides a library for communicating with
 QMP servers. The ``qemu.machine`` subpackage offers rudimentary
 facilities for launching and managing QEMU processes. Refer to each
 package's documentation
-(``>>> help(qemu.qmp)``, ``>>> help(qemu.machine)``)
+(``>>> help(qemu.aqmp)``, ``>>> help(qemu.machine)``)
 for more information.
 
 Contributing
diff --git a/python/README.rst b/python/README.rst
index fcf74f69ea..eb5213337d 100644
--- a/python/README.rst
+++ b/python/README.rst
@@ -3,7 +3,7 @@ QEMU Python Tooling
 
 This directory houses Python tooling used by the QEMU project to build,
 configure, and test QEMU. It is organized by namespace (``qemu``), and
-then by package (e.g. ``qemu/machine``, ``qemu/qmp``, etc).
+then by package (e.g. ``qemu/machine``, ``qemu/aqmp``, etc).
 
 ``setup.py`` is used by ``pip`` to install this tooling to the current
 environment. ``setup.cfg`` provides the packaging configuration used by
diff --git a/python/qemu/qmp/README.rst b/python/qemu/qmp/README.rst
deleted file mode 100644
index 5bfb82535f..0000000000
--- a/python/qemu/qmp/README.rst
+++ /dev/null
@@ -1,9 +0,0 @@
-qemu.qmp package
-================
-
-This package provides a library used for connecting to and communicating
-with QMP servers. It is used extensively by iotests, vm tests,
-avocado tests, and other utilities in the ./scripts directory. It is
-not a fully-fledged SDK and is subject to change at any time.
-
-See the documentation in ``__init__.py`` for more information.
diff --git a/python/qemu/qmp/__init__.py b/python/qemu/qmp/__init__.py
deleted file mode 100644
index 4e08641154..0000000000
--- a/python/qemu/qmp/__init__.py
+++ /dev/null
@@ -1,396 +0,0 @@
-"""
-QEMU Monitor Protocol (QMP) development library & tooling.
-
-This package provides a fairly low-level class for communicating to QMP
-protocol servers, as implemented by QEMU, the QEMU Guest Agent, and the
-QEMU Storage Daemon. This library is not intended for production use.
-
-`QEMUMonitorProtocol` is the primary class of interest, and all errors
-raised derive from `QMPError`.
-"""
-
-# Copyright (C) 2009, 2010 Red Hat Inc.
-#
-# Authors:
-#  Luiz Capitulino <lcapitulino@redhat.com>
-#
-# This work is licensed under the terms of the GNU GPL, version 2.  See
-# the COPYING file in the top-level directory.
-
-import errno
-import json
-import logging
-import socket
-import struct
-from types import TracebackType
-from typing import (
-    Any,
-    Dict,
-    List,
-    Optional,
-    TextIO,
-    Tuple,
-    Type,
-    TypeVar,
-    Union,
-    cast,
-)
-
-
-#: QMPMessage is an entire QMP message of any kind.
-QMPMessage = Dict[str, Any]
-
-#: QMPReturnValue is the 'return' value of a command.
-QMPReturnValue = object
-
-#: QMPObject is any object in a QMP message.
-QMPObject = Dict[str, object]
-
-# QMPMessage can be outgoing commands or incoming events/returns.
-# QMPReturnValue is usually a dict/json object, but due to QAPI's
-# 'returns-whitelist', it can actually be anything.
-#
-# {'return': {}} is a QMPMessage,
-# {} is the QMPReturnValue.
-
-
-InternetAddrT = Tuple[str, int]
-UnixAddrT = str
-SocketAddrT = Union[InternetAddrT, UnixAddrT]
-
-
-class QMPError(Exception):
-    """
-    QMP base exception
-    """
-
-
-class QMPConnectError(QMPError):
-    """
-    QMP connection exception
-    """
-
-
-class QMPCapabilitiesError(QMPError):
-    """
-    QMP negotiate capabilities exception
-    """
-
-
-class QMPTimeoutError(QMPError):
-    """
-    QMP timeout exception
-    """
-
-
-class QMPProtocolError(QMPError):
-    """
-    QMP protocol error; unexpected response
-    """
-
-
-class QMPResponseError(QMPError):
-    """
-    Represents erroneous QMP monitor reply
-    """
-    def __init__(self, reply: QMPMessage):
-        try:
-            desc = reply['error']['desc']
-        except KeyError:
-            desc = reply
-        super().__init__(desc)
-        self.reply = reply
-
-
-class QEMUMonitorProtocol:
-    """
-    Provide an API to connect to QEMU via QEMU Monitor Protocol (QMP) and then
-    allow to handle commands and events.
-    """
-
-    #: Logger object for debugging messages
-    logger = logging.getLogger('QMP')
-
-    def __init__(self, address: SocketAddrT,
-                 server: bool = False,
-                 nickname: Optional[str] = None):
-        """
-        Create a QEMUMonitorProtocol class.
-
-        @param address: QEMU address, can be either a unix socket path (string)
-                        or a tuple in the form ( address, port ) for a TCP
-                        connection
-        @param server: server mode listens on the socket (bool)
-        @raise OSError on socket connection errors
-        @note No connection is established, this is done by the connect() or
-              accept() methods
-        """
-        self.__events: List[QMPMessage] = []
-        self.__address = address
-        self.__sock = self.__get_sock()
-        self.__sockfile: Optional[TextIO] = None
-        self._nickname = nickname
-        if self._nickname:
-            self.logger = logging.getLogger('QMP').getChild(self._nickname)
-        if server:
-            self.__sock.setsockopt(socket.SOL_SOCKET, socket.SO_REUSEADDR, 1)
-            self.__sock.bind(self.__address)
-            self.__sock.listen(1)
-
-    def __get_sock(self) -> socket.socket:
-        if isinstance(self.__address, tuple):
-            family = socket.AF_INET
-        else:
-            family = socket.AF_UNIX
-        return socket.socket(family, socket.SOCK_STREAM)
-
-    def __negotiate_capabilities(self) -> QMPMessage:
-        greeting = self.__json_read()
-        if greeting is None or "QMP" not in greeting:
-            raise QMPConnectError
-        # Greeting seems ok, negotiate capabilities
-        resp = self.cmd('qmp_capabilities')
-        if resp and "return" in resp:
-            return greeting
-        raise QMPCapabilitiesError
-
-    def __json_read(self, only_event: bool = False) -> Optional[QMPMessage]:
-        assert self.__sockfile is not None
-        while True:
-            data = self.__sockfile.readline()
-            if not data:
-                return None
-            # By definition, any JSON received from QMP is a QMPMessage,
-            # and we are asserting only at static analysis time that it
-            # has a particular shape.
-            resp: QMPMessage = json.loads(data)
-            if 'event' in resp:
-                self.logger.debug("<<< %s", resp)
-                self.__events.append(resp)
-                if not only_event:
-                    continue
-            return resp
-
-    def __get_events(self, wait: Union[bool, float] = False) -> None:
-        """
-        Check for new events in the stream and cache them in __events.
-
-        @param wait (bool): block until an event is available.
-        @param wait (float): If wait is a float, treat it as a timeout value.
-
-        @raise QMPTimeoutError: If a timeout float is provided and the timeout
-                                period elapses.
-        @raise QMPConnectError: If wait is True but no events could be
-                                retrieved or if some other error occurred.
-        """
-
-        # Current timeout and blocking status
-        current_timeout = self.__sock.gettimeout()
-
-        # Check for new events regardless and pull them into the cache:
-        self.__sock.settimeout(0)  # i.e. setblocking(False)
-        try:
-            self.__json_read()
-        except OSError as err:
-            # EAGAIN: No data available; not critical
-            if err.errno != errno.EAGAIN:
-                raise
-        finally:
-            self.__sock.settimeout(current_timeout)
-
-        # Wait for new events, if needed.
-        # if wait is 0.0, this means "no wait" and is also implicitly false.
-        if not self.__events and wait:
-            if isinstance(wait, float):
-                self.__sock.settimeout(wait)
-            try:
-                ret = self.__json_read(only_event=True)
-            except socket.timeout as err:
-                raise QMPTimeoutError("Timeout waiting for event") from err
-            except Exception as err:
-                msg = "Error while reading from socket"
-                raise QMPConnectError(msg) from err
-            finally:
-                self.__sock.settimeout(current_timeout)
-
-            if ret is None:
-                raise QMPConnectError("Error while reading from socket")
-
-    T = TypeVar('T')
-
-    def __enter__(self: T) -> T:
-        # Implement context manager enter function.
-        return self
-
-    def __exit__(self,
-                 # pylint: disable=duplicate-code
-                 # see https://github.com/PyCQA/pylint/issues/3619
-                 exc_type: Optional[Type[BaseException]],
-                 exc_val: Optional[BaseException],
-                 exc_tb: Optional[TracebackType]) -> None:
-        # Implement context manager exit function.
-        self.close()
-
-    def connect(self, negotiate: bool = True) -> Optional[QMPMessage]:
-        """
-        Connect to the QMP Monitor and perform capabilities negotiation.
-
-        @return QMP greeting dict, or None if negotiate is false
-        @raise OSError on socket connection errors
-        @raise QMPConnectError if the greeting is not received
-        @raise QMPCapabilitiesError if fails to negotiate capabilities
-        """
-        self.__sock.connect(self.__address)
-        self.__sockfile = self.__sock.makefile(mode='r')
-        if negotiate:
-            return self.__negotiate_capabilities()
-        return None
-
-    def accept(self, timeout: Optional[float] = 15.0) -> QMPMessage:
-        """
-        Await connection from QMP Monitor and perform capabilities negotiation.
-
-        @param timeout: timeout in seconds (nonnegative float number, or
-                        None). The value passed will set the behavior of the
-                        underneath QMP socket as described in [1].
-                        Default value is set to 15.0.
-
-        @return QMP greeting dict
-        @raise OSError on socket connection errors
-        @raise QMPConnectError if the greeting is not received
-        @raise QMPCapabilitiesError if fails to negotiate capabilities
-
-        [1]
-        https://docs.python.org/3/library/socket.html#socket.socket.settimeout
-        """
-        self.__sock.settimeout(timeout)
-        self.__sock, _ = self.__sock.accept()
-        self.__sockfile = self.__sock.makefile(mode='r')
-        return self.__negotiate_capabilities()
-
-    def cmd_obj(self, qmp_cmd: QMPMessage) -> QMPMessage:
-        """
-        Send a QMP command to the QMP Monitor.
-
-        @param qmp_cmd: QMP command to be sent as a Python dict
-        @return QMP response as a Python dict
-        """
-        self.logger.debug(">>> %s", qmp_cmd)
-        self.__sock.sendall(json.dumps(qmp_cmd).encode('utf-8'))
-        resp = self.__json_read()
-        if resp is None:
-            raise QMPConnectError("Unexpected empty reply from server")
-        self.logger.debug("<<< %s", resp)
-        return resp
-
-    def cmd(self, name: str,
-            args: Optional[Dict[str, object]] = None,
-            cmd_id: Optional[object] = None) -> QMPMessage:
-        """
-        Build a QMP command and send it to the QMP Monitor.
-
-        @param name: command name (string)
-        @param args: command arguments (dict)
-        @param cmd_id: command id (dict, list, string or int)
-        """
-        qmp_cmd: QMPMessage = {'execute': name}
-        if args:
-            qmp_cmd['arguments'] = args
-        if cmd_id:
-            qmp_cmd['id'] = cmd_id
-        return self.cmd_obj(qmp_cmd)
-
-    def command(self, cmd: str, **kwds: object) -> QMPReturnValue:
-        """
-        Build and send a QMP command to the monitor, report errors if any
-        """
-        ret = self.cmd(cmd, kwds)
-        if 'error' in ret:
-            raise QMPResponseError(ret)
-        if 'return' not in ret:
-            raise QMPProtocolError(
-                "'return' key not found in QMP response '{}'".format(str(ret))
-            )
-        return cast(QMPReturnValue, ret['return'])
-
-    def pull_event(self,
-                   wait: Union[bool, float] = False) -> Optional[QMPMessage]:
-        """
-        Pulls a single event.
-
-        @param wait (bool): block until an event is available.
-        @param wait (float): If wait is a float, treat it as a timeout value.
-
-        @raise QMPTimeoutError: If a timeout float is provided and the timeout
-                                period elapses.
-        @raise QMPConnectError: If wait is True but no events could be
-                                retrieved or if some other error occurred.
-
-        @return The first available QMP event, or None.
-        """
-        self.__get_events(wait)
-
-        if self.__events:
-            return self.__events.pop(0)
-        return None
-
-    def get_events(self, wait: bool = False) -> List[QMPMessage]:
-        """
-        Get a list of available QMP events and clear all pending events.
-
-        @param wait (bool): block until an event is available.
-        @param wait (float): If wait is a float, treat it as a timeout value.
-
-        @raise QMPTimeoutError: If a timeout float is provided and the timeout
-                                period elapses.
-        @raise QMPConnectError: If wait is True but no events could be
-                                retrieved or if some other error occurred.
-
-        @return The list of available QMP events.
-        """
-        self.__get_events(wait)
-        events = self.__events
-        self.__events = []
-        return events
-
-    def clear_events(self) -> None:
-        """
-        Clear current list of pending events.
-        """
-        self.__events = []
-
-    def close(self) -> None:
-        """
-        Close the socket and socket file.
-        """
-        if self.__sock:
-            self.__sock.close()
-        if self.__sockfile:
-            self.__sockfile.close()
-
-    def settimeout(self, timeout: Optional[float]) -> None:
-        """
-        Set the socket timeout.
-
-        @param timeout (float): timeout in seconds (non-zero), or None.
-        @note This is a wrap around socket.settimeout
-
-        @raise ValueError: if timeout was set to 0.
-        """
-        if timeout == 0:
-            msg = "timeout cannot be 0; this engages non-blocking mode."
-            msg += " Use 'None' instead to disable timeouts."
-            raise ValueError(msg)
-        self.__sock.settimeout(timeout)
-
-    def send_fd_scm(self, fd: int) -> None:
-        """
-        Send a file descriptor to the remote via SCM_RIGHTS.
-        """
-        if self.__sock.family != socket.AF_UNIX:
-            raise RuntimeError("Can't use SCM_RIGHTS on non-AF_UNIX socket.")
-
-        self.__sock.sendmsg(
-            [b' '],
-            [(socket.SOL_SOCKET, socket.SCM_RIGHTS, struct.pack('@i', fd))]
-        )
diff --git a/python/qemu/qmp/py.typed b/python/qemu/qmp/py.typed
deleted file mode 100644
index e69de29bb2..0000000000
diff --git a/python/setup.cfg b/python/setup.cfg
index 510df23698..5140a5b322 100644
--- a/python/setup.cfg
+++ b/python/setup.cfg
@@ -24,10 +24,9 @@ classifiers =
 [options]
 python_requires = >= 3.6
 packages =
-    qemu.qmp
+    qemu.aqmp
     qemu.machine
     qemu.utils
-    qemu.aqmp
 
 [options.package_data]
 * = py.typed
-- 
2.31.1

[PATCH v2 24/25] python: re-enable pylint duplicate-code warnings

[John Snow]

With the old library gone, there's nothing duplicated in the tree, so
the warning suppression can be removed.

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

diff --git a/python/setup.cfg b/python/setup.cfg
index 5140a5b322..c341e922c2 100644
--- a/python/setup.cfg
+++ b/python/setup.cfg
@@ -114,7 +114,6 @@ ignore_missing_imports = True
 disable=consider-using-f-string,
         too-many-function-args,  # mypy handles this with less false positives.
         no-member,  # mypy also handles this better.
-        duplicate-code,  # To be removed by the end of this patch series.
 
 [pylint.basic]
 # Good variable names which should always be accepted, separated by a comma.
-- 
2.31.1

[PATCH v2 25/25] python: rename qemu.aqmp to qemu.qmp

[John Snow]

Now that we are fully switched over to the new QMP library, move it back
over the old namespace. This is being done primarily so that we may
upload this package simply as "qemu.qmp" without introducing confusion
over whether or not "aqmp" is a new protocol or not.

The trade-off is increased confusion inside the QEMU developer
tree. Sorry!

Signed-off-by: John Snow <jsnow@redhat.com>
---
 python/PACKAGE.rst                        |  4 ++--
 python/README.rst                         |  4 ++--
 python/qemu/machine/machine.py            |  4 ++--
 python/qemu/machine/qtest.py              |  2 +-
 python/qemu/{aqmp => qmp}/__init__.py     |  6 +++---
 python/qemu/{aqmp => qmp}/aqmp_tui.py     |  0
 python/qemu/{aqmp => qmp}/error.py        |  0
 python/qemu/{aqmp => qmp}/events.py       |  2 +-
 python/qemu/{aqmp => qmp}/legacy.py       |  0
 python/qemu/{aqmp => qmp}/message.py      |  0
 python/qemu/{aqmp => qmp}/models.py       |  0
 python/qemu/{aqmp => qmp}/protocol.py     |  4 ++--
 python/qemu/{aqmp => qmp}/py.typed        |  0
 python/qemu/{aqmp => qmp}/qmp_client.py   | 16 ++++++++--------
 python/qemu/{aqmp => qmp}/qmp_shell.py    |  4 ++--
 python/qemu/{aqmp => qmp}/util.py         |  0
 python/qemu/utils/qemu_ga_client.py       |  4 ++--
 python/qemu/utils/qom.py                  |  2 +-
 python/qemu/utils/qom_common.py           |  4 ++--
 python/qemu/utils/qom_fuse.py             |  2 +-
 python/setup.cfg                          |  8 ++++----
 python/tests/protocol.py                  | 14 +++++++-------
 scripts/cpu-x86-uarch-abi.py              |  2 +-
 scripts/device-crash-test                 |  4 ++--
 scripts/qmp/qmp-shell                     |  2 +-
 scripts/render_block_graph.py             |  4 ++--
 scripts/simplebench/bench_block_job.py    |  2 +-
 tests/qemu-iotests/iotests.py             |  2 +-
 tests/qemu-iotests/tests/mirror-top-perms |  6 +++---
 29 files changed, 51 insertions(+), 51 deletions(-)
 rename python/qemu/{aqmp => qmp}/__init__.py (87%)
 rename python/qemu/{aqmp => qmp}/aqmp_tui.py (100%)
 rename python/qemu/{aqmp => qmp}/error.py (100%)
 rename python/qemu/{aqmp => qmp}/events.py (99%)
 rename python/qemu/{aqmp => qmp}/legacy.py (100%)
 rename python/qemu/{aqmp => qmp}/message.py (100%)
 rename python/qemu/{aqmp => qmp}/models.py (100%)
 rename python/qemu/{aqmp => qmp}/protocol.py (99%)
 rename python/qemu/{aqmp => qmp}/py.typed (100%)
 rename python/qemu/{aqmp => qmp}/qmp_client.py (97%)
 rename python/qemu/{aqmp => qmp}/qmp_shell.py (99%)
 rename python/qemu/{aqmp => qmp}/util.py (100%)

diff --git a/python/PACKAGE.rst b/python/PACKAGE.rst
index ddfa9ba3f5..b0b86cc4c3 100644
--- a/python/PACKAGE.rst
+++ b/python/PACKAGE.rst
@@ -8,11 +8,11 @@ to change at any time.
 Usage
 -----
 
-The ``qemu.aqmp`` subpackage provides a library for communicating with
+The ``qemu.qmp`` subpackage provides a library for communicating with
 QMP servers. The ``qemu.machine`` subpackage offers rudimentary
 facilities for launching and managing QEMU processes. Refer to each
 package's documentation
-(``>>> help(qemu.aqmp)``, ``>>> help(qemu.machine)``)
+(``>>> help(qemu.qmp)``, ``>>> help(qemu.machine)``)
 for more information.
 
 Contributing
diff --git a/python/README.rst b/python/README.rst
index eb5213337d..9c1fceaee7 100644
--- a/python/README.rst
+++ b/python/README.rst
@@ -3,7 +3,7 @@ QEMU Python Tooling
 
 This directory houses Python tooling used by the QEMU project to build,
 configure, and test QEMU. It is organized by namespace (``qemu``), and
-then by package (e.g. ``qemu/machine``, ``qemu/aqmp``, etc).
+then by package (e.g. ``qemu/machine``, ``qemu/qmp``, etc).
 
 ``setup.py`` is used by ``pip`` to install this tooling to the current
 environment. ``setup.cfg`` provides the packaging configuration used by
@@ -59,7 +59,7 @@ Package installation also normally provides executable console scripts,
 so that tools like ``qmp-shell`` are always available via $PATH. To
 invoke them without installation, you can invoke e.g.:
 
-``> PYTHONPATH=~/src/qemu/python python3 -m qemu.aqmp.qmp_shell``
+``> PYTHONPATH=~/src/qemu/python python3 -m qemu.qmp.qmp_shell``
 
 The mappings between console script name and python module path can be
 found in ``setup.cfg``.
diff --git a/python/qemu/machine/machine.py b/python/qemu/machine/machine.py
index 21fb4a4f30..6e4bf6520c 100644
--- a/python/qemu/machine/machine.py
+++ b/python/qemu/machine/machine.py
@@ -40,8 +40,8 @@
     TypeVar,
 )
 
-from qemu.aqmp import SocketAddrT
-from qemu.aqmp.legacy import (
+from qemu.qmp import SocketAddrT
+from qemu.qmp.legacy import (
     QEMUMonitorProtocol,
     QMPMessage,
     QMPReturnValue,
diff --git a/python/qemu/machine/qtest.py b/python/qemu/machine/qtest.py
index 817c8a5425..f29d5c1042 100644
--- a/python/qemu/machine/qtest.py
+++ b/python/qemu/machine/qtest.py
@@ -26,7 +26,7 @@
     TextIO,
 )
 
-from qemu.aqmp.protocol import SocketAddrT
+from qemu.qmp.protocol import SocketAddrT
 
 from .machine import QEMUMachine
 
diff --git a/python/qemu/aqmp/__init__.py b/python/qemu/qmp/__init__.py
similarity index 87%
rename from python/qemu/aqmp/__init__.py
rename to python/qemu/qmp/__init__.py
index e1efab00af..b3f1d919e2 100644
--- a/python/qemu/aqmp/__init__.py
+++ b/python/qemu/qmp/__init__.py
@@ -6,8 +6,8 @@
 QEMU Guest Agent, and the QEMU Storage Daemon.
 
 `QMPClient` provides the main functionality of this package. All errors
-raised by this library dervive from `QMPError`, see `aqmp.error` for
-additional detail. See `aqmp.events` for an in-depth tutorial on
+raised by this library dervive from `QMPError`, see `qmp.error` for
+additional detail. See `qmp.events` for an in-depth tutorial on
 managing QMP events.
 """
 
@@ -36,7 +36,7 @@
 
 
 # Suppress logging unless an application engages it.
-logging.getLogger('qemu.aqmp').addHandler(logging.NullHandler())
+logging.getLogger('qemu.qmp').addHandler(logging.NullHandler())
 
 
 # The order of these fields impact the Sphinx documentation order.
diff --git a/python/qemu/aqmp/aqmp_tui.py b/python/qemu/qmp/aqmp_tui.py
similarity index 100%
rename from python/qemu/aqmp/aqmp_tui.py
rename to python/qemu/qmp/aqmp_tui.py
diff --git a/python/qemu/aqmp/error.py b/python/qemu/qmp/error.py
similarity index 100%
rename from python/qemu/aqmp/error.py
rename to python/qemu/qmp/error.py
diff --git a/python/qemu/aqmp/events.py b/python/qemu/qmp/events.py
similarity index 99%
rename from python/qemu/aqmp/events.py
rename to python/qemu/qmp/events.py
index f3d4e2b5e8..6199776cc6 100644
--- a/python/qemu/aqmp/events.py
+++ b/python/qemu/qmp/events.py
@@ -1,5 +1,5 @@
 """
-AQMP Events and EventListeners
+QMP Events and EventListeners
 
 Asynchronous QMP uses `EventListener` objects to listen for events. An
 `EventListener` is a FIFO event queue that can be pre-filtered to listen
diff --git a/python/qemu/aqmp/legacy.py b/python/qemu/qmp/legacy.py
similarity index 100%
rename from python/qemu/aqmp/legacy.py
rename to python/qemu/qmp/legacy.py
diff --git a/python/qemu/aqmp/message.py b/python/qemu/qmp/message.py
similarity index 100%
rename from python/qemu/aqmp/message.py
rename to python/qemu/qmp/message.py
diff --git a/python/qemu/aqmp/models.py b/python/qemu/qmp/models.py
similarity index 100%
rename from python/qemu/aqmp/models.py
rename to python/qemu/qmp/models.py
diff --git a/python/qemu/aqmp/protocol.py b/python/qemu/qmp/protocol.py
similarity index 99%
rename from python/qemu/aqmp/protocol.py
rename to python/qemu/qmp/protocol.py
index 9ee6fe4ae2..5d417a95c3 100644
--- a/python/qemu/aqmp/protocol.py
+++ b/python/qemu/qmp/protocol.py
@@ -193,9 +193,9 @@ class AsyncProtocol(Generic[T]):
 
     :param name:
         Name used for logging messages, if any. By default, messages
-        will log to 'qemu.aqmp.protocol', but each individual connection
+        will log to 'qemu.qmp.protocol', but each individual connection
         can be given its own logger by giving it a name; messages will
-        then log to 'qemu.aqmp.protocol.${name}'.
+        then log to 'qemu.qmp.protocol.${name}'.
     """
     # pylint: disable=too-many-instance-attributes
 
diff --git a/python/qemu/aqmp/py.typed b/python/qemu/qmp/py.typed
similarity index 100%
rename from python/qemu/aqmp/py.typed
rename to python/qemu/qmp/py.typed
diff --git a/python/qemu/aqmp/qmp_client.py b/python/qemu/qmp/qmp_client.py
similarity index 97%
rename from python/qemu/aqmp/qmp_client.py
rename to python/qemu/qmp/qmp_client.py
index f1a845cc82..8ea9e45115 100644
--- a/python/qemu/aqmp/qmp_client.py
+++ b/python/qemu/qmp/qmp_client.py
@@ -192,7 +192,7 @@ async def run(self, address='/tmp/qemu.socket'):
               await self.qmp.runstate_changed.wait()
               await self.disconnect()
 
-    See `aqmp.events` for more detail on event handling patterns.
+    See `qmp.events` for more detail on event handling patterns.
     """
     #: Logger object used for debugging messages.
     logger = logging.getLogger(__name__)
@@ -416,7 +416,7 @@ def _do_send(self, msg: Message) -> None:
 
     @upper_half
     def _get_exec_id(self) -> str:
-        exec_id = f"__aqmp#{self._execute_id:05d}"
+        exec_id = f"__qmp#{self._execute_id:05d}"
         self._execute_id += 1
         return exec_id
 
@@ -476,7 +476,7 @@ async def _execute(self, msg: Message, assign_id: bool = True) -> Message:
         An execution ID will be assigned if assign_id is `True`. It can be
         disabled, but this requires that an ID is manually assigned
         instead. For manually assigned IDs, you must not use the string
-        '__aqmp#' anywhere in the ID.
+        '__qmp#' anywhere in the ID.
 
         :param msg: The QMP `Message` to execute.
         :param assign_id: If True, assign a new execution ID.
@@ -490,7 +490,7 @@ async def _execute(self, msg: Message, assign_id: bool = True) -> Message:
             msg['id'] = self._get_exec_id()
         elif 'id' in msg:
             assert isinstance(msg['id'], str)
-            assert '__aqmp#' not in msg['id']
+            assert '__qmp#' not in msg['id']
 
         exec_id = await self._issue(msg)
         return await self._reply(exec_id)
@@ -512,7 +512,7 @@ async def _raw(
             Assign an arbitrary execution ID to this message. If
             `False`, the existing id must either be absent (and no other
             such pending execution may omit an ID) or a string. If it is
-            a string, it must not start with '__aqmp#' and no other such
+            a string, it must not start with '__qmp#' and no other such
             pending execution may currently be using that ID.
 
         :return: Execution reply from the server.
@@ -524,7 +524,7 @@ async def _raw(
             When assign_id is `False`, an ID is given, and it is not a string.
         :raise ValueError:
             When assign_id is `False`, but the ID is not usable;
-            Either because it starts with '__aqmp#' or it is already in-use.
+            Either because it starts with '__qmp#' or it is already in-use.
         """
         # 1. convert generic Mapping or bytes to a QMP Message
         # 2. copy Message objects so that we assign an ID only to the copy.
@@ -534,9 +534,9 @@ async def _raw(
         if not assign_id and 'id' in msg:
             if not isinstance(exec_id, str):
                 raise TypeError(f"ID ('{exec_id}') must be a string.")
-            if exec_id.startswith('__aqmp#'):
+            if exec_id.startswith('__qmp#'):
                 raise ValueError(
-                    f"ID ('{exec_id}') must not start with '__aqmp#'."
+                    f"ID ('{exec_id}') must not start with '__qmp#'."
                 )
 
         if not assign_id and exec_id in self._pending:
diff --git a/python/qemu/aqmp/qmp_shell.py b/python/qemu/qmp/qmp_shell.py
similarity index 99%
rename from python/qemu/aqmp/qmp_shell.py
rename to python/qemu/qmp/qmp_shell.py
index d11bf54b00..571110f4f8 100644
--- a/python/qemu/aqmp/qmp_shell.py
+++ b/python/qemu/qmp/qmp_shell.py
@@ -95,8 +95,8 @@
     Sequence,
 )
 
-from qemu.aqmp import ConnectError, QMPError, SocketAddrT
-from qemu.aqmp.legacy import (
+from qemu.qmp import ConnectError, QMPError, SocketAddrT
+from qemu.qmp.legacy import (
     QEMUMonitorProtocol,
     QMPBadPortError,
     QMPMessage,
diff --git a/python/qemu/aqmp/util.py b/python/qemu/qmp/util.py
similarity index 100%
rename from python/qemu/aqmp/util.py
rename to python/qemu/qmp/util.py
diff --git a/python/qemu/utils/qemu_ga_client.py b/python/qemu/utils/qemu_ga_client.py
index 15ed430c61..8c38a7ac9c 100644
--- a/python/qemu/utils/qemu_ga_client.py
+++ b/python/qemu/utils/qemu_ga_client.py
@@ -50,8 +50,8 @@
     Sequence,
 )
 
-from qemu.aqmp import ConnectError, SocketAddrT
-from qemu.aqmp.legacy import QEMUMonitorProtocol
+from qemu.qmp import ConnectError, SocketAddrT
+from qemu.qmp.legacy import QEMUMonitorProtocol
 
 
 # This script has not seen many patches or careful attention in quite
diff --git a/python/qemu/utils/qom.py b/python/qemu/utils/qom.py
index bb5d1a78f5..bcf192f477 100644
--- a/python/qemu/utils/qom.py
+++ b/python/qemu/utils/qom.py
@@ -32,7 +32,7 @@
 
 import argparse
 
-from qemu.aqmp import ExecuteError
+from qemu.qmp import ExecuteError
 
 from .qom_common import QOMCommand
 
diff --git a/python/qemu/utils/qom_common.py b/python/qemu/utils/qom_common.py
index 6f07451dfa..7f5682fc04 100644
--- a/python/qemu/utils/qom_common.py
+++ b/python/qemu/utils/qom_common.py
@@ -27,8 +27,8 @@
     TypeVar,
 )
 
-from qemu.aqmp import QMPError
-from qemu.aqmp.legacy import QEMUMonitorProtocol
+from qemu.qmp import QMPError
+from qemu.qmp.legacy import QEMUMonitorProtocol
 
 
 # The following is needed only for a type alias.
diff --git a/python/qemu/utils/qom_fuse.py b/python/qemu/utils/qom_fuse.py
index 653a76b93b..8dcd59fcde 100644
--- a/python/qemu/utils/qom_fuse.py
+++ b/python/qemu/utils/qom_fuse.py
@@ -48,7 +48,7 @@
 import fuse
 from fuse import FUSE, FuseOSError, Operations
 
-from qemu.aqmp import ExecuteError
+from qemu.qmp import ExecuteError
 
 from .qom_common import QOMCommand
 
diff --git a/python/setup.cfg b/python/setup.cfg
index c341e922c2..911ae02de7 100644
--- a/python/setup.cfg
+++ b/python/setup.cfg
@@ -24,7 +24,7 @@ classifiers =
 [options]
 python_requires = >= 3.6
 packages =
-    qemu.aqmp
+    qemu.qmp
     qemu.machine
     qemu.utils
 
@@ -66,8 +66,8 @@ console_scripts =
     qom-tree = qemu.utils.qom:QOMTree.entry_point
     qom-fuse = qemu.utils.qom_fuse:QOMFuse.entry_point [fuse]
     qemu-ga-client = qemu.utils.qemu_ga_client:main
-    qmp-shell = qemu.aqmp.qmp_shell:main
-    aqmp-tui = qemu.aqmp.aqmp_tui:main [tui]
+    qmp-shell = qemu.qmp.qmp_shell:main
+    aqmp-tui = qemu.qmp.aqmp_tui:main [tui]
 
 [flake8]
 extend-ignore = E722  # Prefer pylint's bare-except checks to flake8's
@@ -83,7 +83,7 @@ namespace_packages = True
 # fusepy has no type stubs:
 allow_subclassing_any = True
 
-[mypy-qemu.aqmp.aqmp_tui]
+[mypy-qemu.qmp.aqmp_tui]
 # urwid and urwid_readline have no type stubs:
 allow_subclassing_any = True
 
diff --git a/python/tests/protocol.py b/python/tests/protocol.py
index 5cd7938be3..8864e66a94 100644
--- a/python/tests/protocol.py
+++ b/python/tests/protocol.py
@@ -6,9 +6,9 @@
 
 import avocado
 
-from qemu.aqmp import ConnectError, Runstate
-from qemu.aqmp.protocol import AsyncProtocol, StateError
-from qemu.aqmp.util import asyncio_run, create_task
+from qemu.qmp import ConnectError, Runstate
+from qemu.qmp.protocol import AsyncProtocol, StateError
+from qemu.qmp.util import asyncio_run, create_task
 
 
 class NullProtocol(AsyncProtocol[None]):
@@ -170,7 +170,7 @@ def testDefaultName(self):
     def testLogger(self):
         self.assertEqual(
             self.proto.logger.name,
-            'qemu.aqmp.protocol'
+            'qemu.qmp.protocol'
         )
 
     def testName(self):
@@ -183,7 +183,7 @@ def testName(self):
 
         self.assertEqual(
             self.proto.logger.name,
-            'qemu.aqmp.protocol.Steve'
+            'qemu.qmp.protocol.Steve'
         )
 
         self.assertEqual(
@@ -418,7 +418,7 @@ async def _bad_connection(self, family: str):
             await self.proto.accept('/dev/null')
 
     async def _hanging_connection(self):
-        with TemporaryDirectory(suffix='.aqmp') as tmpdir:
+        with TemporaryDirectory(suffix='.qmp') as tmpdir:
             sock = os.path.join(tmpdir, type(self.proto).__name__ + ".sock")
             await self.proto.accept(sock)
 
@@ -574,7 +574,7 @@ async def _asyncTearDown(self):
 
     @TestBase.async_test
     async def testSmoke(self):
-        with TemporaryDirectory(suffix='.aqmp') as tmpdir:
+        with TemporaryDirectory(suffix='.qmp') as tmpdir:
             sock = os.path.join(tmpdir, type(self.proto).__name__ + ".sock")
             server_task = create_task(self.server.accept(sock))
 
diff --git a/scripts/cpu-x86-uarch-abi.py b/scripts/cpu-x86-uarch-abi.py
index c262d2f027..82ff07582f 100644
--- a/scripts/cpu-x86-uarch-abi.py
+++ b/scripts/cpu-x86-uarch-abi.py
@@ -6,7 +6,7 @@
 # compatibility levels for each CPU model.
 #
 
-from qemu.aqmp.legacy import QEMUMonitorProtocol
+from qemu.qmp.legacy import QEMUMonitorProtocol
 import sys
 
 if len(sys.argv) != 2:
diff --git a/scripts/device-crash-test b/scripts/device-crash-test
index 7fbd99158b..4bfc68c008 100755
--- a/scripts/device-crash-test
+++ b/scripts/device-crash-test
@@ -36,7 +36,7 @@ from itertools import chain
 
 sys.path.append(os.path.join(os.path.dirname(__file__), '..', 'python'))
 from qemu.machine import QEMUMachine
-from qemu.aqmp import ConnectError
+from qemu.qmp import ConnectError
 
 logger = logging.getLogger('device-crash-test')
 dbg = logger.debug
@@ -517,7 +517,7 @@ def main():
         # Async QMP, when in use, is chatty about connection failures.
         # This script knowingly generates a ton of connection errors.
         # Silence this logger.
-        logging.getLogger('qemu.aqmp.qmp_client').setLevel(logging.CRITICAL)
+        logging.getLogger('qemu.qmp.qmp_client').setLevel(logging.CRITICAL)
 
     fatal_failures = []
     wl_stats = {}
diff --git a/scripts/qmp/qmp-shell b/scripts/qmp/qmp-shell
index 31b19d73e2..4a20f97db7 100755
--- a/scripts/qmp/qmp-shell
+++ b/scripts/qmp/qmp-shell
@@ -4,7 +4,7 @@ import os
 import sys
 
 sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..', 'python'))
-from qemu.aqmp import qmp_shell
+from qemu.qmp import qmp_shell
 
 
 if __name__ == '__main__':
diff --git a/scripts/render_block_graph.py b/scripts/render_block_graph.py
index 97778927f3..26f43fef27 100755
--- a/scripts/render_block_graph.py
+++ b/scripts/render_block_graph.py
@@ -25,8 +25,8 @@
 from graphviz import Digraph
 
 sys.path.append(os.path.join(os.path.dirname(__file__), '..', 'python'))
-from qemu.aqmp import QMPError
-from qemu.aqmp.legacy import QEMUMonitorProtocol
+from qemu.qmp import QMPError
+from qemu.qmp.legacy import QEMUMonitorProtocol
 
 
 def perm(arr):
diff --git a/scripts/simplebench/bench_block_job.py b/scripts/simplebench/bench_block_job.py
index af9d1646a4..56191db44b 100755
--- a/scripts/simplebench/bench_block_job.py
+++ b/scripts/simplebench/bench_block_job.py
@@ -27,7 +27,7 @@
 
 sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..', 'python'))
 from qemu.machine import QEMUMachine
-from qemu.aqmp import ConnectError
+from qemu.qmp import ConnectError
 
 
 def bench_block_job(cmd, cmd_args, qemu_args):
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index cb21aebe36..28c1cbc30f 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -37,7 +37,7 @@
 from contextlib import contextmanager
 
 from qemu.machine import qtest
-from qemu.aqmp.legacy import QMPMessage
+from qemu.qmp.legacy import QMPMessage
 
 # Use this logger for logging messages directly from the iotests module
 logger = logging.getLogger('qemu.iotests')
diff --git a/tests/qemu-iotests/tests/mirror-top-perms b/tests/qemu-iotests/tests/mirror-top-perms
index f394931a00..d0b4449b35 100755
--- a/tests/qemu-iotests/tests/mirror-top-perms
+++ b/tests/qemu-iotests/tests/mirror-top-perms
@@ -21,7 +21,7 @@
 
 import os
 
-from qemu.aqmp import ConnectError
+from qemu.qmp import ConnectError
 from qemu.machine import machine
 
 import iotests
@@ -99,10 +99,10 @@ class TestMirrorTopPerms(iotests.QMPTestCase):
         self.vm_b.add_blockdev(f'file,node-name=drive0,filename={source}')
         self.vm_b.add_device('virtio-blk,drive=drive0,share-rw=on')
         try:
-            # Silence AQMP errors temporarily.
+            # Silence QMP errors temporarily.
             # TODO: Remove change_log_level and allow the errors to be logged.
             #       This necessitates a PID filter on *all* logging output.
-            with change_log_level('qemu.aqmp'):
+            with change_log_level('qemu.qmp'):
                 self.vm_b.launch()
                 print('ERROR: VM B launched successfully, '
                       'this should not have happened')
-- 
2.31.1

Re: [PATCH v2 01/25] python/aqmp: add __del__ method to legacy interface

[Vladimir Sementsov-Ogievskiy]

15.12.2021 22:39, John Snow wrote:
> asyncio can complain *very* loudly if you forget to back out of things
> gracefully before the garbage collector starts destroying objects that
> contain live references to asyncio Tasks.
> 
> The usual fix is just to remember to call aqmp.disconnect(), but for the
> sake of the legacy wrapper and quick, one-off scripts where a graceful
> shutdown is not necessarily of paramount imporance, add a courtesy
> cleanup that will trigger prior to seeing screenfuls of confusing
> asyncio tracebacks.
> 
> Note that we can't *always* save you from yourself; depending on when
> the GC runs, you might just seriously be out of luck. The best we can do
> in this case is to gently remind you to clean up after yourself.
> 
> (Still much better than multiple pages of incomprehensible python
> warnings for the crime of forgetting to put your toys away.)
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>   python/qemu/aqmp/legacy.py | 18 ++++++++++++++++++
>   1 file changed, 18 insertions(+)
> 
> diff --git a/python/qemu/aqmp/legacy.py b/python/qemu/aqmp/legacy.py
> index 9e7b9fb80b..2ccb136b02 100644
> --- a/python/qemu/aqmp/legacy.py
> +++ b/python/qemu/aqmp/legacy.py
> @@ -16,6 +16,8 @@
>   import qemu.qmp
>   from qemu.qmp import QMPMessage, QMPReturnValue, SocketAddrT
>   
> +from .error import AQMPError
> +from .protocol import Runstate
>   from .qmp_client import QMPClient
>   
>   
> @@ -136,3 +138,19 @@ def settimeout(self, timeout: Optional[float]) -> None:
>   
>       def send_fd_scm(self, fd: int) -> None:
>           self._aqmp.send_fd_scm(fd)
> +
> +    def __del__(self) -> None:
> +        if self._aqmp.runstate == Runstate.IDLE:
> +            return
> +
> +        if not self._aloop.is_running():
> +            self.close()

As I understand, if close() was called by hand, runstate should already be IDLE, and we don't call close() twice here?

> +        else:
> +            # Garbage collection ran while the event loop was running.
> +            # Nothing we can do about it now, but if we don't raise our
> +            # own error, the user will be treated to a lot of traceback
> +            # they might not understand.
> +            raise AQMPError(
> +                "QEMUMonitorProtocol.close()"
> +                " was not called before object was garbage collected"
> +            )
> 

weak, as I'm far from understanding aqmp library:

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>


-- 
Best regards,
Vladimir

Re: [PATCH v2 02/25] python/aqmp: handle asyncio.TimeoutError on execute()

[Vladimir Sementsov-Ogievskiy]

15.12.2021 22:39, John Snow wrote:
> This exception can be injected into any await statement. If we are
> canceled via timeout, we want to clear the pending execution record on
> our way out.

Hmm, but there are more await statements in the file, shouldn't we care about them too ?

> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>   python/qemu/aqmp/qmp_client.py | 8 ++++++--
>   1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/python/qemu/aqmp/qmp_client.py b/python/qemu/aqmp/qmp_client.py
> index 8105e29fa8..6a985ffe30 100644
> --- a/python/qemu/aqmp/qmp_client.py
> +++ b/python/qemu/aqmp/qmp_client.py
> @@ -435,7 +435,11 @@ async def _issue(self, msg: Message) -> Union[None, str]:
>               msg_id = msg['id']
>   
>           self._pending[msg_id] = asyncio.Queue(maxsize=1)
> -        await self._outgoing.put(msg)
> +        try:
> +            await self._outgoing.put(msg)
> +        except:

Doesn't pylint and others complain about plain "except". Do we really need to catch any exception here? As far as I know that's not a good practice.

> +            del self._pending[msg_id]
> +            raise
>   
>           return msg_id
>   
> @@ -452,9 +456,9 @@ async def _reply(self, msg_id: Union[str, None]) -> Message:
>               was lost, or some other problem.
>           """
>           queue = self._pending[msg_id]
> -        result = await queue.get()
>   
>           try:
> +            result = await queue.get()
>               if isinstance(result, ExecInterruptedError):
>                   raise result
>               return result
> 

This one looks good, just include it into existing try-finally

Hmm. _issue() and _reply() are used only in one place, as a pair. It looks like both "awaits" should be better under one try-finally block.

For example, move "self._pending[msg_id] = asyncio.Queue(maxsize=1)" to _execute, and just do try-finally in _execute() around _issue and _reply. Or may be just merge the whole logic in _execute, it doesn't seem too much. What do you think?


-- 
Best regards,
Vladimir

Re: [PATCH v2 03/25] python/aqmp: copy type definitions from qmp

[Vladimir Sementsov-Ogievskiy]

15.12.2021 22:39, John Snow wrote:
> Copy the remaining type definitions from QMP into the qemu.aqmp.legacy
> module. Now, most users don't need to import anything else but
> qemu.aqmp.legacy.
> 
> Signed-off-by: John Snow<jsnow@redhat.com>

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

-- 
Best regards,
Vladimir

Re: [PATCH v2 04/25] python/aqmp: add SocketAddrT to package root

[Vladimir Sementsov-Ogievskiy]

15.12.2021 22:39, John Snow wrote:
> It's a commonly needed definition, it can be re-exported by the root.
> 
> Signed-off-by: John Snow<jsnow@redhat.com>

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

-- 
Best regards,
Vladimir

Re: [PATCH v2 05/25] python/aqmp: rename AQMPError to QMPError

[Vladimir Sementsov-Ogievskiy]

15.12.2021 22:39, John Snow wrote:
> This is in preparation for renaming qemu.aqmp to qemu.qmp. I should have
> done this from this from the very beginning, but it's a convenient time
> to make sure this churn is taken care of.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>

Honestly, I don't want to check how it intersects with exising QMPError from old library, as it's going to be removed anyway. Hope, everything is OK:

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

> ---
>   python/qemu/aqmp/__init__.py   |  6 +++---
>   python/qemu/aqmp/error.py      | 12 ++++++------
>   python/qemu/aqmp/events.py     |  4 ++--
>   python/qemu/aqmp/legacy.py     |  4 ++--
>   python/qemu/aqmp/protocol.py   |  8 ++++----
>   python/qemu/aqmp/qmp_client.py |  8 ++++----
>   6 files changed, 21 insertions(+), 21 deletions(-)
> 
> diff --git a/python/qemu/aqmp/__init__.py b/python/qemu/aqmp/__init__.py
> index c6fa2dda58..e1efab00af 100644
> --- a/python/qemu/aqmp/__init__.py
> +++ b/python/qemu/aqmp/__init__.py
> @@ -6,7 +6,7 @@
>   QEMU Guest Agent, and the QEMU Storage Daemon.
>   
>   `QMPClient` provides the main functionality of this package. All errors
> -raised by this library dervive from `AQMPError`, see `aqmp.error` for
> +raised by this library dervive from `QMPError`, see `aqmp.error` for

preexisting: s/dervive/derive/


-- 
Best regards,
Vladimir

Re: [PATCH v2 13/25] scripts/cpu-x86-uarch-abi: fix CLI parsing

[Daniel P. Berrangé]

On Wed, Dec 15, 2021 at 02:39:27PM -0500, John Snow wrote:
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  scripts/cpu-x86-uarch-abi.py | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


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 v2 14/25] scripts/cpu-x86-uarch-abi: switch to AQMP

[Daniel P. Berrangé]

On Wed, Dec 15, 2021 at 02:39:28PM -0500, John Snow wrote:
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  scripts/cpu-x86-uarch-abi.py | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


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 v2 06/25] python/qemu-ga-client: update instructions to newer CLI syntax

[Vladimir Sementsov-Ogievskiy]

I had to search a bit through the history to check this )

The commit ccd3b3b8112b670f "qemu-option: warn for short-form boolean options" may be noted here.

And may be subject changed to "don't use deprecated syntax in comment" or something like this. server=on is not a *new* syntax I think.

15.12.2021 22:39, John Snow wrote:
> Signed-off-by: John Snow <jsnow@redhat.com>

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

> ---
>   python/qemu/qmp/qemu_ga_client.py | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/python/qemu/qmp/qemu_ga_client.py b/python/qemu/qmp/qemu_ga_client.py
> index 67ac0b4211..b3e1d98c9e 100644
> --- a/python/qemu/qmp/qemu_ga_client.py
> +++ b/python/qemu/qmp/qemu_ga_client.py
> @@ -5,7 +5,7 @@
>   
>   Start QEMU with:
>   
> -# qemu [...] -chardev socket,path=/tmp/qga.sock,server,wait=off,id=qga0 \
> +# qemu [...] -chardev socket,path=/tmp/qga.sock,server=on,wait=off,id=qga0 \
>     -device virtio-serial \
>     -device virtserialport,chardev=qga0,name=org.qemu.guest_agent.0
>   
> 


-- 
Best regards,
Vladimir

Re: [PATCH v2 06/25] python/qemu-ga-client: update instructions to newer CLI syntax

[Daniel P. Berrangé]

On Wed, Dec 15, 2021 at 02:39:20PM -0500, John Snow wrote:
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  python/qemu/qmp/qemu_ga_client.py | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>

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 v2 03/25] python/aqmp: copy type definitions from qmp

[Daniel P. Berrangé]

On Wed, Dec 15, 2021 at 02:39:17PM -0500, John Snow wrote:
> Copy the remaining type definitions from QMP into the qemu.aqmp.legacy
> module. Now, most users don't need to import anything else but
> qemu.aqmp.legacy.

I'm probably missing the historical discussion but it feels very
wierd to be saying

   "most users don't need anything except  <something> legacy"

Naively, I'd expect most users to want something *not* legacy.


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 v2 07/25] python/qmp: switch qemu-ga-client to AQMP

[Vladimir Sementsov-Ogievskiy]

15.12.2021 22:39, John Snow wrote:
> Signed-off-by: John Snow <jsnow@redhat.com>

Not simple to check, how much new behavior is equal to the old one.. And impossible to check, is everything updated that should be )


> ---
>   python/qemu/qmp/qemu_ga_client.py | 22 +++++++++++-----------
>   1 file changed, 11 insertions(+), 11 deletions(-)
> 
> diff --git a/python/qemu/qmp/qemu_ga_client.py b/python/qemu/qmp/qemu_ga_client.py
> index b3e1d98c9e..15ed430c61 100644
> --- a/python/qemu/qmp/qemu_ga_client.py
> +++ b/python/qemu/qmp/qemu_ga_client.py

[..]

>   
>       try:
>           client = QemuGuestAgentClient(address)
> -    except OSError as err:
> +    except ConnectError as err:
>           print(err)
> -        if err.errno == errno.ECONNREFUSED:
> -            print('Hint: qemu is not running?')
> +        if isinstance(err.exc, ConnectionError):
> +            print('(Is QEMU running?)')

It at least a bit changed from checking errno to checking the class, I'd note it in commit message. And anyway commit message may be more informative. Still, I just don't care too much about testing framework.

Nothing seems wrong to me, so weak:
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

-- 
Best regards,
Vladimir

Re: [PATCH v2 08/25] python/qmp: switch qom tools to AQMP

[Vladimir Sementsov-Ogievskiy]

15.12.2021 22:39, John Snow wrote:
> Signed-off-by: John Snow<jsnow@redhat.com>

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

-- 
Best regards,
Vladimir

Re: [PATCH v2 09/25] python/qmp: switch qmp-shell to AQMP

[Vladimir Sementsov-Ogievskiy]

15.12.2021 22:39, John Snow wrote:
> We have a replacement for async QMP, but it doesn't have feature parity
> yet. For now, then, port the old tool onto the new backend.
> 
> Signed-off-by: John Snow<jsnow@redhat.com>

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

-- 
Best regards,
Vladimir

Re: [PATCH v2 10/25] python: move qmp utilities to python/qemu/utils

[Vladimir Sementsov-Ogievskiy]

15.12.2021 22:39, John Snow wrote:
> In order to upload a QMP package to PyPI, I want to remove any scripts
> that I am not 100% confident I want to support upstream, beyond our
> castle walls.
> 
> Move most of our QMP utilities into the utils package so we can split
> them out from the PyPI upload.
> 
> Signed-off-by: John Snow<jsnow@redhat.com>


Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

-- 
Best regards,
Vladimir

Re: [PATCH v2 11/25] python: move qmp-shell under the AQMP package

[Vladimir Sementsov-Ogievskiy]

15.12.2021 22:39, John Snow wrote:
> Signed-off-by: John Snow<jsnow@redhat.com>

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

-- 
Best regards,
Vladimir

Re: [PATCH v2 00/25] Python: delete synchronous qemu.qmp package

[Daniel P. Berrangé]

On Wed, Dec 15, 2021 at 02:39:14PM -0500, John Snow wrote:
> GitLab: https://gitlab.com/jsnow/qemu/-/commits/python-qmp-legacy-switch
> CI: https://gitlab.com/jsnow/qemu/-/pipelines/430491195
> 
> Hi, this series is part of an effort to publish the qemu.qmp package on
> PyPI. It is the first of three series to complete this work:
> 
> --> (1) Switch the new Async QMP library in to python/qemu/qmp
>     (2) Fork python/qemu/qmp out into its own repository,
>         with updated GitLab CI/CD targets to build packages.
>     (3) Update qemu.git to install qemu.qmp from PyPI,
>         and then delete python/qemu/qmp.

What timeframe are you suggesting step (3) for ?

In the series for (2) you're calling it version 0.0.1 indicating
it is liable to  have API incompatible changes.

For step (3), either we're going to have to fetch a precise
version number to avoid risk of API breakage, or we're going
to have to call it stable in (2) and commit to the API.


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 v2 12/25] python/machine: permanently switch to AQMP

[Vladimir Sementsov-Ogievskiy]

15.12.2021 22:39, John Snow wrote:
> Remove the QEMU_PYTHON_LEGACY_QMP environment variable, making the
> switch permanent. Update Exceptions and import paths as necessary.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>   python/qemu/machine/machine.py | 18 +++++++-----------
>   python/qemu/machine/qtest.py   |  2 +-
>   2 files changed, 8 insertions(+), 12 deletions(-)
> 
> diff --git a/python/qemu/machine/machine.py b/python/qemu/machine/machine.py
> index 67ab06ca2b..21fb4a4f30 100644
> --- a/python/qemu/machine/machine.py
> +++ b/python/qemu/machine/machine.py
> @@ -40,21 +40,16 @@
>       TypeVar,
>   )
>   
> -from qemu.qmp import (  # pylint: disable=import-error
> +from qemu.aqmp import SocketAddrT
> +from qemu.aqmp.legacy import (
> +    QEMUMonitorProtocol,
>       QMPMessage,
>       QMPReturnValue,
> -    SocketAddrT,
>   )
>   
>   from . import console_socket
>   
>   
> -if os.environ.get('QEMU_PYTHON_LEGACY_QMP'):
> -    from qemu.qmp import QEMUMonitorProtocol
> -else:
> -    from qemu.aqmp.legacy import QEMUMonitorProtocol
> -
> -
>   LOG = logging.getLogger(__name__)
>   
>   
> @@ -710,8 +705,9 @@ def events_wait(self,
>           :param timeout: Optional timeout, in seconds.
>                           See QEMUMonitorProtocol.pull_event.
>   
> -        :raise QMPTimeoutError: If timeout was non-zero and no matching events
> -                                were found.
> +        :raise asyncio.TimeoutError:
> +            If timeout was non-zero and no matching events were found.
> +
>           :return: A QMP event matching the filter criteria.
>                    If timeout was 0 and no event matched, None.
>           """
> @@ -734,7 +730,7 @@ def _match(event: QMPMessage) -> bool:
>               event = self._qmp.pull_event(wait=timeout)
>               if event is None:
>                   # NB: None is only returned when timeout is false-ish.
> -                # Timeouts raise QMPTimeoutError instead!
> +                # Timeouts raise asyncio.TimeoutError instead!
>                   break
>               if _match(event):
>                   return event
> diff --git a/python/qemu/machine/qtest.py b/python/qemu/machine/qtest.py
> index f2f9aaa5e5..817c8a5425 100644
> --- a/python/qemu/machine/qtest.py
> +++ b/python/qemu/machine/qtest.py
> @@ -26,7 +26,7 @@
>       TextIO,
>   )
>   
> -from qemu.qmp import SocketAddrT  # pylint: disable=import-error
> +from qemu.aqmp.protocol import SocketAddrT

You can also import it simply from qemu.aqmp

>   
>   from .machine import QEMUMachine
>   
> 

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

-- 
Best regards,
Vladimir

Re: [PATCH v2 13/25] scripts/cpu-x86-uarch-abi: fix CLI parsing

[Vladimir Sementsov-Ogievskiy]

15.12.2021 22:39, John Snow wrote:
> Signed-off-by: John Snow<jsnow@redhat.com>


Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

-- 
Best regards,
Vladimir

Re: [PATCH v2 14/25] scripts/cpu-x86-uarch-abi: switch to AQMP

[Vladimir Sementsov-Ogievskiy]

15.12.2021 22:39, John Snow wrote:
> Signed-off-by: John Snow<jsnow@redhat.com>


Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

-- 
Best regards,
Vladimir

Re: [PATCH v2 15/25] scripts/render-block-graph: switch to AQMP

[Vladimir Sementsov-Ogievskiy]

15.12.2021 22:39, John Snow wrote:
> Creating an instance of qemu.aqmp.ExecuteError is too involved here, so
> just drop the specificity down to a generic AQMPError.

s/AQMPError/QMPError/  ?

> 
> Signed-off-by: John Snow <jsnow@redhat.com>


Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

> ---
>   scripts/render_block_graph.py | 8 +++-----
>   1 file changed, 3 insertions(+), 5 deletions(-)
> 
> diff --git a/scripts/render_block_graph.py b/scripts/render_block_graph.py
> index da6acf050d..97778927f3 100755
> --- a/scripts/render_block_graph.py
> +++ b/scripts/render_block_graph.py
> @@ -25,10 +25,8 @@
>   from graphviz import Digraph
>   
>   sys.path.append(os.path.join(os.path.dirname(__file__), '..', 'python'))
> -from qemu.qmp import (
> -    QEMUMonitorProtocol,
> -    QMPResponseError,
> -)
> +from qemu.aqmp import QMPError
> +from qemu.aqmp.legacy import QEMUMonitorProtocol
>   
>   
>   def perm(arr):
> @@ -105,7 +103,7 @@ def command(self, cmd):
>           reply = json.loads(subprocess.check_output(ar))
>   
>           if 'error' in reply:
> -            raise QMPResponseError(reply)
> +            raise QMPError(reply)
>   
>           return reply['return']
>   
> 


-- 
Best regards,
Vladimir

Re: [PATCH v2 16/25] scripts/bench-block-job: switch to AQMP

[Vladimir Sementsov-Ogievskiy]

15.12.2021 22:39, John Snow wrote:
> For this commit, we only need to remove accommodations for the
> synchronous QMP library.
> 
> Signed-off-by: John Snow<jsnow@redhat.com>


Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

-- 
Best regards,
Vladimir

Re: [PATCH v2 17/25] iotests/mirror-top-perms: switch to AQMP

[Vladimir Sementsov-Ogievskiy]

15.12.2021 22:39, John Snow wrote:
> Signed-off-by: John Snow <jsnow@redhat.com>


Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

> 
> ---
> 
> Note: I still need to adjust the logging. The problem now is that the
> logging messages include the PID of the test process, so they need to be
> filtered out. I'll investigate that for a follow-up, or for v2.
> 
> I could just add yet another filtering function somewhere, but I think
> it's getting out of hand with how many filters and loggers there are, so
> I want to give it a slightly more serious treatment instead of a
> hackjob.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>

Hmm, something is wrong with your scripts or git-notes practices )

> ---
>   tests/qemu-iotests/tests/mirror-top-perms | 7 +++----
>   1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/tests/qemu-iotests/tests/mirror-top-perms b/tests/qemu-iotests/tests/mirror-top-perms
> index 0a51a613f3..f394931a00 100755
> --- a/tests/qemu-iotests/tests/mirror-top-perms
> +++ b/tests/qemu-iotests/tests/mirror-top-perms
> @@ -23,7 +23,6 @@ import os
>   
>   from qemu.aqmp import ConnectError
>   from qemu.machine import machine
> -from qemu.qmp import QMPConnectError
>   
>   import iotests
>   from iotests import change_log_level, qemu_img
> @@ -101,13 +100,13 @@ class TestMirrorTopPerms(iotests.QMPTestCase):
>           self.vm_b.add_device('virtio-blk,drive=drive0,share-rw=on')
>           try:
>               # Silence AQMP errors temporarily.
> -            # TODO: Remove this and just allow the errors to be logged when
> -            # AQMP fully replaces QMP.
> +            # TODO: Remove change_log_level and allow the errors to be logged.
> +            #       This necessitates a PID filter on *all* logging output.
>               with change_log_level('qemu.aqmp'):
>                   self.vm_b.launch()
>                   print('ERROR: VM B launched successfully, '
>                         'this should not have happened')
> -        except (QMPConnectError, ConnectError):
> +        except ConnectError:
>               assert 'Is another process using the image' in self.vm_b.get_log()
>   
>           result = self.vm.qmp('block-job-cancel',
> 


-- 
Best regards,
Vladimir

Re: [PATCH v2 18/25] iotests: switch to AQMP

[Vladimir Sementsov-Ogievskiy]

15.12.2021 22:39, John Snow wrote:
> Simply import the type defition from the new location.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>


Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

> ---
>   tests/qemu-iotests/iotests.py | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
> index 83bfedb902..cb21aebe36 100644
> --- a/tests/qemu-iotests/iotests.py
> +++ b/tests/qemu-iotests/iotests.py
> @@ -37,7 +37,7 @@
>   from contextlib import contextmanager
>   
>   from qemu.machine import qtest
> -from qemu.qmp import QMPMessage
> +from qemu.aqmp.legacy import QMPMessage
>   
>   # Use this logger for logging messages directly from the iotests module
>   logger = logging.getLogger('qemu.iotests')
> 


-- 
Best regards,
Vladimir

Re: [PATCH v2 19/25] python: temporarily silence pylint duplicate-code warnings

[Vladimir Sementsov-Ogievskiy]

15.12.2021 22:39, John Snow wrote:
> The next several commits copy some code from qemu.qmp to qemu.aqmp, then
> delete qemu.qmp. In the interim, to prevent test failures, the duplicate
> code detection needs to be silenced to prevent bisect problems with CI
> testing.
> 
> Signed-off-by: John Snow<jsnow@redhat.com>


Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

-- 
Best regards,
Vladimir

Re: [PATCH v2 20/25] python/aqmp: take QMPBadPortError and parse_address from qemu.qmp

[Vladimir Sementsov-Ogievskiy]

15.12.2021 22:39, John Snow wrote:
> Shift these definitions over from the qmp package to the async qmp
> package.
> 
> Signed-off-by: John Snow<jsnow@redhat.com>

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

-- 
Best regards,
Vladimir

Re: [PATCH v2 21/25] python/aqmp: fully separate from qmp.QEMUMonitorProtocol

[Vladimir Sementsov-Ogievskiy]

15.12.2021 22:39, John Snow wrote:
> After this patch, qemu.aqmp.legacy.QEMUMonitorProtocol no longer
> inherits from qemu.qmp.QEMUMonitorProtocol. To do this, several
> inherited methods need to be explicitly re-defined.
> 
> Signed-off-by: John Snow<jsnow@redhat.com>


Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

-- 
Best regards,
Vladimir

Re: [PATCH v2 22/25] python/aqmp: copy qmp docstrings to qemu.aqmp.legacy

[Vladimir Sementsov-Ogievskiy]

15.12.2021 22:39, John Snow wrote:
> Copy the docstrings out of qemu.qmp, adjusting them as necessary to
> more accurately reflect the current state of this class.
> 
> Signed-off-by: John Snow<jsnow@redhat.com>



Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

-- 
Best regards,
Vladimir

Re: [PATCH v2 23/25] python: remove the old QMP package

[Vladimir Sementsov-Ogievskiy]

15.12.2021 22:39, John Snow wrote:
> Thank you for your service!
> 
> Signed-off-by: John Snow<jsnow@redhat.com>

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

-- 
Best regards,
Vladimir

Re: [PATCH v2 24/25] python: re-enable pylint duplicate-code warnings

[Vladimir Sementsov-Ogievskiy]

15.12.2021 22:39, John Snow wrote:
> With the old library gone, there's nothing duplicated in the tree, so
> the warning suppression can be removed.
> 
> Signed-off-by: John Snow<jsnow@redhat.com>

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

-- 
Best regards,
Vladimir

Re: [PATCH v2 25/25] python: rename qemu.aqmp to qemu.qmp

[Vladimir Sementsov-Ogievskiy]

15.12.2021 22:39, John Snow wrote:
> Now that we are fully switched over to the new QMP library, move it back
> over the old namespace. This is being done primarily so that we may
> upload this package simply as "qemu.qmp" without introducing confusion
> over whether or not "aqmp" is a new protocol or not.
> 
> The trade-off is increased confusion inside the QEMU developer
> tree. Sorry!
> 
> Signed-off-by: John Snow<jsnow@redhat.com>

Great job!

I looked thorough the patch, changes looks correct. Simply rename every aqmp / AQMP occurrence.. But:


[root@kvm review]# git grep -i aqmp
python/qemu/qmp/aqmp_tui.py:AQMP TUI
python/qemu/qmp/aqmp_tui.py:AQMP TUI is an asynchronous interface built on top the of the AQMP library.
python/qemu/qmp/aqmp_tui.py:Example Usage: aqmp-tui <SOCKET | TCP IP:PORT>
python/qemu/qmp/aqmp_tui.py:Full Usage: aqmp-tui --help
python/qemu/qmp/aqmp_tui.py:    Implements the AQMP TUI.
python/qemu/qmp/aqmp_tui.py:    parser = argparse.ArgumentParser(description='AQMP TUI')
python/qemu/qmp/legacy.py:        self._aqmp = QMPClient(nickname)
python/qemu/qmp/legacy.py:        if self._aqmp.greeting is not None:
python/qemu/qmp/legacy.py:            return self._aqmp.greeting._asdict()
python/qemu/qmp/legacy.py:        self._aqmp.await_greeting = negotiate
python/qemu/qmp/legacy.py:        self._aqmp.negotiate = negotiate
python/qemu/qmp/legacy.py:            self._aqmp.connect(self._address)
python/qemu/qmp/legacy.py:        self._aqmp.await_greeting = True
python/qemu/qmp/legacy.py:        self._aqmp.negotiate = True
python/qemu/qmp/legacy.py:            self._aqmp.accept(self._address),
python/qemu/qmp/legacy.py:                self._aqmp._raw(qmp_cmd, assign_id=False),
python/qemu/qmp/legacy.py:            self._aqmp.execute(cmd, kwds),
python/qemu/qmp/legacy.py:            if self._aqmp.events.empty():
python/qemu/qmp/legacy.py:                self._aqmp.events.get(),
python/qemu/qmp/legacy.py:        events = [dict(x) for x in self._aqmp.events.clear()]
python/qemu/qmp/legacy.py:        self._aqmp.events.clear()
python/qemu/qmp/legacy.py:            self._aqmp.disconnect()
python/qemu/qmp/legacy.py:        self._aqmp.send_fd_scm(fd)
python/qemu/qmp/legacy.py:        if self._aqmp.runstate == Runstate.IDLE:
python/setup.cfg:# AQMP TUI dependencies
python/setup.cfg:    aqmp-tui = qemu.qmp.aqmp_tui:main [tui]
python/setup.cfg:[mypy-qemu.qmp.aqmp_tui]

[root@kvm review]# git ls-tree -r --name-only HEAD | grep -i aqmp
python/qemu/qmp/aqmp_tui.py


I think, this all should be renamed too

-- 
Best regards,
Vladimir

Re: [PATCH v2 01/25] python/aqmp: add __del__ method to legacy interface

[Beraldo Leal]

On Wed, Dec 15, 2021 at 02:39:15PM -0500, John Snow wrote:
> asyncio can complain *very* loudly if you forget to back out of things
> gracefully before the garbage collector starts destroying objects that
> contain live references to asyncio Tasks.
> 
> The usual fix is just to remember to call aqmp.disconnect(), but for the
> sake of the legacy wrapper and quick, one-off scripts where a graceful
> shutdown is not necessarily of paramount imporance, add a courtesy
> cleanup that will trigger prior to seeing screenfuls of confusing
> asyncio tracebacks.
> 
> Note that we can't *always* save you from yourself; depending on when
> the GC runs, you might just seriously be out of luck. The best we can do
> in this case is to gently remind you to clean up after yourself.
> 
> (Still much better than multiple pages of incomprehensible python
> warnings for the crime of forgetting to put your toys away.)
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  python/qemu/aqmp/legacy.py | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/python/qemu/aqmp/legacy.py b/python/qemu/aqmp/legacy.py
> index 9e7b9fb80b..2ccb136b02 100644
> --- a/python/qemu/aqmp/legacy.py
> +++ b/python/qemu/aqmp/legacy.py
> @@ -16,6 +16,8 @@
>  import qemu.qmp
>  from qemu.qmp import QMPMessage, QMPReturnValue, SocketAddrT
>  
> +from .error import AQMPError
> +from .protocol import Runstate
>  from .qmp_client import QMPClient
>  
>  
> @@ -136,3 +138,19 @@ def settimeout(self, timeout: Optional[float]) -> None:
>  
>      def send_fd_scm(self, fd: int) -> None:
>          self._aqmp.send_fd_scm(fd)
> +
> +    def __del__(self) -> None:
> +        if self._aqmp.runstate == Runstate.IDLE:
> +            return
> +
> +        if not self._aloop.is_running():
> +            self.close()
> +        else:
> +            # Garbage collection ran while the event loop was running.
> +            # Nothing we can do about it now, but if we don't raise our
> +            # own error, the user will be treated to a lot of traceback
> +            # they might not understand.
> +            raise AQMPError(
> +                "QEMUMonitorProtocol.close()"
> +                " was not called before object was garbage collected"
> +            )

From the Python PoV, LGTM.

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

--
Beraldo

Re: [PATCH v2 02/25] python/aqmp: handle asyncio.TimeoutError on execute()

[Beraldo Leal]

On Wed, Dec 15, 2021 at 02:39:16PM -0500, John Snow wrote:
> This exception can be injected into any await statement. If we are
> canceled via timeout, we want to clear the pending execution record on
> our way out.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  python/qemu/aqmp/qmp_client.py | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/python/qemu/aqmp/qmp_client.py b/python/qemu/aqmp/qmp_client.py
> index 8105e29fa8..6a985ffe30 100644
> --- a/python/qemu/aqmp/qmp_client.py
> +++ b/python/qemu/aqmp/qmp_client.py
> @@ -435,7 +435,11 @@ async def _issue(self, msg: Message) -> Union[None, str]:
>              msg_id = msg['id']
>  
>          self._pending[msg_id] = asyncio.Queue(maxsize=1)
> -        await self._outgoing.put(msg)
> +        try:
> +            await self._outgoing.put(msg)
> +        except:
> +            del self._pending[msg_id]
> +            raise

At first glance both, except and raise are not good practices, but I
think I got what you are doing here, just intercepting to delete the
pending message and raising it again. So maybe it will be safe to close
the eyes here. ;)

>  
>          return msg_id
>  
> @@ -452,9 +456,9 @@ async def _reply(self, msg_id: Union[str, None]) -> Message:
>              was lost, or some other problem.
>          """
>          queue = self._pending[msg_id]
> -        result = await queue.get()
>  
>          try:
> +            result = await queue.get()
>              if isinstance(result, ExecInterruptedError):
>                  raise result
>              return result

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

--
Beraldo

Re: [PATCH v2 04/25] python/aqmp: add SocketAddrT to package root

[Beraldo Leal]

On Wed, Dec 15, 2021 at 02:39:18PM -0500, John Snow wrote:
> It's a commonly needed definition, it can be re-exported by the root.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  python/qemu/aqmp/__init__.py | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/python/qemu/aqmp/__init__.py b/python/qemu/aqmp/__init__.py
> index 880d5b6fa7..c6fa2dda58 100644
> --- a/python/qemu/aqmp/__init__.py
> +++ b/python/qemu/aqmp/__init__.py
> @@ -26,7 +26,12 @@
>  from .error import AQMPError
>  from .events import EventListener
>  from .message import Message
> -from .protocol import ConnectError, Runstate, StateError
> +from .protocol import (
> +    ConnectError,
> +    Runstate,
> +    SocketAddrT,
> +    StateError,
> +)
>  from .qmp_client import ExecInterruptedError, ExecuteError, QMPClient
>  
>  
> @@ -48,4 +53,7 @@
>      'ConnectError',
>      'ExecuteError',
>      'ExecInterruptedError',
> +
> +    # Type aliases
> +    'SocketAddrT',
>  )

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

--
Beraldo

Re: [PATCH v2 07/25] python/qmp: switch qemu-ga-client to AQMP

[Beraldo Leal]

On Wed, Dec 15, 2021 at 02:39:21PM -0500, John Snow wrote:
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  python/qemu/qmp/qemu_ga_client.py | 22 +++++++++++-----------
>  1 file changed, 11 insertions(+), 11 deletions(-)
> 
> diff --git a/python/qemu/qmp/qemu_ga_client.py b/python/qemu/qmp/qemu_ga_client.py
> index b3e1d98c9e..15ed430c61 100644
> --- a/python/qemu/qmp/qemu_ga_client.py
> +++ b/python/qemu/qmp/qemu_ga_client.py
> @@ -37,8 +37,8 @@
>  # the COPYING file in the top-level directory.
>  
>  import argparse
> +import asyncio
>  import base64
> -import errno
>  import os
>  import random
>  import sys
> @@ -50,8 +50,8 @@
>      Sequence,
>  )
>  
> -from qemu import qmp
> -from qemu.qmp import SocketAddrT
> +from qemu.aqmp import ConnectError, SocketAddrT
> +from qemu.aqmp.legacy import QEMUMonitorProtocol
>  
>  
>  # This script has not seen many patches or careful attention in quite
> @@ -61,7 +61,7 @@
>  # pylint: disable=missing-docstring
>  
>  
> -class QemuGuestAgent(qmp.QEMUMonitorProtocol):
> +class QemuGuestAgent(QEMUMonitorProtocol):
>      def __getattr__(self, name: str) -> Callable[..., Any]:
>          def wrapper(**kwds: object) -> object:
>              return self.command('guest-' + name.replace('_', '-'), **kwds)
> @@ -149,7 +149,7 @@ def ping(self, timeout: Optional[float]) -> bool:
>          self.qga.settimeout(timeout)
>          try:
>              self.qga.ping()
> -        except TimeoutError:
> +        except asyncio.TimeoutError:
>              return False
>          return True
>  
> @@ -172,7 +172,7 @@ def suspend(self, mode: str) -> None:
>          try:
>              getattr(self.qga, 'suspend' + '_' + mode)()
>              # On error exception will raise
> -        except TimeoutError:
> +        except asyncio.TimeoutError:
>              # On success command will timed out
>              return
>  
> @@ -182,7 +182,7 @@ def shutdown(self, mode: str = 'powerdown') -> None:
>  
>          try:
>              self.qga.shutdown(mode=mode)
> -        except TimeoutError:
> +        except asyncio.TimeoutError:
>              pass
>  
>  
> @@ -277,7 +277,7 @@ def _cmd_reboot(client: QemuGuestAgentClient, args: Sequence[str]) -> None:
>  
>  def send_command(address: str, cmd: str, args: Sequence[str]) -> None:
>      if not os.path.exists(address):
> -        print('%s not found' % address)
> +        print(f"'{address}' not found. (Is QEMU running?)")
>          sys.exit(1)
>  
>      if cmd not in commands:
> @@ -287,10 +287,10 @@ def send_command(address: str, cmd: str, args: Sequence[str]) -> None:
>  
>      try:
>          client = QemuGuestAgentClient(address)
> -    except OSError as err:
> +    except ConnectError as err:
>          print(err)
> -        if err.errno == errno.ECONNREFUSED:
> -            print('Hint: qemu is not running?')
> +        if isinstance(err.exc, ConnectionError):
> +            print('(Is QEMU running?)')
>          sys.exit(1)
>  
>      if cmd == 'fsfreeze' and args[0] == 'freeze':

From the Python PoV, LGTM:

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

--
Beraldo

Re: [PATCH v2 08/25] python/qmp: switch qom tools to AQMP

[Beraldo Leal]

On Wed, Dec 15, 2021 at 02:39:22PM -0500, John Snow wrote:
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  python/qemu/qmp/qom.py        |  5 +++--
>  python/qemu/qmp/qom_common.py |  3 ++-
>  python/qemu/qmp/qom_fuse.py   | 11 ++++++-----
>  3 files changed, 11 insertions(+), 8 deletions(-)
> 
> diff --git a/python/qemu/qmp/qom.py b/python/qemu/qmp/qom.py
> index 8ff28a8343..bb5d1a78f5 100644
> --- a/python/qemu/qmp/qom.py
> +++ b/python/qemu/qmp/qom.py
> @@ -32,7 +32,8 @@
>  
>  import argparse
>  
> -from . import QMPResponseError
> +from qemu.aqmp import ExecuteError
> +
>  from .qom_common import QOMCommand
>  
>  
> @@ -233,7 +234,7 @@ def _list_node(self, path: str) -> None:
>                  rsp = self.qmp.command('qom-get', path=path,
>                                         property=item.name)
>                  print(f"  {item.name}: {rsp} ({item.type})")
> -            except QMPResponseError as err:
> +            except ExecuteError as err:
>                  print(f"  {item.name}: <EXCEPTION: {err!s}> ({item.type})")
>          print('')
>          for item in items:
> diff --git a/python/qemu/qmp/qom_common.py b/python/qemu/qmp/qom_common.py
> index a59ae1a2a1..6f07451dfa 100644
> --- a/python/qemu/qmp/qom_common.py
> +++ b/python/qemu/qmp/qom_common.py
> @@ -27,7 +27,8 @@
>      TypeVar,
>  )
>  
> -from . import QEMUMonitorProtocol, QMPError
> +from qemu.aqmp import QMPError
> +from qemu.aqmp.legacy import QEMUMonitorProtocol
>  
>  
>  # The following is needed only for a type alias.
> diff --git a/python/qemu/qmp/qom_fuse.py b/python/qemu/qmp/qom_fuse.py
> index 43f4671fdb..653a76b93b 100644
> --- a/python/qemu/qmp/qom_fuse.py
> +++ b/python/qemu/qmp/qom_fuse.py
> @@ -48,7 +48,8 @@
>  import fuse
>  from fuse import FUSE, FuseOSError, Operations
>  
> -from . import QMPResponseError
> +from qemu.aqmp import ExecuteError
> +
>  from .qom_common import QOMCommand
>  
>  
> @@ -99,7 +100,7 @@ def is_object(self, path: str) -> bool:
>          try:
>              self.qom_list(path)
>              return True
> -        except QMPResponseError:
> +        except ExecuteError:
>              return False
>  
>      def is_property(self, path: str) -> bool:
> @@ -112,7 +113,7 @@ def is_property(self, path: str) -> bool:
>                  if item.name == prop:
>                      return True
>              return False
> -        except QMPResponseError:
> +        except ExecuteError:
>              return False
>  
>      def is_link(self, path: str) -> bool:
> @@ -125,7 +126,7 @@ def is_link(self, path: str) -> bool:
>                  if item.name == prop and item.link:
>                      return True
>              return False
> -        except QMPResponseError:
> +        except ExecuteError:
>              return False
>  
>      def read(self, path: str, size: int, offset: int, fh: IO[bytes]) -> bytes:
> @@ -138,7 +139,7 @@ def read(self, path: str, size: int, offset: int, fh: IO[bytes]) -> bytes:
>          try:
>              data = str(self.qmp.command('qom-get', path=path, property=prop))
>              data += '\n'  # make values shell friendly
> -        except QMPResponseError as err:
> +        except ExecuteError as err:
>              raise FuseOSError(EPERM) from err
>  
>          if offset > len(data):

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

--
Beraldo

Re: [PATCH v2 11/25] python: move qmp-shell under the AQMP package

[Beraldo Leal]

On Wed, Dec 15, 2021 at 02:39:25PM -0500, John Snow wrote:
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  python/README.rst                      | 2 +-
>  python/qemu/{qmp => aqmp}/qmp_shell.py | 0
>  python/setup.cfg                       | 2 +-
>  scripts/qmp/qmp-shell                  | 2 +-
>  4 files changed, 3 insertions(+), 3 deletions(-)
>  rename python/qemu/{qmp => aqmp}/qmp_shell.py (100%)
> 
> diff --git a/python/README.rst b/python/README.rst
> index 9c1fceaee7..fcf74f69ea 100644
> --- a/python/README.rst
> +++ b/python/README.rst
> @@ -59,7 +59,7 @@ Package installation also normally provides executable console scripts,
>  so that tools like ``qmp-shell`` are always available via $PATH. To
>  invoke them without installation, you can invoke e.g.:
>  
> -``> PYTHONPATH=~/src/qemu/python python3 -m qemu.qmp.qmp_shell``
> +``> PYTHONPATH=~/src/qemu/python python3 -m qemu.aqmp.qmp_shell``
>  
>  The mappings between console script name and python module path can be
>  found in ``setup.cfg``.
> diff --git a/python/qemu/qmp/qmp_shell.py b/python/qemu/aqmp/qmp_shell.py
> similarity index 100%
> rename from python/qemu/qmp/qmp_shell.py
> rename to python/qemu/aqmp/qmp_shell.py
> diff --git a/python/setup.cfg b/python/setup.cfg
> index 78421411d2..168a79c867 100644
> --- a/python/setup.cfg
> +++ b/python/setup.cfg
> @@ -67,7 +67,7 @@ console_scripts =
>      qom-tree = qemu.utils.qom:QOMTree.entry_point
>      qom-fuse = qemu.utils.qom_fuse:QOMFuse.entry_point [fuse]
>      qemu-ga-client = qemu.utils.qemu_ga_client:main
> -    qmp-shell = qemu.qmp.qmp_shell:main
> +    qmp-shell = qemu.aqmp.qmp_shell:main
>      aqmp-tui = qemu.aqmp.aqmp_tui:main [tui]
>  
>  [flake8]
> diff --git a/scripts/qmp/qmp-shell b/scripts/qmp/qmp-shell
> index 4a20f97db7..31b19d73e2 100755
> --- a/scripts/qmp/qmp-shell
> +++ b/scripts/qmp/qmp-shell
> @@ -4,7 +4,7 @@ import os
>  import sys
>  
>  sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..', 'python'))
> -from qemu.qmp import qmp_shell
> +from qemu.aqmp import qmp_shell
>  
>  
>  if __name__ == '__main__':

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

--
Beraldo

Re: [PATCH v2 12/25] python/machine: permanently switch to AQMP

[Beraldo Leal]

On Wed, Dec 15, 2021 at 02:39:26PM -0500, John Snow wrote:
> Remove the QEMU_PYTHON_LEGACY_QMP environment variable, making the
> switch permanent. Update Exceptions and import paths as necessary.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  python/qemu/machine/machine.py | 18 +++++++-----------
>  python/qemu/machine/qtest.py   |  2 +-
>  2 files changed, 8 insertions(+), 12 deletions(-)
> 
> diff --git a/python/qemu/machine/machine.py b/python/qemu/machine/machine.py
> index 67ab06ca2b..21fb4a4f30 100644
> --- a/python/qemu/machine/machine.py
> +++ b/python/qemu/machine/machine.py
> @@ -40,21 +40,16 @@
>      TypeVar,
>  )
>  
> -from qemu.qmp import (  # pylint: disable=import-error
> +from qemu.aqmp import SocketAddrT
> +from qemu.aqmp.legacy import (
> +    QEMUMonitorProtocol,
>      QMPMessage,
>      QMPReturnValue,
> -    SocketAddrT,
>  )
>  
>  from . import console_socket
>  
>  
> -if os.environ.get('QEMU_PYTHON_LEGACY_QMP'):
> -    from qemu.qmp import QEMUMonitorProtocol
> -else:
> -    from qemu.aqmp.legacy import QEMUMonitorProtocol
> -
> -
>  LOG = logging.getLogger(__name__)
>  
>  
> @@ -710,8 +705,9 @@ def events_wait(self,
>          :param timeout: Optional timeout, in seconds.
>                          See QEMUMonitorProtocol.pull_event.
>  
> -        :raise QMPTimeoutError: If timeout was non-zero and no matching events
> -                                were found.
> +        :raise asyncio.TimeoutError:
> +            If timeout was non-zero and no matching events were found.
> +
>          :return: A QMP event matching the filter criteria.
>                   If timeout was 0 and no event matched, None.
>          """
> @@ -734,7 +730,7 @@ def _match(event: QMPMessage) -> bool:
>              event = self._qmp.pull_event(wait=timeout)
>              if event is None:
>                  # NB: None is only returned when timeout is false-ish.
> -                # Timeouts raise QMPTimeoutError instead!
> +                # Timeouts raise asyncio.TimeoutError instead!
>                  break
>              if _match(event):
>                  return event
> diff --git a/python/qemu/machine/qtest.py b/python/qemu/machine/qtest.py
> index f2f9aaa5e5..817c8a5425 100644
> --- a/python/qemu/machine/qtest.py
> +++ b/python/qemu/machine/qtest.py
> @@ -26,7 +26,7 @@
>      TextIO,
>  )
>  
> -from qemu.qmp import SocketAddrT  # pylint: disable=import-error
> +from qemu.aqmp.protocol import SocketAddrT
>  
>  from .machine import QEMUMachine
>  

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

--
Beraldo

Re: [PATCH v2 14/25] scripts/cpu-x86-uarch-abi: switch to AQMP

[Beraldo Leal]

On Wed, Dec 15, 2021 at 02:39:28PM -0500, John Snow wrote:
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  scripts/cpu-x86-uarch-abi.py | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/scripts/cpu-x86-uarch-abi.py b/scripts/cpu-x86-uarch-abi.py
> index 8963d90f0b..c262d2f027 100644
> --- a/scripts/cpu-x86-uarch-abi.py
> +++ b/scripts/cpu-x86-uarch-abi.py
> @@ -6,7 +6,7 @@
>  # compatibility levels for each CPU model.
>  #
>  
> -from qemu import qmp
> +from qemu.aqmp.legacy import QEMUMonitorProtocol
>  import sys
>  
>  if len(sys.argv) != 2:
> @@ -66,7 +66,7 @@
>  
>  
>  sock = sys.argv[1]
> -shell = qmp.QEMUMonitorProtocol(sock)
> +shell = QEMUMonitorProtocol(sock)
>  shell.connect()
>  
>  models = shell.cmd("query-cpu-definitions")

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

--
Beraldo

Re: [PATCH v2 15/25] scripts/render-block-graph: switch to AQMP

[Beraldo Leal]

On Wed, Dec 15, 2021 at 02:39:29PM -0500, John Snow wrote:
> Creating an instance of qemu.aqmp.ExecuteError is too involved here, so
> just drop the specificity down to a generic AQMPError.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  scripts/render_block_graph.py | 8 +++-----
>  1 file changed, 3 insertions(+), 5 deletions(-)
> 
> diff --git a/scripts/render_block_graph.py b/scripts/render_block_graph.py
> index da6acf050d..97778927f3 100755
> --- a/scripts/render_block_graph.py
> +++ b/scripts/render_block_graph.py
> @@ -25,10 +25,8 @@
>  from graphviz import Digraph
>  
>  sys.path.append(os.path.join(os.path.dirname(__file__), '..', 'python'))
> -from qemu.qmp import (
> -    QEMUMonitorProtocol,
> -    QMPResponseError,
> -)
> +from qemu.aqmp import QMPError
> +from qemu.aqmp.legacy import QEMUMonitorProtocol
>  
>  
>  def perm(arr):
> @@ -105,7 +103,7 @@ def command(self, cmd):
>          reply = json.loads(subprocess.check_output(ar))
>  
>          if 'error' in reply:
> -            raise QMPResponseError(reply)
> +            raise QMPError(reply)
>  
>          return reply['return']

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

--
Beraldo

Re: [PATCH v2 16/25] scripts/bench-block-job: switch to AQMP

[Beraldo Leal]

On Wed, Dec 15, 2021 at 02:39:30PM -0500, John Snow wrote:
> For this commit, we only need to remove accommodations for the
> synchronous QMP library.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  scripts/simplebench/bench_block_job.py | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/scripts/simplebench/bench_block_job.py b/scripts/simplebench/bench_block_job.py
> index a403c35b08..af9d1646a4 100755
> --- a/scripts/simplebench/bench_block_job.py
> +++ b/scripts/simplebench/bench_block_job.py
> @@ -27,7 +27,6 @@
>  
>  sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..', 'python'))
>  from qemu.machine import QEMUMachine
> -from qemu.qmp import QMPConnectError
>  from qemu.aqmp import ConnectError
>  
>  
> @@ -50,7 +49,7 @@ def bench_block_job(cmd, cmd_args, qemu_args):
>          vm.launch()
>      except OSError as e:
>          return {'error': 'popen failed: ' + str(e)}
> -    except (QMPConnectError, ConnectError, socket.timeout):
> +    except (ConnectError, socket.timeout):
>          return {'error': 'qemu failed: ' + str(vm.get_log())}
>  
>      try:

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

--
Beraldo

Re: [PATCH v2 17/25] iotests/mirror-top-perms: switch to AQMP

[Beraldo Leal]

On Wed, Dec 15, 2021 at 02:39:31PM -0500, John Snow wrote:
> Signed-off-by: John Snow <jsnow@redhat.com>
> 
> ---
> 
> Note: I still need to adjust the logging. The problem now is that the
> logging messages include the PID of the test process, so they need to be
> filtered out. I'll investigate that for a follow-up, or for v2.
> 
> I could just add yet another filtering function somewhere, but I think
> it's getting out of hand with how many filters and loggers there are, so
> I want to give it a slightly more serious treatment instead of a
> hackjob.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  tests/qemu-iotests/tests/mirror-top-perms | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/tests/qemu-iotests/tests/mirror-top-perms b/tests/qemu-iotests/tests/mirror-top-perms
> index 0a51a613f3..f394931a00 100755
> --- a/tests/qemu-iotests/tests/mirror-top-perms
> +++ b/tests/qemu-iotests/tests/mirror-top-perms
> @@ -23,7 +23,6 @@ import os
>  
>  from qemu.aqmp import ConnectError
>  from qemu.machine import machine
> -from qemu.qmp import QMPConnectError
>  
>  import iotests
>  from iotests import change_log_level, qemu_img
> @@ -101,13 +100,13 @@ class TestMirrorTopPerms(iotests.QMPTestCase):
>          self.vm_b.add_device('virtio-blk,drive=drive0,share-rw=on')
>          try:
>              # Silence AQMP errors temporarily.
> -            # TODO: Remove this and just allow the errors to be logged when
> -            # AQMP fully replaces QMP.
> +            # TODO: Remove change_log_level and allow the errors to be logged.
> +            #       This necessitates a PID filter on *all* logging output.
>              with change_log_level('qemu.aqmp'):
>                  self.vm_b.launch()
>                  print('ERROR: VM B launched successfully, '
>                        'this should not have happened')
> -        except (QMPConnectError, ConnectError):
> +        except ConnectError:
>              assert 'Is another process using the image' in self.vm_b.get_log()
>  
>          result = self.vm.qmp('block-job-cancel',

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

--
Beraldo

Re: [PATCH v2 18/25] iotests: switch to AQMP

[Beraldo Leal]

On Wed, Dec 15, 2021 at 02:39:32PM -0500, John Snow wrote:
> Simply import the type defition from the new location.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  tests/qemu-iotests/iotests.py | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
> index 83bfedb902..cb21aebe36 100644
> --- a/tests/qemu-iotests/iotests.py
> +++ b/tests/qemu-iotests/iotests.py
> @@ -37,7 +37,7 @@
>  from contextlib import contextmanager
>  
>  from qemu.machine import qtest
> -from qemu.qmp import QMPMessage
> +from qemu.aqmp.legacy import QMPMessage
>  
>  # Use this logger for logging messages directly from the iotests module
>  logger = logging.getLogger('qemu.iotests')

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

--
Beraldo

Re: [PATCH v2 20/25] python/aqmp: take QMPBadPortError and parse_address from qemu.qmp

[Beraldo Leal]

On Wed, Dec 15, 2021 at 02:39:34PM -0500, John Snow wrote:
> Shift these definitions over from the qmp package to the async qmp
> package.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  python/qemu/aqmp/aqmp_tui.py |  2 +-
>  python/qemu/aqmp/legacy.py   | 30 ++++++++++++++++++++++++++----
>  python/qemu/qmp/__init__.py  | 26 --------------------------
>  3 files changed, 27 insertions(+), 31 deletions(-)
> 
> diff --git a/python/qemu/aqmp/aqmp_tui.py b/python/qemu/aqmp/aqmp_tui.py
> index a2929f771c..184a3e4690 100644
> --- a/python/qemu/aqmp/aqmp_tui.py
> +++ b/python/qemu/aqmp/aqmp_tui.py
> @@ -35,8 +35,8 @@
>  import urwid
>  import urwid_readline
>  
> -from ..qmp import QEMUMonitorProtocol, QMPBadPortError
>  from .error import ProtocolError
> +from .legacy import QEMUMonitorProtocol, QMPBadPortError
>  from .message import DeserializationError, Message, UnexpectedTypeError
>  from .protocol import ConnectError, Runstate
>  from .qmp_client import ExecInterruptedError, QMPClient
> diff --git a/python/qemu/aqmp/legacy.py b/python/qemu/aqmp/legacy.py
> index 0890f95b16..76b09671cc 100644
> --- a/python/qemu/aqmp/legacy.py
> +++ b/python/qemu/aqmp/legacy.py
> @@ -22,9 +22,6 @@
>  from .qmp_client import QMPClient
>  
>  
> -# (Temporarily) Re-export QMPBadPortError
> -QMPBadPortError = qemu.qmp.QMPBadPortError
> -
>  #: QMPMessage is an entire QMP message of any kind.
>  QMPMessage = Dict[str, Any]
>  
> @@ -45,6 +42,12 @@
>  # pylint: disable=missing-docstring
>  
>  
> +class QMPBadPortError(QMPError):
> +    """
> +    Unable to parse socket address: Port was non-numerical.
> +    """
> +
> +
>  class QEMUMonitorProtocol(qemu.qmp.QEMUMonitorProtocol):
>      def __init__(self, address: SocketAddrT,
>                   server: bool = False,
> @@ -72,7 +75,26 @@ def _get_greeting(self) -> Optional[QMPMessage]:
>          return None
>  
>      # __enter__ and __exit__ need no changes
> -    # parse_address needs no changes
> +
> +    @classmethod
> +    def parse_address(cls, address: str) -> SocketAddrT:
> +        """
> +        Parse a string into a QMP address.
> +
> +        Figure out if the argument is in the port:host form.
> +        If it's not, it's probably a file path.
> +        """
> +        components = address.split(':')
> +        if len(components) == 2:
> +            try:
> +                port = int(components[1])
> +            except ValueError:
> +                msg = f"Bad port: '{components[1]}' in '{address}'."
> +                raise QMPBadPortError(msg) from None
> +            return (components[0], port)
> +
> +        # Treat as filepath.
> +        return address
>  
>      def connect(self, negotiate: bool = True) -> Optional[QMPMessage]:
>          self._aqmp.await_greeting = negotiate
> diff --git a/python/qemu/qmp/__init__.py b/python/qemu/qmp/__init__.py
> index 358c0971d0..4e08641154 100644
> --- a/python/qemu/qmp/__init__.py
> +++ b/python/qemu/qmp/__init__.py
> @@ -102,12 +102,6 @@ def __init__(self, reply: QMPMessage):
>          self.reply = reply
>  
>  
> -class QMPBadPortError(QMPError):
> -    """
> -    Unable to parse socket address: Port was non-numerical.
> -    """
> -
> -
>  class QEMUMonitorProtocol:
>      """
>      Provide an API to connect to QEMU via QEMU Monitor Protocol (QMP) and then
> @@ -237,26 +231,6 @@ def __exit__(self,
>          # Implement context manager exit function.
>          self.close()
>  
> -    @classmethod
> -    def parse_address(cls, address: str) -> SocketAddrT:
> -        """
> -        Parse a string into a QMP address.
> -
> -        Figure out if the argument is in the port:host form.
> -        If it's not, it's probably a file path.
> -        """
> -        components = address.split(':')
> -        if len(components) == 2:
> -            try:
> -                port = int(components[1])
> -            except ValueError:
> -                msg = f"Bad port: '{components[1]}' in '{address}'."
> -                raise QMPBadPortError(msg) from None
> -            return (components[0], port)
> -
> -        # Treat as filepath.
> -        return address
> -
>      def connect(self, negotiate: bool = True) -> Optional[QMPMessage]:
>          """
>          Connect to the QMP Monitor and perform capabilities negotiation.

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

--
Beraldo

Re: [PATCH v2 21/25] python/aqmp: fully separate from qmp.QEMUMonitorProtocol

[Beraldo Leal]

On Wed, Dec 15, 2021 at 02:39:35PM -0500, John Snow wrote:
> After this patch, qemu.aqmp.legacy.QEMUMonitorProtocol no longer
> inherits from qemu.qmp.QEMUMonitorProtocol. To do this, several
> inherited methods need to be explicitly re-defined.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  python/qemu/aqmp/legacy.py | 38 ++++++++++++++++++++++++++++++++------
>  1 file changed, 32 insertions(+), 6 deletions(-)
> 
> diff --git a/python/qemu/aqmp/legacy.py b/python/qemu/aqmp/legacy.py
> index 76b09671cc..8f38e7d912 100644
> --- a/python/qemu/aqmp/legacy.py
> +++ b/python/qemu/aqmp/legacy.py
> @@ -5,18 +5,18 @@
>  """
>  
>  import asyncio
> +from types import TracebackType
>  from typing import (
>      Any,
>      Awaitable,
>      Dict,
>      List,
>      Optional,
> +    Type,
>      TypeVar,
>      Union,
>  )
>  
> -import qemu.qmp
> -
>  from .error import QMPError
>  from .protocol import Runstate, SocketAddrT
>  from .qmp_client import QMPClient
> @@ -48,9 +48,9 @@ class QMPBadPortError(QMPError):
>      """
>  
>  
> -class QEMUMonitorProtocol(qemu.qmp.QEMUMonitorProtocol):
> +class QEMUMonitorProtocol:
>      def __init__(self, address: SocketAddrT,
> -                 server: bool = False,
> +                 server: bool = False,  # pylint: disable=unused-argument
>                   nickname: Optional[str] = None):
>  
>          # pylint: disable=super-init-not-called
> @@ -74,7 +74,18 @@ def _get_greeting(self) -> Optional[QMPMessage]:
>              return self._aqmp.greeting._asdict()
>          return None
>  
> -    # __enter__ and __exit__ need no changes
> +    def __enter__(self: _T) -> _T:
> +        # Implement context manager enter function.
> +        return self
> +
> +    def __exit__(self,
> +                 # pylint: disable=duplicate-code
> +                 # see https://github.com/PyCQA/pylint/issues/3619
> +                 exc_type: Optional[Type[BaseException]],
> +                 exc_val: Optional[BaseException],
> +                 exc_tb: Optional[TracebackType]) -> None:
> +        # Implement context manager exit function.
> +        self.close()
>  
>      @classmethod
>      def parse_address(cls, address: str) -> SocketAddrT:
> @@ -131,7 +142,22 @@ def cmd_obj(self, qmp_cmd: QMPMessage) -> QMPMessage:
>              )
>          )
>  
> -    # Default impl of cmd() delegates to cmd_obj
> +    def cmd(self, name: str,
> +            args: Optional[Dict[str, object]] = None,
> +            cmd_id: Optional[object] = None) -> QMPMessage:
> +        """
> +        Build a QMP command and send it to the QMP Monitor.
> +
> +        @param name: command name (string)
> +        @param args: command arguments (dict)
> +        @param cmd_id: command id (dict, list, string or int)
> +        """
> +        qmp_cmd: QMPMessage = {'execute': name}
> +        if args:
> +            qmp_cmd['arguments'] = args
> +        if cmd_id:
> +            qmp_cmd['id'] = cmd_id
> +        return self.cmd_obj(qmp_cmd)
>  
>      def command(self, cmd: str, **kwds: object) -> QMPReturnValue:
>          return self._sync(

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

--
Beraldo

Re: [PATCH v2 22/25] python/aqmp: copy qmp docstrings to qemu.aqmp.legacy

[Beraldo Leal]

On Wed, Dec 15, 2021 at 02:39:36PM -0500, John Snow wrote:
> Copy the docstrings out of qemu.qmp, adjusting them as necessary to
> more accurately reflect the current state of this class.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  python/qemu/aqmp/legacy.py | 110 ++++++++++++++++++++++++++++++++++---
>  1 file changed, 102 insertions(+), 8 deletions(-)
> 
> diff --git a/python/qemu/aqmp/legacy.py b/python/qemu/aqmp/legacy.py
> index 8f38e7d912..6c250cd46a 100644
> --- a/python/qemu/aqmp/legacy.py
> +++ b/python/qemu/aqmp/legacy.py
> @@ -1,9 +1,23 @@
>  """
> -Sync QMP Wrapper
> +(Legacy) Sync QMP Wrapper
>  
> -This class pretends to be qemu.qmp.QEMUMonitorProtocol.
> +This module provides the `QEMUMonitorProtocol` class, which is a
> +synchronous wrapper around `QMPClient`.
> +
> +Its design closely resembles that of the original QEMUMonitorProtocol
> +class, originally written by Luiz Capitulino.
>  """
>  
> +# Copyright (C) 2009, 2010, 2021 Red Hat Inc.
> +#
> +# Authors:
> +#  Luiz Capitulino <lcapitulino@redhat.com>
> +#  John Snow <jsnow@redhat.com>
> +#
> +# This work is licensed under the terms of the GNU GPL, version 2.
> +# See the COPYING file in the top-level directory.
> +
> +
>  import asyncio
>  from types import TracebackType
>  from typing import (
> @@ -39,9 +53,6 @@
>  # {} is the QMPReturnValue.
>  
>  
> -# pylint: disable=missing-docstring
> -
> -
>  class QMPBadPortError(QMPError):
>      """
>      Unable to parse socket address: Port was non-numerical.
> @@ -49,6 +60,21 @@ class QMPBadPortError(QMPError):
>  
>  
>  class QEMUMonitorProtocol:
> +    """
> +    Provide an API to connect to QEMU via QEMU Monitor Protocol (QMP)
> +    and then allow to handle commands and events.
> +
> +    :param address:  QEMU address, can be either a unix socket path (string)
> +                     or a tuple in the form ( address, port ) for a TCP
> +                     connection
> +    :param server:   Deprecated, ignored. (See 'accept')
> +    :param nickname: Optional nickname used for logging.
> +
> +    ..note::
> +        No connection is established during `__init__`, this is done by
> +        the `connect()` or `accept()` methods.
> +    """
> +
>      def __init__(self, address: SocketAddrT,
>                   server: bool = False,  # pylint: disable=unused-argument
>                   nickname: Optional[str] = None):
> @@ -108,6 +134,12 @@ def parse_address(cls, address: str) -> SocketAddrT:
>          return address
>  
>      def connect(self, negotiate: bool = True) -> Optional[QMPMessage]:
> +        """
> +        Connect to the QMP Monitor and perform capabilities negotiation.
> +
> +        :return: QMP greeting dict, or None if negotiate is false
> +        :raise ConnectError: on connection errors
> +        """
>          self._aqmp.await_greeting = negotiate
>          self._aqmp.negotiate = negotiate
>  
> @@ -117,6 +149,16 @@ def connect(self, negotiate: bool = True) -> Optional[QMPMessage]:
>          return self._get_greeting()
>  
>      def accept(self, timeout: Optional[float] = 15.0) -> QMPMessage:
> +        """
> +        Await connection from QMP Monitor and perform capabilities negotiation.
> +
> +        :param timeout:
> +            timeout in seconds (nonnegative float number, or None).
> +            If None, there is no timeout, and this may block forever.
> +
> +        :return: QMP greeting dict
> +        :raise ConnectError: on connection errors
> +        """
>          self._aqmp.await_greeting = True
>          self._aqmp.negotiate = True
>  
> @@ -130,6 +172,12 @@ def accept(self, timeout: Optional[float] = 15.0) -> QMPMessage:
>          return ret
>  
>      def cmd_obj(self, qmp_cmd: QMPMessage) -> QMPMessage:
> +        """
> +        Send a QMP command to the QMP Monitor.
> +
> +        :param qmp_cmd: QMP command to be sent as a Python dict
> +        :return: QMP response as a Python dict
> +        """
>          return dict(
>              self._sync(
>                  # pylint: disable=protected-access
> @@ -148,9 +196,9 @@ def cmd(self, name: str,
>          """
>          Build a QMP command and send it to the QMP Monitor.
>  
> -        @param name: command name (string)
> -        @param args: command arguments (dict)
> -        @param cmd_id: command id (dict, list, string or int)
> +        :param name: command name (string)
> +        :param args: command arguments (dict)
> +        :param cmd_id: command id (dict, list, string or int)
>          """
>          qmp_cmd: QMPMessage = {'execute': name}
>          if args:
> @@ -160,6 +208,9 @@ def cmd(self, name: str,
>          return self.cmd_obj(qmp_cmd)
>  
>      def command(self, cmd: str, **kwds: object) -> QMPReturnValue:
> +        """
> +        Build and send a QMP command to the monitor, report errors if any
> +        """
>          return self._sync(
>              self._aqmp.execute(cmd, kwds),
>              self._timeout
> @@ -167,6 +218,19 @@ def command(self, cmd: str, **kwds: object) -> QMPReturnValue:
>  
>      def pull_event(self,
>                     wait: Union[bool, float] = False) -> Optional[QMPMessage]:
> +        """
> +        Pulls a single event.
> +
> +        :param wait:
> +            If False or 0, do not wait. Return None if no events ready.
> +            If True, wait forever until the next event.
> +            Otherwise, wait for the specified number of seconds.
> +
> +        :raise asyncio.TimeoutError:
> +            When a timeout is requested and the timeout period elapses.
> +
> +        :return: The first available QMP event, or None.
> +        """
>          if not wait:
>              # wait is False/0: "do not wait, do not except."
>              if self._aqmp.events.empty():
> @@ -187,6 +251,20 @@ def pull_event(self,
>          )
>  
>      def get_events(self, wait: Union[bool, float] = False) -> List[QMPMessage]:
> +        """
> +        Get a list of QMP events and clear all pending events.
> +
> +        :param wait:
> +            If False or 0, do not wait. Return None if no events ready.
> +            If True, wait until we have at least one event.
> +            Otherwise, wait for up to the specified number of seconds for at
> +            least one event.
> +
> +        :raise asyncio.TimeoutError:
> +            When a timeout is requested and the timeout period elapses.
> +
> +        :return: A list of QMP events.
> +        """
>          events = [dict(x) for x in self._aqmp.events.clear()]
>          if events:
>              return events
> @@ -195,17 +273,33 @@ def get_events(self, wait: Union[bool, float] = False) -> List[QMPMessage]:
>          return [event] if event is not None else []
>  
>      def clear_events(self) -> None:
> +        """Clear current list of pending events."""
>          self._aqmp.events.clear()
>  
>      def close(self) -> None:
> +        """Close the connection."""
>          self._sync(
>              self._aqmp.disconnect()
>          )
>  
>      def settimeout(self, timeout: Optional[float]) -> None:
> +        """
> +        Set the timeout for QMP RPC execution.
> +
> +        This timeout affects the `cmd`, `cmd_obj`, and `command` methods.
> +        The `accept`, `pull_event` and `get_event` methods have their
> +        own configurable timeouts.
> +
> +        :param timeout:
> +            timeout in seconds, or None.
> +            None will wait indefinitely.
> +        """
>          self._timeout = timeout
>  
>      def send_fd_scm(self, fd: int) -> None:
> +        """
> +        Send a file descriptor to the remote via SCM_RIGHTS.
> +        """
>          self._aqmp.send_fd_scm(fd)
>  
>      def __del__(self) -> None:

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

--
Beraldo

Re: [PATCH v2 23/25] python: remove the old QMP package

[Beraldo Leal]

On Wed, Dec 15, 2021 at 02:39:37PM -0500, John Snow wrote:
> Thank you for your service!
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  python/PACKAGE.rst          |   4 +-
>  python/README.rst           |   2 +-
>  python/qemu/qmp/README.rst  |   9 -
>  python/qemu/qmp/__init__.py | 396 ------------------------------------
>  python/qemu/qmp/py.typed    |   0
>  python/setup.cfg            |   3 +-
>  6 files changed, 4 insertions(+), 410 deletions(-)
>  delete mode 100644 python/qemu/qmp/README.rst
>  delete mode 100644 python/qemu/qmp/__init__.py
>  delete mode 100644 python/qemu/qmp/py.typed
> 
> diff --git a/python/PACKAGE.rst b/python/PACKAGE.rst
> index b0b86cc4c3..ddfa9ba3f5 100644
> --- a/python/PACKAGE.rst
> +++ b/python/PACKAGE.rst
> @@ -8,11 +8,11 @@ to change at any time.
>  Usage
>  -----
>  
> -The ``qemu.qmp`` subpackage provides a library for communicating with
> +The ``qemu.aqmp`` subpackage provides a library for communicating with
>  QMP servers. The ``qemu.machine`` subpackage offers rudimentary
>  facilities for launching and managing QEMU processes. Refer to each
>  package's documentation
> -(``>>> help(qemu.qmp)``, ``>>> help(qemu.machine)``)
> +(``>>> help(qemu.aqmp)``, ``>>> help(qemu.machine)``)
>  for more information.
>  
>  Contributing
> diff --git a/python/README.rst b/python/README.rst
> index fcf74f69ea..eb5213337d 100644
> --- a/python/README.rst
> +++ b/python/README.rst
> @@ -3,7 +3,7 @@ QEMU Python Tooling
>  
>  This directory houses Python tooling used by the QEMU project to build,
>  configure, and test QEMU. It is organized by namespace (``qemu``), and
> -then by package (e.g. ``qemu/machine``, ``qemu/qmp``, etc).
> +then by package (e.g. ``qemu/machine``, ``qemu/aqmp``, etc).
>  
>  ``setup.py`` is used by ``pip`` to install this tooling to the current
>  environment. ``setup.cfg`` provides the packaging configuration used by
> diff --git a/python/qemu/qmp/README.rst b/python/qemu/qmp/README.rst
> deleted file mode 100644
> index 5bfb82535f..0000000000
> --- a/python/qemu/qmp/README.rst
> +++ /dev/null
> @@ -1,9 +0,0 @@
> -qemu.qmp package
> -================
> -
> -This package provides a library used for connecting to and communicating
> -with QMP servers. It is used extensively by iotests, vm tests,
> -avocado tests, and other utilities in the ./scripts directory. It is
> -not a fully-fledged SDK and is subject to change at any time.
> -
> -See the documentation in ``__init__.py`` for more information.
> diff --git a/python/qemu/qmp/__init__.py b/python/qemu/qmp/__init__.py
> deleted file mode 100644
> index 4e08641154..0000000000
> --- a/python/qemu/qmp/__init__.py
> +++ /dev/null
> @@ -1,396 +0,0 @@
> -"""
> -QEMU Monitor Protocol (QMP) development library & tooling.
> -
> -This package provides a fairly low-level class for communicating to QMP
> -protocol servers, as implemented by QEMU, the QEMU Guest Agent, and the
> -QEMU Storage Daemon. This library is not intended for production use.
> -
> -`QEMUMonitorProtocol` is the primary class of interest, and all errors
> -raised derive from `QMPError`.
> -"""
> -
> -# Copyright (C) 2009, 2010 Red Hat Inc.
> -#
> -# Authors:
> -#  Luiz Capitulino <lcapitulino@redhat.com>
> -#
> -# This work is licensed under the terms of the GNU GPL, version 2.  See
> -# the COPYING file in the top-level directory.
> -
> -import errno
> -import json
> -import logging
> -import socket
> -import struct
> -from types import TracebackType
> -from typing import (
> -    Any,
> -    Dict,
> -    List,
> -    Optional,
> -    TextIO,
> -    Tuple,
> -    Type,
> -    TypeVar,
> -    Union,
> -    cast,
> -)
> -
> -
> -#: QMPMessage is an entire QMP message of any kind.
> -QMPMessage = Dict[str, Any]
> -
> -#: QMPReturnValue is the 'return' value of a command.
> -QMPReturnValue = object
> -
> -#: QMPObject is any object in a QMP message.
> -QMPObject = Dict[str, object]
> -
> -# QMPMessage can be outgoing commands or incoming events/returns.
> -# QMPReturnValue is usually a dict/json object, but due to QAPI's
> -# 'returns-whitelist', it can actually be anything.
> -#
> -# {'return': {}} is a QMPMessage,
> -# {} is the QMPReturnValue.
> -
> -
> -InternetAddrT = Tuple[str, int]
> -UnixAddrT = str
> -SocketAddrT = Union[InternetAddrT, UnixAddrT]
> -
> -
> -class QMPError(Exception):
> -    """
> -    QMP base exception
> -    """
> -
> -
> -class QMPConnectError(QMPError):
> -    """
> -    QMP connection exception
> -    """
> -
> -
> -class QMPCapabilitiesError(QMPError):
> -    """
> -    QMP negotiate capabilities exception
> -    """
> -
> -
> -class QMPTimeoutError(QMPError):
> -    """
> -    QMP timeout exception
> -    """
> -
> -
> -class QMPProtocolError(QMPError):
> -    """
> -    QMP protocol error; unexpected response
> -    """
> -
> -
> -class QMPResponseError(QMPError):
> -    """
> -    Represents erroneous QMP monitor reply
> -    """
> -    def __init__(self, reply: QMPMessage):
> -        try:
> -            desc = reply['error']['desc']
> -        except KeyError:
> -            desc = reply
> -        super().__init__(desc)
> -        self.reply = reply
> -
> -
> -class QEMUMonitorProtocol:
> -    """
> -    Provide an API to connect to QEMU via QEMU Monitor Protocol (QMP) and then
> -    allow to handle commands and events.
> -    """
> -
> -    #: Logger object for debugging messages
> -    logger = logging.getLogger('QMP')
> -
> -    def __init__(self, address: SocketAddrT,
> -                 server: bool = False,
> -                 nickname: Optional[str] = None):
> -        """
> -        Create a QEMUMonitorProtocol class.
> -
> -        @param address: QEMU address, can be either a unix socket path (string)
> -                        or a tuple in the form ( address, port ) for a TCP
> -                        connection
> -        @param server: server mode listens on the socket (bool)
> -        @raise OSError on socket connection errors
> -        @note No connection is established, this is done by the connect() or
> -              accept() methods
> -        """
> -        self.__events: List[QMPMessage] = []
> -        self.__address = address
> -        self.__sock = self.__get_sock()
> -        self.__sockfile: Optional[TextIO] = None
> -        self._nickname = nickname
> -        if self._nickname:
> -            self.logger = logging.getLogger('QMP').getChild(self._nickname)
> -        if server:
> -            self.__sock.setsockopt(socket.SOL_SOCKET, socket.SO_REUSEADDR, 1)
> -            self.__sock.bind(self.__address)
> -            self.__sock.listen(1)
> -
> -    def __get_sock(self) -> socket.socket:
> -        if isinstance(self.__address, tuple):
> -            family = socket.AF_INET
> -        else:
> -            family = socket.AF_UNIX
> -        return socket.socket(family, socket.SOCK_STREAM)
> -
> -    def __negotiate_capabilities(self) -> QMPMessage:
> -        greeting = self.__json_read()
> -        if greeting is None or "QMP" not in greeting:
> -            raise QMPConnectError
> -        # Greeting seems ok, negotiate capabilities
> -        resp = self.cmd('qmp_capabilities')
> -        if resp and "return" in resp:
> -            return greeting
> -        raise QMPCapabilitiesError
> -
> -    def __json_read(self, only_event: bool = False) -> Optional[QMPMessage]:
> -        assert self.__sockfile is not None
> -        while True:
> -            data = self.__sockfile.readline()
> -            if not data:
> -                return None
> -            # By definition, any JSON received from QMP is a QMPMessage,
> -            # and we are asserting only at static analysis time that it
> -            # has a particular shape.
> -            resp: QMPMessage = json.loads(data)
> -            if 'event' in resp:
> -                self.logger.debug("<<< %s", resp)
> -                self.__events.append(resp)
> -                if not only_event:
> -                    continue
> -            return resp
> -
> -    def __get_events(self, wait: Union[bool, float] = False) -> None:
> -        """
> -        Check for new events in the stream and cache them in __events.
> -
> -        @param wait (bool): block until an event is available.
> -        @param wait (float): If wait is a float, treat it as a timeout value.
> -
> -        @raise QMPTimeoutError: If a timeout float is provided and the timeout
> -                                period elapses.
> -        @raise QMPConnectError: If wait is True but no events could be
> -                                retrieved or if some other error occurred.
> -        """
> -
> -        # Current timeout and blocking status
> -        current_timeout = self.__sock.gettimeout()
> -
> -        # Check for new events regardless and pull them into the cache:
> -        self.__sock.settimeout(0)  # i.e. setblocking(False)
> -        try:
> -            self.__json_read()
> -        except OSError as err:
> -            # EAGAIN: No data available; not critical
> -            if err.errno != errno.EAGAIN:
> -                raise
> -        finally:
> -            self.__sock.settimeout(current_timeout)
> -
> -        # Wait for new events, if needed.
> -        # if wait is 0.0, this means "no wait" and is also implicitly false.
> -        if not self.__events and wait:
> -            if isinstance(wait, float):
> -                self.__sock.settimeout(wait)
> -            try:
> -                ret = self.__json_read(only_event=True)
> -            except socket.timeout as err:
> -                raise QMPTimeoutError("Timeout waiting for event") from err
> -            except Exception as err:
> -                msg = "Error while reading from socket"
> -                raise QMPConnectError(msg) from err
> -            finally:
> -                self.__sock.settimeout(current_timeout)
> -
> -            if ret is None:
> -                raise QMPConnectError("Error while reading from socket")
> -
> -    T = TypeVar('T')
> -
> -    def __enter__(self: T) -> T:
> -        # Implement context manager enter function.
> -        return self
> -
> -    def __exit__(self,
> -                 # pylint: disable=duplicate-code
> -                 # see https://github.com/PyCQA/pylint/issues/3619
> -                 exc_type: Optional[Type[BaseException]],
> -                 exc_val: Optional[BaseException],
> -                 exc_tb: Optional[TracebackType]) -> None:
> -        # Implement context manager exit function.
> -        self.close()
> -
> -    def connect(self, negotiate: bool = True) -> Optional[QMPMessage]:
> -        """
> -        Connect to the QMP Monitor and perform capabilities negotiation.
> -
> -        @return QMP greeting dict, or None if negotiate is false
> -        @raise OSError on socket connection errors
> -        @raise QMPConnectError if the greeting is not received
> -        @raise QMPCapabilitiesError if fails to negotiate capabilities
> -        """
> -        self.__sock.connect(self.__address)
> -        self.__sockfile = self.__sock.makefile(mode='r')
> -        if negotiate:
> -            return self.__negotiate_capabilities()
> -        return None
> -
> -    def accept(self, timeout: Optional[float] = 15.0) -> QMPMessage:
> -        """
> -        Await connection from QMP Monitor and perform capabilities negotiation.
> -
> -        @param timeout: timeout in seconds (nonnegative float number, or
> -                        None). The value passed will set the behavior of the
> -                        underneath QMP socket as described in [1].
> -                        Default value is set to 15.0.
> -
> -        @return QMP greeting dict
> -        @raise OSError on socket connection errors
> -        @raise QMPConnectError if the greeting is not received
> -        @raise QMPCapabilitiesError if fails to negotiate capabilities
> -
> -        [1]
> -        https://docs.python.org/3/library/socket.html#socket.socket.settimeout
> -        """
> -        self.__sock.settimeout(timeout)
> -        self.__sock, _ = self.__sock.accept()
> -        self.__sockfile = self.__sock.makefile(mode='r')
> -        return self.__negotiate_capabilities()
> -
> -    def cmd_obj(self, qmp_cmd: QMPMessage) -> QMPMessage:
> -        """
> -        Send a QMP command to the QMP Monitor.
> -
> -        @param qmp_cmd: QMP command to be sent as a Python dict
> -        @return QMP response as a Python dict
> -        """
> -        self.logger.debug(">>> %s", qmp_cmd)
> -        self.__sock.sendall(json.dumps(qmp_cmd).encode('utf-8'))
> -        resp = self.__json_read()
> -        if resp is None:
> -            raise QMPConnectError("Unexpected empty reply from server")
> -        self.logger.debug("<<< %s", resp)
> -        return resp
> -
> -    def cmd(self, name: str,
> -            args: Optional[Dict[str, object]] = None,
> -            cmd_id: Optional[object] = None) -> QMPMessage:
> -        """
> -        Build a QMP command and send it to the QMP Monitor.
> -
> -        @param name: command name (string)
> -        @param args: command arguments (dict)
> -        @param cmd_id: command id (dict, list, string or int)
> -        """
> -        qmp_cmd: QMPMessage = {'execute': name}
> -        if args:
> -            qmp_cmd['arguments'] = args
> -        if cmd_id:
> -            qmp_cmd['id'] = cmd_id
> -        return self.cmd_obj(qmp_cmd)
> -
> -    def command(self, cmd: str, **kwds: object) -> QMPReturnValue:
> -        """
> -        Build and send a QMP command to the monitor, report errors if any
> -        """
> -        ret = self.cmd(cmd, kwds)
> -        if 'error' in ret:
> -            raise QMPResponseError(ret)
> -        if 'return' not in ret:
> -            raise QMPProtocolError(
> -                "'return' key not found in QMP response '{}'".format(str(ret))
> -            )
> -        return cast(QMPReturnValue, ret['return'])
> -
> -    def pull_event(self,
> -                   wait: Union[bool, float] = False) -> Optional[QMPMessage]:
> -        """
> -        Pulls a single event.
> -
> -        @param wait (bool): block until an event is available.
> -        @param wait (float): If wait is a float, treat it as a timeout value.
> -
> -        @raise QMPTimeoutError: If a timeout float is provided and the timeout
> -                                period elapses.
> -        @raise QMPConnectError: If wait is True but no events could be
> -                                retrieved or if some other error occurred.
> -
> -        @return The first available QMP event, or None.
> -        """
> -        self.__get_events(wait)
> -
> -        if self.__events:
> -            return self.__events.pop(0)
> -        return None
> -
> -    def get_events(self, wait: bool = False) -> List[QMPMessage]:
> -        """
> -        Get a list of available QMP events and clear all pending events.
> -
> -        @param wait (bool): block until an event is available.
> -        @param wait (float): If wait is a float, treat it as a timeout value.
> -
> -        @raise QMPTimeoutError: If a timeout float is provided and the timeout
> -                                period elapses.
> -        @raise QMPConnectError: If wait is True but no events could be
> -                                retrieved or if some other error occurred.
> -
> -        @return The list of available QMP events.
> -        """
> -        self.__get_events(wait)
> -        events = self.__events
> -        self.__events = []
> -        return events
> -
> -    def clear_events(self) -> None:
> -        """
> -        Clear current list of pending events.
> -        """
> -        self.__events = []
> -
> -    def close(self) -> None:
> -        """
> -        Close the socket and socket file.
> -        """
> -        if self.__sock:
> -            self.__sock.close()
> -        if self.__sockfile:
> -            self.__sockfile.close()
> -
> -    def settimeout(self, timeout: Optional[float]) -> None:
> -        """
> -        Set the socket timeout.
> -
> -        @param timeout (float): timeout in seconds (non-zero), or None.
> -        @note This is a wrap around socket.settimeout
> -
> -        @raise ValueError: if timeout was set to 0.
> -        """
> -        if timeout == 0:
> -            msg = "timeout cannot be 0; this engages non-blocking mode."
> -            msg += " Use 'None' instead to disable timeouts."
> -            raise ValueError(msg)
> -        self.__sock.settimeout(timeout)
> -
> -    def send_fd_scm(self, fd: int) -> None:
> -        """
> -        Send a file descriptor to the remote via SCM_RIGHTS.
> -        """
> -        if self.__sock.family != socket.AF_UNIX:
> -            raise RuntimeError("Can't use SCM_RIGHTS on non-AF_UNIX socket.")
> -
> -        self.__sock.sendmsg(
> -            [b' '],
> -            [(socket.SOL_SOCKET, socket.SCM_RIGHTS, struct.pack('@i', fd))]
> -        )
> diff --git a/python/qemu/qmp/py.typed b/python/qemu/qmp/py.typed
> deleted file mode 100644
> index e69de29bb2..0000000000
> diff --git a/python/setup.cfg b/python/setup.cfg
> index 510df23698..5140a5b322 100644
> --- a/python/setup.cfg
> +++ b/python/setup.cfg
> @@ -24,10 +24,9 @@ classifiers =
>  [options]
>  python_requires = >= 3.6
>  packages =
> -    qemu.qmp
> +    qemu.aqmp
>      qemu.machine
>      qemu.utils
> -    qemu.aqmp
>  
>  [options.package_data]
>  * = py.typed

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

--
Beraldo

Re: [PATCH v2 24/25] python: re-enable pylint duplicate-code warnings

[Beraldo Leal]

On Wed, Dec 15, 2021 at 02:39:38PM -0500, John Snow wrote:
> With the old library gone, there's nothing duplicated in the tree, so
> the warning suppression can be removed.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  python/setup.cfg | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/python/setup.cfg b/python/setup.cfg
> index 5140a5b322..c341e922c2 100644
> --- a/python/setup.cfg
> +++ b/python/setup.cfg
> @@ -114,7 +114,6 @@ ignore_missing_imports = True
>  disable=consider-using-f-string,
>          too-many-function-args,  # mypy handles this with less false positives.
>          no-member,  # mypy also handles this better.
> -        duplicate-code,  # To be removed by the end of this patch series.
>  
>  [pylint.basic]
>  # Good variable names which should always be accepted, separated by a comma.

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

--
Beraldo

Re: [PATCH v2 00/25] Python: delete synchronous qemu.qmp package

[John Snow]

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

On Thu, Dec 16, 2021 at 5:51 AM Daniel P. Berrangé <berrange@redhat.com>
wrote:

> On Wed, Dec 15, 2021 at 02:39:14PM -0500, John Snow wrote:
> > GitLab: https://gitlab.com/jsnow/qemu/-/commits/python-qmp-legacy-switch
> > CI: https://gitlab.com/jsnow/qemu/-/pipelines/430491195
> >
> > Hi, this series is part of an effort to publish the qemu.qmp package on
> > PyPI. It is the first of three series to complete this work:
> >
> > --> (1) Switch the new Async QMP library in to python/qemu/qmp
> >     (2) Fork python/qemu/qmp out into its own repository,
> >         with updated GitLab CI/CD targets to build packages.
> >     (3) Update qemu.git to install qemu.qmp from PyPI,
> >         and then delete python/qemu/qmp.
>
> What timeframe are you suggesting step (3) for ?
>
>
Roughly around when (2) happens; I don't want to maintain the same code in
two different places.


> In the series for (2) you're calling it version 0.0.1 indicating
> it is liable to  have API incompatible changes.
>

Yes. (We can pin to version 0.0.1 in-tree and bump it manually when
required.)


> For step (3), either we're going to have to fetch a precise
> version number to avoid risk of API breakage, or we're going
> to have to call it stable in (2) and commit to the API.
>

Pinning is what I'd prefer, I have some changes in mind that may require
some API changes, so I'd rather not call it stable yet.

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

Re: [PATCH v2 05/25] python/aqmp: rename AQMPError to QMPError

[John Snow]

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

On Thu, Dec 16, 2021 at 5:09 AM Vladimir Sementsov-Ogievskiy <
vsementsov@virtuozzo.com> wrote:

> 15.12.2021 22:39, John Snow wrote:
> > This is in preparation for renaming qemu.aqmp to qemu.qmp. I should have
> > done this from this from the very beginning, but it's a convenient time
> > to make sure this churn is taken care of.
> >
> > Signed-off-by: John Snow <jsnow@redhat.com>
>
> Honestly, I don't want to check how it intersects with exising QMPError
> from old library, as it's going to be removed anyway. Hope, everything is
> OK:
>
>
it's all namespaced, and it doesn't appear to conflict in practice.


> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>
> ---
> >   python/qemu/aqmp/__init__.py   |  6 +++---
> >   python/qemu/aqmp/error.py      | 12 ++++++------
> >   python/qemu/aqmp/events.py     |  4 ++--
> >   python/qemu/aqmp/legacy.py     |  4 ++--
> >   python/qemu/aqmp/protocol.py   |  8 ++++----
> >   python/qemu/aqmp/qmp_client.py |  8 ++++----
> >   6 files changed, 21 insertions(+), 21 deletions(-)
> >
> > diff --git a/python/qemu/aqmp/__init__.py b/python/qemu/aqmp/__init__.py
> > index c6fa2dda58..e1efab00af 100644
> > --- a/python/qemu/aqmp/__init__.py
> > +++ b/python/qemu/aqmp/__init__.py
> > @@ -6,7 +6,7 @@
> >   QEMU Guest Agent, and the QEMU Storage Daemon.
> >
> >   `QMPClient` provides the main functionality of this package. All errors
> > -raised by this library dervive from `AQMPError`, see `aqmp.error` for
> > +raised by this library dervive from `QMPError`, see `aqmp.error` for
>
> preexisting: s/dervive/derive/
>
>
Whoops. I'll add a patch to fix it. Thanks.

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

Re: [PATCH v2 01/25] python/aqmp: add __del__ method to legacy interface

[John Snow]

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

On Thu, Dec 16, 2021 at 4:31 AM Vladimir Sementsov-Ogievskiy <
vsementsov@virtuozzo.com> wrote:

> 15.12.2021 22:39, John Snow wrote:
> > asyncio can complain *very* loudly if you forget to back out of things
> > gracefully before the garbage collector starts destroying objects that
> > contain live references to asyncio Tasks.
> >
> > The usual fix is just to remember to call aqmp.disconnect(), but for the
> > sake of the legacy wrapper and quick, one-off scripts where a graceful
> > shutdown is not necessarily of paramount imporance, add a courtesy
> > cleanup that will trigger prior to seeing screenfuls of confusing
> > asyncio tracebacks.
> >
> > Note that we can't *always* save you from yourself; depending on when
> > the GC runs, you might just seriously be out of luck. The best we can do
> > in this case is to gently remind you to clean up after yourself.
> >
> > (Still much better than multiple pages of incomprehensible python
> > warnings for the crime of forgetting to put your toys away.)
> >
> > Signed-off-by: John Snow <jsnow@redhat.com>
> > ---
> >   python/qemu/aqmp/legacy.py | 18 ++++++++++++++++++
> >   1 file changed, 18 insertions(+)
> >
> > diff --git a/python/qemu/aqmp/legacy.py b/python/qemu/aqmp/legacy.py
> > index 9e7b9fb80b..2ccb136b02 100644
> > --- a/python/qemu/aqmp/legacy.py
> > +++ b/python/qemu/aqmp/legacy.py
> > @@ -16,6 +16,8 @@
> >   import qemu.qmp
> >   from qemu.qmp import QMPMessage, QMPReturnValue, SocketAddrT
> >
> > +from .error import AQMPError
> > +from .protocol import Runstate
> >   from .qmp_client import QMPClient
> >
> >
> > @@ -136,3 +138,19 @@ def settimeout(self, timeout: Optional[float]) ->
> None:
> >
> >       def send_fd_scm(self, fd: int) -> None:
> >           self._aqmp.send_fd_scm(fd)
> > +
> > +    def __del__(self) -> None:
> > +        if self._aqmp.runstate == Runstate.IDLE:
> > +            return
> > +
> > +        if not self._aloop.is_running():
> > +            self.close()
>
> As I understand, if close() was called by hand, runstate should already be
> IDLE, and we don't call close() twice here?
>
>
Right ... calling close() twice is actually OK (the second one just does
nothing) but what I am avoiding here is this scenario:

- close() was already called
- garbage collection runs while the event loop is running
- we should therefore NOT raise an exception.

The problem is that even if a second close() does nothing, we are not able
to call into it if we're already inside of an active event loop -- so in
order to achieve the no-op behavior from the GC context, I need to manually
check the runstate to determine if there /WILL/ be a problem when the GC
runs. If it's idle, there definitely won't be.


> > +        else:
> > +            # Garbage collection ran while the event loop was running.
> > +            # Nothing we can do about it now, but if we don't raise our
> > +            # own error, the user will be treated to a lot of traceback
> > +            # they might not understand.
> > +            raise AQMPError(
> > +                "QEMUMonitorProtocol.close()"
> > +                " was not called before object was garbage collected"
> > +            )
> >
>
> weak, as I'm far from understanding aqmp library:
>
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>
>
thanks!
--js

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

Re: [PATCH v2 02/25] python/aqmp: handle asyncio.TimeoutError on execute()

[John Snow]

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

On Thu, Dec 16, 2021 at 4:51 AM Vladimir Sementsov-Ogievskiy <
vsementsov@virtuozzo.com> wrote:

> 15.12.2021 22:39, John Snow wrote:
> > This exception can be injected into any await statement. If we are
> > canceled via timeout, we want to clear the pending execution record on
> > our way out.
>
> Hmm, but there are more await statements in the file, shouldn't we care
> about them too ?
>
>
Did any catch your eye? Depending on where it fails, it may not need any
additional cleanup. In this case, it's important to delete the _pending
entry so that we don't leave stale entries behind.


> >
> > Signed-off-by: John Snow <jsnow@redhat.com>
> > ---
> >   python/qemu/aqmp/qmp_client.py | 8 ++++++--
> >   1 file changed, 6 insertions(+), 2 deletions(-)
> >
> > diff --git a/python/qemu/aqmp/qmp_client.py
> b/python/qemu/aqmp/qmp_client.py
> > index 8105e29fa8..6a985ffe30 100644
> > --- a/python/qemu/aqmp/qmp_client.py
> > +++ b/python/qemu/aqmp/qmp_client.py
> > @@ -435,7 +435,11 @@ async def _issue(self, msg: Message) -> Union[None,
> str]:
> >               msg_id = msg['id']
> >
> >           self._pending[msg_id] = asyncio.Queue(maxsize=1)
> > -        await self._outgoing.put(msg)
> > +        try:
> > +            await self._outgoing.put(msg)
> > +        except:
>
> Doesn't pylint and others complain about plain "except". Do we really need
> to catch any exception here? As far as I know that's not a good practice.
>
>
pylint won't complain as long as you also ubiquitously re-raise the
exception. It's only a bad practice to suppress all exceptions, but it's OK
to define cleanup actions.

> +            del self._pending[msg_id]
> > +            raise
> >
> >           return msg_id
> >
> > @@ -452,9 +456,9 @@ async def _reply(self, msg_id: Union[str, None]) ->
> Message:
> >               was lost, or some other problem.
> >           """
> >           queue = self._pending[msg_id]
> > -        result = await queue.get()
> >
> >           try:
> > +            result = await queue.get()
> >               if isinstance(result, ExecInterruptedError):
> >                   raise result
> >               return result
> >
>
> This one looks good, just include it into existing try-finally
>
> Hmm. _issue() and _reply() are used only in one place, as a pair. It looks
> like both "awaits" should be better under one try-finally block.
>

They could. I split them for the sake of sub-classing if you wanted to
perform additional actions on the outgoing/incoming arms of the execute()
action. Specifically, I am accommodating the case that someone wants to
subclass QMPClient and create methods where a QMP command is *sent* but is
not *awaited*, i.e. _issue() is called without an immediate _reply(). This
allows us the chance to create something like a PendingExecution object
that could be awaited later on.

The simpler case, execute(), doesn't bother with separating those actions
and just awaits the reply immediately.


>
>
For example, move "self._pending[msg_id] = asyncio.Queue(maxsize=1)" to
> _execute, and just do try-finally in _execute() around _issue and _reply.
> Or may be just merge the whole logic in _execute, it doesn't seem too much.
> What do you think?
>
>

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

Re: [PATCH v2 02/25] python/aqmp: handle asyncio.TimeoutError on execute()

[John Snow]

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

On Thu, Dec 16, 2021 at 7:39 AM Beraldo Leal <bleal@redhat.com> wrote:

> On Wed, Dec 15, 2021 at 02:39:16PM -0500, John Snow wrote:
> > This exception can be injected into any await statement. If we are
> > canceled via timeout, we want to clear the pending execution record on
> > our way out.
> >
> > Signed-off-by: John Snow <jsnow@redhat.com>
> > ---
> >  python/qemu/aqmp/qmp_client.py | 8 ++++++--
> >  1 file changed, 6 insertions(+), 2 deletions(-)
> >
> > diff --git a/python/qemu/aqmp/qmp_client.py
> b/python/qemu/aqmp/qmp_client.py
> > index 8105e29fa8..6a985ffe30 100644
> > --- a/python/qemu/aqmp/qmp_client.py
> > +++ b/python/qemu/aqmp/qmp_client.py
> > @@ -435,7 +435,11 @@ async def _issue(self, msg: Message) -> Union[None,
> str]:
> >              msg_id = msg['id']
> >
> >          self._pending[msg_id] = asyncio.Queue(maxsize=1)
> > -        await self._outgoing.put(msg)
> > +        try:
> > +            await self._outgoing.put(msg)
> > +        except:
> > +            del self._pending[msg_id]
> > +            raise
>
> At first glance both, except and raise are not good practices, but I
> think I got what you are doing here, just intercepting to delete the
> pending message and raising it again. So maybe it will be safe to close
> the eyes here. ;)
>
>
Yeah, IMO it's 100% legitimate to define cleanup actions using catch-all
except statements as long as you re-raise. There's no other construct that
handles this case, AFAIK.

try
except <- on error
else <- on NOT error
finally <- always

Sometimes you just wanna undo stuff only on error. Maybe a higher layer
will "handle" that exception, maybe it'll crash the program. We don't know
here, we just know we need to undo some stuff.


> >
> >          return msg_id
> >
> > @@ -452,9 +456,9 @@ async def _reply(self, msg_id: Union[str, None]) ->
> Message:
> >              was lost, or some other problem.
> >          """
> >          queue = self._pending[msg_id]
> > -        result = await queue.get()
> >
> >          try:
> > +            result = await queue.get()
> >              if isinstance(result, ExecInterruptedError):
> >                  raise result
> >              return result
>
> Reviewed-by: Beraldo Leal <bleal@redhat.com>
>
>
ty :)

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

Re: [PATCH v2 03/25] python/aqmp: copy type definitions from qmp

[John Snow]

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

On Thu, Dec 16, 2021 at 5:19 AM Daniel P. Berrangé <berrange@redhat.com>
wrote:

> On Wed, Dec 15, 2021 at 02:39:17PM -0500, John Snow wrote:
> > Copy the remaining type definitions from QMP into the qemu.aqmp.legacy
> > module. Now, most users don't need to import anything else but
> > qemu.aqmp.legacy.
>
> I'm probably missing the historical discussion but it feels very
> wierd to be saying
>
>    "most users don't need anything except  <something> legacy"
>
> Naively, I'd expect most users to want something *not* legacy.
>
>
> Regards,
> Daniel
>

The legacy.py module is a wrapper that pretends to be the
QEMUMonitorProtocol class written by Luiz Capitulino. It exists as a
synchronous wrapper around the async core. However, it's not a design that
I think actually makes sense long-term for the library as a standalone
project, so I named it legacy to intuit that it's providing compatibility
for an older interface.

The goal is to eventually remove it in favor of a sync.py that has a sync
interface that's more closely aligned to the async core. I've got a series
for this, but I wanted to pursue this tree-wide switcheroo first.

As for the phrasing, I suppose I mean: "most users [that need the legacy
interface] don't need to import both the old interface AND the legacy
wrapper that mimics it, they can just import the legacy wrapper."

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

Re: [PATCH v2 06/25] python/qemu-ga-client: update instructions to newer CLI syntax

[John Snow]

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

On Thu, Dec 16, 2021 at 5:14 AM Vladimir Sementsov-Ogievskiy <
vsementsov@virtuozzo.com> wrote:

> I had to search a bit through the history to check this )
>
> The commit ccd3b3b8112b670f "qemu-option: warn for short-form boolean
> options" may be noted here.
>
> And may be subject changed to "don't use deprecated syntax in comment" or
> something like this. server=on is not a *new* syntax I think.
>
>
Fair. I've updated the commit message.


> 15.12.2021 22:39, John Snow wrote:
> > Signed-off-by: John Snow <jsnow@redhat.com>
>
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>
>
Thanks!

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

Re: [PATCH v2 07/25] python/qmp: switch qemu-ga-client to AQMP

[John Snow]

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

On Thu, Dec 16, 2021 at 5:31 AM Vladimir Sementsov-Ogievskiy <
vsementsov@virtuozzo.com> wrote:

> 15.12.2021 22:39, John Snow wrote:
> > Signed-off-by: John Snow <jsnow@redhat.com>
>
> Not simple to check, how much new behavior is equal to the old one.. And
> impossible to check, is everything updated that should be )
>
>
Agree, it's hard to tell. Mostly: iotests has been using the new interface
for the 6.2 release testing cycle and we didn't find many bugs, so I'm
assuming we can largely switch these, too.

Big assumption! but, I actually doubt people use this script much ...


>
> > ---
> >   python/qemu/qmp/qemu_ga_client.py | 22 +++++++++++-----------
> >   1 file changed, 11 insertions(+), 11 deletions(-)
> >
> > diff --git a/python/qemu/qmp/qemu_ga_client.py
> b/python/qemu/qmp/qemu_ga_client.py
> > index b3e1d98c9e..15ed430c61 100644
> > --- a/python/qemu/qmp/qemu_ga_client.py
> > +++ b/python/qemu/qmp/qemu_ga_client.py
>
> [..]
>
> >
> >       try:
> >           client = QemuGuestAgentClient(address)
> > -    except OSError as err:
> > +    except ConnectError as err:
> >           print(err)
> > -        if err.errno == errno.ECONNREFUSED:
> > -            print('Hint: qemu is not running?')
> > +        if isinstance(err.exc, ConnectionError):
> > +            print('(Is QEMU running?)')
>
> It at least a bit changed from checking errno to checking the class, I'd
> note it in commit message. And anyway commit message may be more
> informative. Still, I just don't care too much about testing framework.
>
>
OK, I'll update the commit message. The problem is that the backing errors
originally emitted by a blocking socket may no longer be exactly those
raised by the async core, so it's not 1:1.

In this case, the new QMP library always raises qemu.[a]qmp.ConnectError,
and that houses a backing exception with the "real reason". that "real
reason" may be the built-in ConnectionError, which just indicates some kind
of connection problem, including ECONNREFUSED. So the scope of this
exception has expanded a little.

>
> Nothing seems wrong to me, so weak:
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>
> --
> Best regards,
> Vladimir
>
>

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

Re: [PATCH v2 12/25] python/machine: permanently switch to AQMP

[John Snow]

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

On Thu, Dec 16, 2021 at 5:52 AM Vladimir Sementsov-Ogievskiy <
vsementsov@virtuozzo.com> wrote:

> 15.12.2021 22:39, John Snow wrote:
> > Remove the QEMU_PYTHON_LEGACY_QMP environment variable, making the
> > switch permanent. Update Exceptions and import paths as necessary.
> >
> > Signed-off-by: John Snow <jsnow@redhat.com>
> > ---
> >   python/qemu/machine/machine.py | 18 +++++++-----------
> >   python/qemu/machine/qtest.py   |  2 +-
> >   2 files changed, 8 insertions(+), 12 deletions(-)
> >
> > diff --git a/python/qemu/machine/machine.py
> b/python/qemu/machine/machine.py
> > index 67ab06ca2b..21fb4a4f30 100644
> > --- a/python/qemu/machine/machine.py
> > +++ b/python/qemu/machine/machine.py
> > @@ -40,21 +40,16 @@
> >       TypeVar,
> >   )
> >
> > -from qemu.qmp import (  # pylint: disable=import-error
> > +from qemu.aqmp import SocketAddrT
> > +from qemu.aqmp.legacy import (
> > +    QEMUMonitorProtocol,
> >       QMPMessage,
> >       QMPReturnValue,
> > -    SocketAddrT,
> >   )
> >
> >   from . import console_socket
> >
> >
> > -if os.environ.get('QEMU_PYTHON_LEGACY_QMP'):
> > -    from qemu.qmp import QEMUMonitorProtocol
> > -else:
> > -    from qemu.aqmp.legacy import QEMUMonitorProtocol
> > -
> > -
> >   LOG = logging.getLogger(__name__)
> >
> >
> > @@ -710,8 +705,9 @@ def events_wait(self,
> >           :param timeout: Optional timeout, in seconds.
> >                           See QEMUMonitorProtocol.pull_event.
> >
> > -        :raise QMPTimeoutError: If timeout was non-zero and no matching
> events
> > -                                were found.
> > +        :raise asyncio.TimeoutError:
> > +            If timeout was non-zero and no matching events were found.
> > +
> >           :return: A QMP event matching the filter criteria.
> >                    If timeout was 0 and no event matched, None.
> >           """
> > @@ -734,7 +730,7 @@ def _match(event: QMPMessage) -> bool:
> >               event = self._qmp.pull_event(wait=timeout)
> >               if event is None:
> >                   # NB: None is only returned when timeout is false-ish.
> > -                # Timeouts raise QMPTimeoutError instead!
> > +                # Timeouts raise asyncio.TimeoutError instead!
> >                   break
> >               if _match(event):
> >                   return event
> > diff --git a/python/qemu/machine/qtest.py b/python/qemu/machine/qtest.py
> > index f2f9aaa5e5..817c8a5425 100644
> > --- a/python/qemu/machine/qtest.py
> > +++ b/python/qemu/machine/qtest.py
> > @@ -26,7 +26,7 @@
> >       TextIO,
> >   )
> >
> > -from qemu.qmp import SocketAddrT  # pylint: disable=import-error
> > +from qemu.aqmp.protocol import SocketAddrT
>
> You can also import it simply from qemu.aqmp
>
>
Guess what order those patches got written in :)

You're right, I'll update this.


> >
> >   from .machine import QEMUMachine
> >
> >
>
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>
>
Thanks

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

Re: [PATCH v2 02/25] python/aqmp: handle asyncio.TimeoutError on execute()

[Vladimir Sementsov-Ogievskiy]

16.12.2021 20:22, John Snow wrote:
> 
> 
> On Thu, Dec 16, 2021 at 4:51 AM Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com <mailto:vsementsov@virtuozzo.com>> wrote:
> 
>     15.12.2021 22:39, John Snow wrote:
>      > This exception can be injected into any await statement. If we are
>      > canceled via timeout, we want to clear the pending execution record on
>      > our way out.
> 
>     Hmm, but there are more await statements in the file, shouldn't we care about them too ?
> 
> 
> Did any catch your eye? Depending on where it fails, it may not need any additional cleanup. In this case, it's important to delete the _pending entry so that we don't leave stale entries behind.

No. I simply searched for "await" reading the first sentence of commit message. Now I better follow what you are doing.

> 
>      >
>      > Signed-off-by: John Snow <jsnow@redhat.com <mailto:jsnow@redhat.com>>
>      > ---
>      >   python/qemu/aqmp/qmp_client.py | 8 ++++++--
>      >   1 file changed, 6 insertions(+), 2 deletions(-)
>      >
>      > diff --git a/python/qemu/aqmp/qmp_client.py b/python/qemu/aqmp/qmp_client.py
>      > index 8105e29fa8..6a985ffe30 100644
>      > --- a/python/qemu/aqmp/qmp_client.py
>      > +++ b/python/qemu/aqmp/qmp_client.py
>      > @@ -435,7 +435,11 @@ async def _issue(self, msg: Message) -> Union[None, str]:
>      >               msg_id = msg['id']
>      >
>      >           self._pending[msg_id] = asyncio.Queue(maxsize=1)
>      > -        await self._outgoing.put(msg)
>      > +        try:
>      > +            await self._outgoing.put(msg)
>      > +        except:
> 
>     Doesn't pylint and others complain about plain "except". Do we really need to catch any exception here? As far as I know that's not a good practice.
> 
> 
> pylint won't complain as long as you also ubiquitously re-raise the exception. It's only a bad practice to suppress all exceptions, but it's OK to define cleanup actions.
> 
>      > +            del self._pending[msg_id]
>      > +            raise
>      >
>      >           return msg_id
>      >
>      > @@ -452,9 +456,9 @@ async def _reply(self, msg_id: Union[str, None]) -> Message:
>      >               was lost, or some other problem.
>      >           """
>      >           queue = self._pending[msg_id]
>      > -        result = await queue.get()
>      >
>      >           try:
>      > +            result = await queue.get()
>      >               if isinstance(result, ExecInterruptedError):
>      >                   raise result
>      >               return result
>      >
> 
>     This one looks good, just include it into existing try-finally
> 
>     Hmm. _issue() and _reply() are used only in one place, as a pair. It looks like both "awaits" should be better under one try-finally block.
> 
> 
> They could. I split them for the sake of sub-classing if you wanted to perform additional actions on the outgoing/incoming arms of the execute() action. Specifically, I am accommodating the case that someone wants to subclass QMPClient and create methods where a QMP command is *sent* but is not *awaited*, i.e. _issue() is called without an immediate _reply(). This allows us the chance to create something like a PendingExecution object that could be awaited later on.
> 
> The simpler case, execute(), doesn't bother with separating those actions and just awaits the reply immediately.
> 
> 
>     For example, move "self._pending[msg_id] = asyncio.Queue(maxsize=1)" to _execute, and just do try-finally in _execute() around _issue and _reply. Or may be just merge the whole logic in _execute, it doesn't seem too much. What do you think?
> 


OK, that's all reasonable, thanks:
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

-- 
Best regards,
Vladimir

Re: [PATCH v2 15/25] scripts/render-block-graph: switch to AQMP

[John Snow]

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

On Thu, Dec 16, 2021 at 5:58 AM Vladimir Sementsov-Ogievskiy <
vsementsov@virtuozzo.com> wrote:

> 15.12.2021 22:39, John Snow wrote:
> > Creating an instance of qemu.aqmp.ExecuteError is too involved here, so
> > just drop the specificity down to a generic AQMPError.
>
> s/AQMPError/QMPError/  ?
>
>
The problem with relying on linters to achieve consistency is that they
don't lint your commit messages :)

Fixed, thank you.


> >
> > Signed-off-by: John Snow <jsnow@redhat.com>
>
>
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>
> > ---
> >   scripts/render_block_graph.py | 8 +++-----
> >   1 file changed, 3 insertions(+), 5 deletions(-)
> >
> > diff --git a/scripts/render_block_graph.py
> b/scripts/render_block_graph.py
> > index da6acf050d..97778927f3 100755
> > --- a/scripts/render_block_graph.py
> > +++ b/scripts/render_block_graph.py
> > @@ -25,10 +25,8 @@
> >   from graphviz import Digraph
> >
> >   sys.path.append(os.path.join(os.path.dirname(__file__), '..',
> 'python'))
> > -from qemu.qmp import (
> > -    QEMUMonitorProtocol,
> > -    QMPResponseError,
> > -)
> > +from qemu.aqmp import QMPError
> > +from qemu.aqmp.legacy import QEMUMonitorProtocol
> >
> >
> >   def perm(arr):
> > @@ -105,7 +103,7 @@ def command(self, cmd):
> >           reply = json.loads(subprocess.check_output(ar))
> >
> >           if 'error' in reply:
> > -            raise QMPResponseError(reply)
> > +            raise QMPError(reply)
> >
> >           return reply['return']
> >
> >
>
>
> --
> Best regards,
> Vladimir
>
>

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

Re: [PATCH v2 25/25] python: rename qemu.aqmp to qemu.qmp

[John Snow]

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

On Thu, Dec 16, 2021 at 6:41 AM Vladimir Sementsov-Ogievskiy <
vsementsov@virtuozzo.com> wrote:

> 15.12.2021 22:39, John Snow wrote:
> > Now that we are fully switched over to the new QMP library, move it back
> > over the old namespace. This is being done primarily so that we may
> > upload this package simply as "qemu.qmp" without introducing confusion
> > over whether or not "aqmp" is a new protocol or not.
> >
> > The trade-off is increased confusion inside the QEMU developer
> > tree. Sorry!
> >
> > Signed-off-by: John Snow<jsnow@redhat.com>
>
> Great job!
>
> I looked thorough the patch, changes looks correct. Simply rename every
> aqmp / AQMP occurrence.. But:
>
>
> [root@kvm review]# git grep -i aqmp
> python/qemu/qmp/aqmp_tui.py:AQMP TUI
> python/qemu/qmp/aqmp_tui.py:AQMP TUI is an asynchronous interface built on
> top the of the AQMP library.
> python/qemu/qmp/aqmp_tui.py:Example Usage: aqmp-tui <SOCKET | TCP IP:PORT>
> python/qemu/qmp/aqmp_tui.py:Full Usage: aqmp-tui --help
> python/qemu/qmp/aqmp_tui.py:    Implements the AQMP TUI.
> python/qemu/qmp/aqmp_tui.py:    parser =
> argparse.ArgumentParser(description='AQMP TUI')
> python/qemu/qmp/legacy.py:        self._aqmp = QMPClient(nickname)
> python/qemu/qmp/legacy.py:        if self._aqmp.greeting is not None:
> python/qemu/qmp/legacy.py:            return self._aqmp.greeting._asdict()
> python/qemu/qmp/legacy.py:        self._aqmp.await_greeting = negotiate
> python/qemu/qmp/legacy.py:        self._aqmp.negotiate = negotiate
> python/qemu/qmp/legacy.py:            self._aqmp.connect(self._address)
> python/qemu/qmp/legacy.py:        self._aqmp.await_greeting = True
> python/qemu/qmp/legacy.py:        self._aqmp.negotiate = True
> python/qemu/qmp/legacy.py:            self._aqmp.accept(self._address),
> python/qemu/qmp/legacy.py:                self._aqmp._raw(qmp_cmd,
> assign_id=False),
> python/qemu/qmp/legacy.py:            self._aqmp.execute(cmd, kwds),
> python/qemu/qmp/legacy.py:            if self._aqmp.events.empty():
> python/qemu/qmp/legacy.py:                self._aqmp.events.get(),
> python/qemu/qmp/legacy.py:        events = [dict(x) for x in
> self._aqmp.events.clear()]
> python/qemu/qmp/legacy.py:        self._aqmp.events.clear()
> python/qemu/qmp/legacy.py:            self._aqmp.disconnect()
> python/qemu/qmp/legacy.py:        self._aqmp.send_fd_scm(fd)
> python/qemu/qmp/legacy.py:        if self._aqmp.runstate == Runstate.IDLE:
> python/setup.cfg:# AQMP TUI dependencies
> python/setup.cfg:    aqmp-tui = qemu.qmp.aqmp_tui:main [tui]
> python/setup.cfg:[mypy-qemu.qmp.aqmp_tui]
>
> [root@kvm review]# git ls-tree -r --name-only HEAD | grep -i aqmp
> python/qemu/qmp/aqmp_tui.py
>
>
> I think, this all should be renamed too


For aqmp_tui.py, sure. The new TUI isn't 100% ready to replace qmp-shell
yet, so I wasn't entirely certain what to name it... qmp-tui?

*shrugs*.

for legacy.py, it's just an internal variable name and I wasn't sure it was
worth the churn just to change a private variable. I could still do it if
you feel strongly about it.

--js

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

Re: [PATCH v2 25/25] python: rename qemu.aqmp to qemu.qmp

[Vladimir Sementsov-Ogievskiy]

17.12.2021 00:10, John Snow wrote:
> 
> 
> On Thu, Dec 16, 2021 at 6:41 AM Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com <mailto:vsementsov@virtuozzo.com>> wrote:
> 
>     15.12.2021 22:39, John Snow wrote:
>      > Now that we are fully switched over to the new QMP library, move it back
>      > over the old namespace. This is being done primarily so that we may
>      > upload this package simply as "qemu.qmp" without introducing confusion
>      > over whether or not "aqmp" is a new protocol or not.
>      >
>      > The trade-off is increased confusion inside the QEMU developer
>      > tree. Sorry!
>      >
>      > Signed-off-by: John Snow<jsnow@redhat.com <mailto:jsnow@redhat.com>>
> 
>     Great job!
> 
>     I looked thorough the patch, changes looks correct. Simply rename every aqmp / AQMP occurrence.. But:
> 
> 
>     [root@kvm review]# git grep -i aqmp
>     python/qemu/qmp/aqmp_tui.py:AQMP TUI
>     python/qemu/qmp/aqmp_tui.py:AQMP TUI is an asynchronous interface built on top the of the AQMP library.
>     python/qemu/qmp/aqmp_tui.py:Example Usage: aqmp-tui <SOCKET | TCP IP:PORT>
>     python/qemu/qmp/aqmp_tui.py:Full Usage: aqmp-tui --help
>     python/qemu/qmp/aqmp_tui.py:    Implements the AQMP TUI.
>     python/qemu/qmp/aqmp_tui.py:    parser = argparse.ArgumentParser(description='AQMP TUI')
>     python/qemu/qmp/legacy.py:        self._aqmp = QMPClient(nickname)
>     python/qemu/qmp/legacy.py:        if self._aqmp.greeting is not None:
>     python/qemu/qmp/legacy.py:            return self._aqmp.greeting._asdict()
>     python/qemu/qmp/legacy.py:        self._aqmp.await_greeting = negotiate
>     python/qemu/qmp/legacy.py:        self._aqmp.negotiate = negotiate
>     python/qemu/qmp/legacy.py:            self._aqmp.connect(self._address)
>     python/qemu/qmp/legacy.py:        self._aqmp.await_greeting = True
>     python/qemu/qmp/legacy.py:        self._aqmp.negotiate = True
>     python/qemu/qmp/legacy.py:            self._aqmp.accept(self._address),
>     python/qemu/qmp/legacy.py:                self._aqmp._raw(qmp_cmd, assign_id=False),
>     python/qemu/qmp/legacy.py:            self._aqmp.execute(cmd, kwds),
>     python/qemu/qmp/legacy.py:            if self._aqmp.events.empty():
>     python/qemu/qmp/legacy.py:                self._aqmp.events.get(),
>     python/qemu/qmp/legacy.py:        events = [dict(x) for x in self._aqmp.events.clear()]
>     python/qemu/qmp/legacy.py:        self._aqmp.events.clear()
>     python/qemu/qmp/legacy.py:            self._aqmp.disconnect()
>     python/qemu/qmp/legacy.py:        self._aqmp.send_fd_scm(fd)
>     python/qemu/qmp/legacy.py:        if self._aqmp.runstate == Runstate.IDLE:
>     python/setup.cfg:# AQMP TUI dependencies
>     python/setup.cfg:    aqmp-tui = qemu.qmp.aqmp_tui:main [tui]
>     python/setup.cfg:[mypy-qemu.qmp.aqmp_tui]
> 
>     [root@kvm review]# git ls-tree -r --name-only HEAD | grep -i aqmp
>     python/qemu/qmp/aqmp_tui.py
> 
> 
>     I think, this all should be renamed too
> 
> 
> For aqmp_tui.py, sure. The new TUI isn't 100% ready to replace qmp-shell yet, so I wasn't entirely certain what to name it... qmp-tui?
> 
> *shrugs*.

I don't remember what tui is abbreviating) qmp-tui is OK, and it may be renamed to qmp-shell when it is ready to replace it..

> 
> for legacy.py, it's just an internal variable name and I wasn't sure it was worth the churn just to change a private variable. I could still do it if you feel strongly about it.
> 

I'd rename everything.


-- 
Best regards,
Vladimir

Re: [PATCH v2 25/25] python: rename qemu.aqmp to qemu.qmp

[John Snow]

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

On Fri, Dec 17, 2021, 2:40 AM Vladimir Sementsov-Ogievskiy <
vsementsov@virtuozzo.com> wrote:

> 17.12.2021 00:10, John Snow wrote:
> >
> >
> > On Thu, Dec 16, 2021 at 6:41 AM Vladimir Sementsov-Ogievskiy <
> vsementsov@virtuozzo.com <mailto:vsementsov@virtuozzo.com>> wrote:
> >
> >     15.12.2021 22:39, John Snow wrote:
> >      > Now that we are fully switched over to the new QMP library, move
> it back
> >      > over the old namespace. This is being done primarily so that we
> may
> >      > upload this package simply as "qemu.qmp" without introducing
> confusion
> >      > over whether or not "aqmp" is a new protocol or not.
> >      >
> >      > The trade-off is increased confusion inside the QEMU developer
> >      > tree. Sorry!
> >      >
> >      > Signed-off-by: John Snow<jsnow@redhat.com <mailto:
> jsnow@redhat.com>>
> >
> >     Great job!
> >
> >     I looked thorough the patch, changes looks correct. Simply rename
> every aqmp / AQMP occurrence.. But:
> >
> >
> >     [root@kvm review]# git grep -i aqmp
> >     python/qemu/qmp/aqmp_tui.py:AQMP TUI
> >     python/qemu/qmp/aqmp_tui.py:AQMP TUI is an asynchronous interface
> built on top the of the AQMP library.
> >     python/qemu/qmp/aqmp_tui.py:Example Usage: aqmp-tui <SOCKET | TCP
> IP:PORT>
> >     python/qemu/qmp/aqmp_tui.py:Full Usage: aqmp-tui --help
> >     python/qemu/qmp/aqmp_tui.py:    Implements the AQMP TUI.
> >     python/qemu/qmp/aqmp_tui.py:    parser =
> argparse.ArgumentParser(description='AQMP TUI')
> >     python/qemu/qmp/legacy.py:        self._aqmp = QMPClient(nickname)
> >     python/qemu/qmp/legacy.py:        if self._aqmp.greeting is not None:
> >     python/qemu/qmp/legacy.py:            return
> self._aqmp.greeting._asdict()
> >     python/qemu/qmp/legacy.py:        self._aqmp.await_greeting =
> negotiate
> >     python/qemu/qmp/legacy.py:        self._aqmp.negotiate = negotiate
> >     python/qemu/qmp/legacy.py:
> self._aqmp.connect(self._address)
> >     python/qemu/qmp/legacy.py:        self._aqmp.await_greeting = True
> >     python/qemu/qmp/legacy.py:        self._aqmp.negotiate = True
> >     python/qemu/qmp/legacy.py:
> self._aqmp.accept(self._address),
> >     python/qemu/qmp/legacy.py:                self._aqmp._raw(qmp_cmd,
> assign_id=False),
> >     python/qemu/qmp/legacy.py:            self._aqmp.execute(cmd, kwds),
> >     python/qemu/qmp/legacy.py:            if self._aqmp.events.empty():
> >     python/qemu/qmp/legacy.py:                self._aqmp.events.get(),
> >     python/qemu/qmp/legacy.py:        events = [dict(x) for x in
> self._aqmp.events.clear()]
> >     python/qemu/qmp/legacy.py:        self._aqmp.events.clear()
> >     python/qemu/qmp/legacy.py:            self._aqmp.disconnect()
> >     python/qemu/qmp/legacy.py:        self._aqmp.send_fd_scm(fd)
> >     python/qemu/qmp/legacy.py:        if self._aqmp.runstate ==
> Runstate.IDLE:
> >     python/setup.cfg:# AQMP TUI dependencies
> >     python/setup.cfg:    aqmp-tui = qemu.qmp.aqmp_tui:main [tui]
> >     python/setup.cfg:[mypy-qemu.qmp.aqmp_tui]
> >
> >     [root@kvm review]# git ls-tree -r --name-only HEAD | grep -i aqmp
> >     python/qemu/qmp/aqmp_tui.py
> >
> >
> >     I think, this all should be renamed too
> >
> >
> > For aqmp_tui.py, sure. The new TUI isn't 100% ready to replace qmp-shell
> yet, so I wasn't entirely certain what to name it... qmp-tui?
> >
> > *shrugs*.
>
> I don't remember what tui is abbreviating) qmp-tui is OK, and it may be
> renamed to qmp-shell when it is ready to replace it..
>

"text user interface", by analogy with GUI (graphical UI).


> >
> > for legacy.py, it's just an internal variable name and I wasn't sure it
> was worth the churn just to change a private variable. I could still do it
> if you feel strongly about it.
> >
>
> I'd rename everything.
>

Alright, I'll do so in the respin.


>
> --
> Best regards,
> Vladimir
>

Thanks for the reviews!

>

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