[PATCH 0/5] python: backport socket changes from python-qemu-qmp

[PATCH 0/5] python: backport socket changes from python-qemu-qmp

[John Snow]

This is a small patchset designed to backport the changes made to the
qemu.qmp code to utilize pre-existing sockets. This contains some small
changes to machine.py to match the new API. This is necessary to do
before dropping qemu.qmp from qemu.git so we can utilize the future
0.0.3 version of qemu.qmp.

(This should also fix the bug where the buffering limit was not being
applied properly connections utilizing pre-existing sockets.)

John Snow (5):
  python/qmp: allow sockets to be passed to connect()
  python/qmp/legacy: allow using sockets for connect()
  python/machine: use connect-based interface for existing sockets
  python/qmp/legacy: remove open_with_socket() calls
  Revert "python/qmp/protocol: add open_with_socket()"

 python/qemu/machine/machine.py | 17 +++++++------
 python/qemu/qmp/legacy.py      | 26 +++++++++-----------
 python/qemu/qmp/protocol.py    | 45 +++++++++++++++-------------------
 3 files changed, 41 insertions(+), 47 deletions(-)

-- 
2.40.0

[PATCH 1/5] python/qmp: allow sockets to be passed to connect()

[John Snow]

Allow existing sockets to be passed to connect(). The changes are pretty
minimal, and this allows for far greater flexibility in setting up
communications with an endpoint.

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

diff --git a/python/qemu/qmp/protocol.py b/python/qemu/qmp/protocol.py
index 22e60298d2..d534db4631 100644
--- a/python/qemu/qmp/protocol.py
+++ b/python/qemu/qmp/protocol.py
@@ -370,7 +370,7 @@ async def accept(self) -> None:
 
     @upper_half
     @require(Runstate.IDLE)
-    async def connect(self, address: SocketAddrT,
+    async def connect(self, address: Union[SocketAddrT, socket.socket],
                       ssl: Optional[SSLContext] = None) -> None:
         """
         Connect to the server and begin processing message queues.
@@ -615,7 +615,7 @@ async def _do_accept(self) -> None:
         self.logger.debug("Connection accepted.")
 
     @upper_half
-    async def _do_connect(self, address: SocketAddrT,
+    async def _do_connect(self, address: Union[SocketAddrT, socket.socket],
                           ssl: Optional[SSLContext] = None) -> None:
         """
         Acting as the transport client, initiate a connection to a server.
@@ -634,9 +634,17 @@ async def _do_connect(self, address: SocketAddrT,
         # otherwise yield.
         await asyncio.sleep(0)
 
-        self.logger.debug("Connecting to %s ...", address)
-
-        if isinstance(address, tuple):
+        if isinstance(address, socket.socket):
+            self.logger.debug("Connecting with existing socket: "
+                              "fd=%d, family=%r, type=%r",
+                              address.fileno(), address.family, address.type)
+            connect = asyncio.open_connection(
+                limit=self._limit,
+                ssl=ssl,
+                sock=address,
+            )
+        elif isinstance(address, tuple):
+            self.logger.debug("Connecting to %s ...", address)
             connect = asyncio.open_connection(
                 address[0],
                 address[1],
@@ -644,13 +652,14 @@ async def _do_connect(self, address: SocketAddrT,
                 limit=self._limit,
             )
         else:
+            self.logger.debug("Connecting to file://%s ...", address)
             connect = asyncio.open_unix_connection(
                 path=address,
                 ssl=ssl,
                 limit=self._limit,
             )
+
         self._reader, self._writer = await connect
-
         self.logger.debug("Connected.")
 
     @upper_half
-- 
2.40.0

[PATCH 2/5] python/qmp/legacy: allow using sockets for connect()

[John Snow]

Instead of asserting that we have an address, allow the use of sockets
instead of addresses during a call to connect().

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

diff --git a/python/qemu/qmp/legacy.py b/python/qemu/qmp/legacy.py
index 8b09ee7dbb..b1eb3f360f 100644
--- a/python/qemu/qmp/legacy.py
+++ b/python/qemu/qmp/legacy.py
@@ -150,12 +150,13 @@ def connect(self, negotiate: bool = True) -> Optional[QMPMessage]:
         :return: QMP greeting dict, or None if negotiate is false
         :raise ConnectError: on connection errors
         """
-        assert self._address is not None
+        addr_or_sock = self._address or self._sock
+        assert addr_or_sock is not None
         self._qmp.await_greeting = negotiate
         self._qmp.negotiate = negotiate
 
         self._sync(
-            self._qmp.connect(self._address)
+            self._qmp.connect(addr_or_sock)
         )
         return self._get_greeting()
 
-- 
2.40.0

[PATCH 3/5] python/machine: use connect-based interface for existing sockets

[John Snow]

Instead of using accept() with sockets (which uses open_with_socket()),
use calls to connect() to utilize existing sockets instead. A benefit of
this is more robust error handling already present within the connect()
call that isn't present in open_with_socket().

Signed-off-by: John Snow <jsnow@redhat.com>
---
 python/qemu/machine/machine.py | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/python/qemu/machine/machine.py b/python/qemu/machine/machine.py
index e57c254484..cc636cb6bd 100644
--- a/python/qemu/machine/machine.py
+++ b/python/qemu/machine/machine.py
@@ -337,18 +337,17 @@ def _pre_launch(self) -> None:
             self._remove_files.append(self._console_address)
 
         if self._qmp_set:
-            monitor_address = None
             sock = None
             if self._monitor_address is None:
                 self._sock_pair = socket.socketpair()
                 sock = self._sock_pair[1]
             if isinstance(self._monitor_address, str):
                 self._remove_files.append(self._monitor_address)
-                monitor_address = self._monitor_address
+
             self._qmp_connection = QEMUMonitorProtocol(
-                address=monitor_address,
+                address=self._monitor_address,
                 sock=sock,
-                server=True,
+                server=bool(self._monitor_address),
                 nickname=self._name
             )
 
@@ -370,7 +369,10 @@ def _post_launch(self) -> None:
         if self._sock_pair:
             self._sock_pair[0].close()
         if self._qmp_connection:
-            self._qmp.accept(self._qmp_timer)
+            if self._sock_pair:
+                self._qmp.connect()
+            else:
+                self._qmp.accept(self._qmp_timer)
 
     def _close_qemu_log_file(self) -> None:
         if self._qemu_log_file is not None:
-- 
2.40.0

[PATCH 4/5] python/qmp/legacy: remove open_with_socket() calls

[John Snow]

Favor using connect() when passing a socket instead of
open_with_socket(). Simultaneously, update constructor calls to use the
combined address argument for QEMUMonitorProtocol().

Signed-off-by: John Snow <jsnow@redhat.com>
---
 python/qemu/machine/machine.py |  7 ++++---
 python/qemu/qmp/legacy.py      | 29 ++++++++++++-----------------
 2 files changed, 16 insertions(+), 20 deletions(-)

diff --git a/python/qemu/machine/machine.py b/python/qemu/machine/machine.py
index cc636cb6bd..c16a0b6fed 100644
--- a/python/qemu/machine/machine.py
+++ b/python/qemu/machine/machine.py
@@ -337,16 +337,17 @@ def _pre_launch(self) -> None:
             self._remove_files.append(self._console_address)
 
         if self._qmp_set:
-            sock = None
             if self._monitor_address is None:
                 self._sock_pair = socket.socketpair()
                 sock = self._sock_pair[1]
             if isinstance(self._monitor_address, str):
                 self._remove_files.append(self._monitor_address)
 
+            sock_or_addr = self._monitor_address or sock
+            assert sock_or_addr is not None
+
             self._qmp_connection = QEMUMonitorProtocol(
-                address=self._monitor_address,
-                sock=sock,
+                sock_or_addr,
                 server=bool(self._monitor_address),
                 nickname=self._name
             )
diff --git a/python/qemu/qmp/legacy.py b/python/qemu/qmp/legacy.py
index b1eb3f360f..e1e9383978 100644
--- a/python/qemu/qmp/legacy.py
+++ b/python/qemu/qmp/legacy.py
@@ -68,34 +68,31 @@ 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 or None
-    :param sock:     a socket or None
+    :param address:  QEMU address, can be a unix socket path (string), a tuple
+                     in the form ( address, port ) for a TCP connection, or an
+                     existing `socket.socket` object.
     :param server:   Act as the socket server. (See 'accept')
+                     Not applicable when passing a socket directly.
     :param nickname: Optional nickname used for logging.
     """
 
     def __init__(self,
-                 address: Optional[SocketAddrT] = None,
-                 sock: Optional[socket.socket] = None,
+                 address: Union[SocketAddrT, socket.socket],
                  server: bool = False,
                  nickname: Optional[str] = None):
 
-        assert address or sock
+        if server and isinstance(address, socket.socket):
+            raise ValueError(
+                "server argument should be False when passing a socket")
+
         self._qmp = QMPClient(nickname)
         self._aloop = asyncio.get_event_loop()
         self._address = address
-        self._sock = sock
         self._timeout: Optional[float] = None
 
         if server:
-            if sock:
-                assert self._sock is not None
-                self._sync(self._qmp.open_with_socket(self._sock))
-            else:
-                assert self._address is not None
-                self._sync(self._qmp.start_server(self._address))
+            assert not isinstance(self._address, socket.socket)
+            self._sync(self._qmp.start_server(self._address))
 
     _T = TypeVar('_T')
 
@@ -150,13 +147,11 @@ def connect(self, negotiate: bool = True) -> Optional[QMPMessage]:
         :return: QMP greeting dict, or None if negotiate is false
         :raise ConnectError: on connection errors
         """
-        addr_or_sock = self._address or self._sock
-        assert addr_or_sock is not None
         self._qmp.await_greeting = negotiate
         self._qmp.negotiate = negotiate
 
         self._sync(
-            self._qmp.connect(addr_or_sock)
+            self._qmp.connect(self._address)
         )
         return self._get_greeting()
 
-- 
2.40.0

[PATCH 5/5] Revert "python/qmp/protocol: add open_with_socket()"

[John Snow]

This reverts commit a3cfea92e2030926e00a2519d299384ea648e36e.

(It's being rolled back in favor of a different API, which brings the
in-tree and out-of-tree versions of qemu.qmp back in sync.)

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

diff --git a/python/qemu/qmp/protocol.py b/python/qemu/qmp/protocol.py
index d534db4631..753182131f 100644
--- a/python/qemu/qmp/protocol.py
+++ b/python/qemu/qmp/protocol.py
@@ -297,19 +297,6 @@ async def start_server_and_accept(
         await self.accept()
         assert self.runstate == Runstate.RUNNING
 
-    @upper_half
-    @require(Runstate.IDLE)
-    async def open_with_socket(self, sock: socket.socket) -> None:
-        """
-        Start connection with given socket.
-
-        :param sock: A socket.
-
-        :raise StateError: When the `Runstate` is not `IDLE`.
-        """
-        self._reader, self._writer = await asyncio.open_connection(sock=sock)
-        self._set_state(Runstate.CONNECTING)
-
     @upper_half
     @require(Runstate.IDLE)
     async def start_server(self, address: SocketAddrT,
@@ -357,12 +344,11 @@ async def accept(self) -> None:
             protocol-level failure occurs while establishing a new
             session, the wrapped error may also be an `QMPError`.
         """
-        if not self._reader:
-            if self._accepted is None:
-                raise QMPError("Cannot call accept() before start_server().")
-            await self._session_guard(
-                self._do_accept(),
-                'Failed to establish connection')
+        if self._accepted is None:
+            raise QMPError("Cannot call accept() before start_server().")
+        await self._session_guard(
+            self._do_accept(),
+            'Failed to establish connection')
         await self._session_guard(
             self._establish_session(),
             'Failed to establish session')
-- 
2.40.0

Re: [PATCH 0/5] python: backport socket changes from python-qemu-qmp

[John Snow]

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

On Wed, May 17, 2023, 12:34 PM John Snow <jsnow@redhat.com> wrote:

> This is a small patchset designed to backport the changes made to the
> qemu.qmp code to utilize pre-existing sockets. This contains some small
> changes to machine.py to match the new API. This is necessary to do
> before dropping qemu.qmp from qemu.git so we can utilize the future
> 0.0.3 version of qemu.qmp.
>
> (This should also fix the bug where the buffering limit was not being
> applied properly


^to

connections utilizing pre-existing sockets.)
>
> John Snow (5):
>   python/qmp: allow sockets to be passed to connect()
>   python/qmp/legacy: allow using sockets for connect()
>   python/machine: use connect-based interface for existing sockets
>   python/qmp/legacy: remove open_with_socket() calls
>   Revert "python/qmp/protocol: add open_with_socket()"
>
>  python/qemu/machine/machine.py | 17 +++++++------
>  python/qemu/qmp/legacy.py      | 26 +++++++++-----------
>  python/qemu/qmp/protocol.py    | 45 +++++++++++++++-------------------
>  3 files changed, 41 insertions(+), 47 deletions(-)
>
> --
> 2.40.0
>

ping - I'll send a PR for this on Friday if no objections. It just
synchronizes the two codebases ahead of my plan to finally delete the
duplication once and for all.

(and *that* series will need some good review and attention.)

--js

>

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