[PATCH v3 0/3] python/qemu/machine: fix potential hang in QMP accept

[PATCH v3 0/3] python/qemu/machine: fix potential hang in QMP accept

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

Hi,

As reported earlier by Richard Henderson ("virgl avocado hang" thread), avocado
tests may hang when QEMU exits before the QMP connection is established.

v3:
 - after merge in https://gitlab.com/qemu-project/python-qemu-qmp
 - resend as requested by John Snow

v2:
 - use a socketpair() for QMP (instead of async concurrent code from v1) as
   suggested by Daniel Berrange.
 - should not regress (hopefully)

Marc-André Lureau (3):
  python/qmp/protocol: add open_with_socket()
  python/qmp/legacy: make QEMUMonitorProtocol accept a socket
  python/qemu/machine: use socketpair() for QMP by default

 python/qemu/machine/machine.py | 24 ++++++++++++++++--------
 python/qemu/qmp/legacy.py      | 18 +++++++++++++++---
 python/qemu/qmp/protocol.py    | 25 ++++++++++++++++++++-----
 3 files changed, 51 insertions(+), 16 deletions(-)

-- 
2.39.0

[PATCH v3 1/3] python/qmp/protocol: add open_with_socket()

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

Instead of listening for incoming connections with a SocketAddr, add a
new method open_with_socket() that accepts an existing socket.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
---
 python/qemu/qmp/protocol.py | 25 ++++++++++++++++++++-----
 1 file changed, 20 insertions(+), 5 deletions(-)

diff --git a/python/qemu/qmp/protocol.py b/python/qemu/qmp/protocol.py
index 6ea86650ad..4710a57f91 100644
--- a/python/qemu/qmp/protocol.py
+++ b/python/qemu/qmp/protocol.py
@@ -18,6 +18,7 @@
 from enum import Enum
 from functools import wraps
 import logging
+import socket
 from ssl import SSLContext
 from typing import (
     Any,
@@ -296,6 +297,19 @@ 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,
@@ -343,11 +357,12 @@ async def accept(self) -> None:
             protocol-level failure occurs while establishing a new
             session, the wrapped error may also be an `QMPError`.
         """
-        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 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')
         await self._session_guard(
             self._establish_session(),
             'Failed to establish session')
-- 
2.39.0

[PATCH v3 2/3] python/qmp/legacy: make QEMUMonitorProtocol accept a socket

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

Teach QEMUMonitorProtocol to accept an exisiting socket.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
---
 python/qemu/qmp/legacy.py | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

diff --git a/python/qemu/qmp/legacy.py b/python/qemu/qmp/legacy.py
index 1951754455..8b09ee7dbb 100644
--- a/python/qemu/qmp/legacy.py
+++ b/python/qemu/qmp/legacy.py
@@ -22,6 +22,7 @@
 #
 
 import asyncio
+import socket
 from types import TracebackType
 from typing import (
     Any,
@@ -69,22 +70,32 @@ class QEMUMonitorProtocol:
 
     :param address:  QEMU address, can be either a unix socket path (string)
                      or a tuple in the form ( address, port ) for a TCP
-                     connection
+                     connection or None
+    :param sock:     a socket or None
     :param server:   Act as the socket server. (See 'accept')
     :param nickname: Optional nickname used for logging.
     """
 
-    def __init__(self, address: SocketAddrT,
+    def __init__(self,
+                 address: Optional[SocketAddrT] = None,
+                 sock: Optional[socket.socket] = None,
                  server: bool = False,
                  nickname: Optional[str] = None):
 
+        assert address or sock
         self._qmp = QMPClient(nickname)
         self._aloop = asyncio.get_event_loop()
         self._address = address
+        self._sock = sock
         self._timeout: Optional[float] = None
 
         if server:
-            self._sync(self._qmp.start_server(self._address))
+            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))
 
     _T = TypeVar('_T')
 
@@ -139,6 +150,7 @@ 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
         self._qmp.await_greeting = negotiate
         self._qmp.negotiate = negotiate
 
-- 
2.39.0

[PATCH v3 3/3] python/qemu/machine: use socketpair() for QMP by default

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

When no monitor address is given, establish the QMP communication through
a socketpair() (API is also supported on Windows since Python 3.5)

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
---
 python/qemu/machine/machine.py | 24 ++++++++++++++++--------
 1 file changed, 16 insertions(+), 8 deletions(-)

diff --git a/python/qemu/machine/machine.py b/python/qemu/machine/machine.py
index 748a0d807c..5b2e499e68 100644
--- a/python/qemu/machine/machine.py
+++ b/python/qemu/machine/machine.py
@@ -158,17 +158,13 @@ def __init__(self,
         self._qmp_timer = qmp_timer
 
         self._name = name or f"qemu-{os.getpid()}-{id(self):02x}"
+        self._sock_pair: Optional[Tuple[socket.socket, socket.socket]] = None
         self._temp_dir: Optional[str] = None
         self._base_temp_dir = base_temp_dir
         self._sock_dir = sock_dir
         self._log_dir = log_dir
 
-        if monitor_address is not None:
-            self._monitor_address = monitor_address
-        else:
-            self._monitor_address = os.path.join(
-                self.sock_dir, f"{self._name}-monitor.sock"
-            )
+        self._monitor_address = monitor_address
 
         self._console_log_path = console_log
         if self._console_log_path:
@@ -303,7 +299,11 @@ def _base_args(self) -> List[str]:
         args = ['-display', 'none', '-vga', 'none']
 
         if self._qmp_set:
-            if isinstance(self._monitor_address, tuple):
+            if self._sock_pair:
+                fd = self._sock_pair[0].fileno()
+                os.set_inheritable(fd, True)
+                moncdev = f"socket,id=mon,fd={fd}"
+            elif isinstance(self._monitor_address, tuple):
                 moncdev = "socket,id=mon,host={},port={}".format(
                     *self._monitor_address
                 )
@@ -337,10 +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(
-                self._monitor_address,
+                address=monitor_address,
+                sock=sock,
                 server=True,
                 nickname=self._name
             )
@@ -360,6 +367,7 @@ def _pre_launch(self) -> None:
         ))
 
     def _post_launch(self) -> None:
+        self._sock_pair[0].close()
         if self._qmp_connection:
             self._qmp.accept(self._qmp_timer)
 
-- 
2.39.0

Re: [PATCH v3 3/3] python/qemu/machine: use socketpair() for QMP by default

[John Snow]

On Wed, Jan 11, 2023 at 3:01 AM <marcandre.lureau@redhat.com> wrote:
>
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> When no monitor address is given, establish the QMP communication through
> a socketpair() (API is also supported on Windows since Python 3.5)
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>  python/qemu/machine/machine.py | 24 ++++++++++++++++--------
>  1 file changed, 16 insertions(+), 8 deletions(-)
>
> diff --git a/python/qemu/machine/machine.py b/python/qemu/machine/machine.py
> index 748a0d807c..5b2e499e68 100644
> --- a/python/qemu/machine/machine.py
> +++ b/python/qemu/machine/machine.py
> @@ -158,17 +158,13 @@ def __init__(self,
>          self._qmp_timer = qmp_timer
>
>          self._name = name or f"qemu-{os.getpid()}-{id(self):02x}"
> +        self._sock_pair: Optional[Tuple[socket.socket, socket.socket]] = None
>          self._temp_dir: Optional[str] = None
>          self._base_temp_dir = base_temp_dir
>          self._sock_dir = sock_dir
>          self._log_dir = log_dir
>
> -        if monitor_address is not None:
> -            self._monitor_address = monitor_address
> -        else:
> -            self._monitor_address = os.path.join(
> -                self.sock_dir, f"{self._name}-monitor.sock"
> -            )
> +        self._monitor_address = monitor_address
>
>          self._console_log_path = console_log
>          if self._console_log_path:
> @@ -303,7 +299,11 @@ def _base_args(self) -> List[str]:
>          args = ['-display', 'none', '-vga', 'none']
>
>          if self._qmp_set:
> -            if isinstance(self._monitor_address, tuple):
> +            if self._sock_pair:
> +                fd = self._sock_pair[0].fileno()
> +                os.set_inheritable(fd, True)
> +                moncdev = f"socket,id=mon,fd={fd}"
> +            elif isinstance(self._monitor_address, tuple):
>                  moncdev = "socket,id=mon,host={},port={}".format(
>                      *self._monitor_address
>                  )
> @@ -337,10 +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(
> -                self._monitor_address,
> +                address=monitor_address,
> +                sock=sock,
>                  server=True,
>                  nickname=self._name
>              )
> @@ -360,6 +367,7 @@ def _pre_launch(self) -> None:
>          ))
>
>      def _post_launch(self) -> None:
> +        self._sock_pair[0].close()

Needs an assert or an error-check here for _sock_pair to be non-None,
otherwise mypy will shout. Try running "make check-dev" from
qemu.git/python directory as a smoke test.


>          if self._qmp_connection:
>              self._qmp.accept(self._qmp_timer)
>
> --
> 2.39.0
>

Re: [PATCH v3 3/3] python/qemu/machine: use socketpair() for QMP by default

[Marc-André Lureau]

Hi

On Wed, Jan 18, 2023 at 2:36 AM John Snow <jsnow@redhat.com> wrote:
>
> On Wed, Jan 11, 2023 at 3:01 AM <marcandre.lureau@redhat.com> wrote:
> >
> > From: Marc-André Lureau <marcandre.lureau@redhat.com>
> >
> > When no monitor address is given, establish the QMP communication through
> > a socketpair() (API is also supported on Windows since Python 3.5)
> >
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
> > ---
> >  python/qemu/machine/machine.py | 24 ++++++++++++++++--------
> >  1 file changed, 16 insertions(+), 8 deletions(-)
> >
> > diff --git a/python/qemu/machine/machine.py b/python/qemu/machine/machine.py
> > index 748a0d807c..5b2e499e68 100644
> > --- a/python/qemu/machine/machine.py
> > +++ b/python/qemu/machine/machine.py
> > @@ -158,17 +158,13 @@ def __init__(self,
> >          self._qmp_timer = qmp_timer
> >
> >          self._name = name or f"qemu-{os.getpid()}-{id(self):02x}"
> > +        self._sock_pair: Optional[Tuple[socket.socket, socket.socket]] = None
> >          self._temp_dir: Optional[str] = None
> >          self._base_temp_dir = base_temp_dir
> >          self._sock_dir = sock_dir
> >          self._log_dir = log_dir
> >
> > -        if monitor_address is not None:
> > -            self._monitor_address = monitor_address
> > -        else:
> > -            self._monitor_address = os.path.join(
> > -                self.sock_dir, f"{self._name}-monitor.sock"
> > -            )
> > +        self._monitor_address = monitor_address
> >
> >          self._console_log_path = console_log
> >          if self._console_log_path:
> > @@ -303,7 +299,11 @@ def _base_args(self) -> List[str]:
> >          args = ['-display', 'none', '-vga', 'none']
> >
> >          if self._qmp_set:
> > -            if isinstance(self._monitor_address, tuple):
> > +            if self._sock_pair:
> > +                fd = self._sock_pair[0].fileno()
> > +                os.set_inheritable(fd, True)
> > +                moncdev = f"socket,id=mon,fd={fd}"
> > +            elif isinstance(self._monitor_address, tuple):
> >                  moncdev = "socket,id=mon,host={},port={}".format(
> >                      *self._monitor_address
> >                  )
> > @@ -337,10 +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(
> > -                self._monitor_address,
> > +                address=monitor_address,
> > +                sock=sock,
> >                  server=True,
> >                  nickname=self._name
> >              )
> > @@ -360,6 +367,7 @@ def _pre_launch(self) -> None:
> >          ))
> >
> >      def _post_launch(self) -> None:
> > +        self._sock_pair[0].close()
>
> Needs an assert or an error-check here for _sock_pair to be non-None,
> otherwise mypy will shout. Try running "make check-dev" from
> qemu.git/python directory as a smoke test.

Good catch, it should be:

+        if self._sock_pair:
+            self._sock_pair[0].close()

Let me know if you want me to resend the whole series, or if you can
touch it during picking.


-- 
Marc-André Lureau

Re: [PATCH v3 3/3] python/qemu/machine: use socketpair() for QMP by default

[John Snow]

On Wed, Jan 18, 2023 at 3:03 AM Marc-André Lureau
<marcandre.lureau@gmail.com> wrote:
>
> Hi
>
> On Wed, Jan 18, 2023 at 2:36 AM John Snow <jsnow@redhat.com> wrote:
> >
> > On Wed, Jan 11, 2023 at 3:01 AM <marcandre.lureau@redhat.com> wrote:
> > >
> > > From: Marc-André Lureau <marcandre.lureau@redhat.com>
> > >
> > > When no monitor address is given, establish the QMP communication through
> > > a socketpair() (API is also supported on Windows since Python 3.5)
> > >
> > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > > Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
> > > ---
> > >  python/qemu/machine/machine.py | 24 ++++++++++++++++--------
> > >  1 file changed, 16 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/python/qemu/machine/machine.py b/python/qemu/machine/machine.py
> > > index 748a0d807c..5b2e499e68 100644
> > > --- a/python/qemu/machine/machine.py
> > > +++ b/python/qemu/machine/machine.py
> > > @@ -158,17 +158,13 @@ def __init__(self,
> > >          self._qmp_timer = qmp_timer
> > >
> > >          self._name = name or f"qemu-{os.getpid()}-{id(self):02x}"
> > > +        self._sock_pair: Optional[Tuple[socket.socket, socket.socket]] = None
> > >          self._temp_dir: Optional[str] = None
> > >          self._base_temp_dir = base_temp_dir
> > >          self._sock_dir = sock_dir
> > >          self._log_dir = log_dir
> > >
> > > -        if monitor_address is not None:
> > > -            self._monitor_address = monitor_address
> > > -        else:
> > > -            self._monitor_address = os.path.join(
> > > -                self.sock_dir, f"{self._name}-monitor.sock"
> > > -            )
> > > +        self._monitor_address = monitor_address
> > >
> > >          self._console_log_path = console_log
> > >          if self._console_log_path:
> > > @@ -303,7 +299,11 @@ def _base_args(self) -> List[str]:
> > >          args = ['-display', 'none', '-vga', 'none']
> > >
> > >          if self._qmp_set:
> > > -            if isinstance(self._monitor_address, tuple):
> > > +            if self._sock_pair:
> > > +                fd = self._sock_pair[0].fileno()
> > > +                os.set_inheritable(fd, True)
> > > +                moncdev = f"socket,id=mon,fd={fd}"
> > > +            elif isinstance(self._monitor_address, tuple):
> > >                  moncdev = "socket,id=mon,host={},port={}".format(
> > >                      *self._monitor_address
> > >                  )
> > > @@ -337,10 +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(
> > > -                self._monitor_address,
> > > +                address=monitor_address,
> > > +                sock=sock,
> > >                  server=True,
> > >                  nickname=self._name
> > >              )
> > > @@ -360,6 +367,7 @@ def _pre_launch(self) -> None:
> > >          ))
> > >
> > >      def _post_launch(self) -> None:
> > > +        self._sock_pair[0].close()
> >
> > Needs an assert or an error-check here for _sock_pair to be non-None,
> > otherwise mypy will shout. Try running "make check-dev" from
> > qemu.git/python directory as a smoke test.
>
> Good catch, it should be:
>
> +        if self._sock_pair:
> +            self._sock_pair[0].close()
>
> Let me know if you want me to resend the whole series, or if you can
> touch it during picking.

Touching it up during picking, running tests, PR soon. Thanks,
--js

Re: [PATCH v3 3/3] python/qemu/machine: use socketpair() for QMP by default

[Vladimir Sementsov-Ogievskiy]

Hi!

By my investigation, this commit (bd4c0ef409140bd1be393407c04005ac077d4574) breaks long qmp output again.

Simple test:

$ cd python
$ cat test.py
#!/usr/bin/env python3

import sys
from qemu.machine import QEMUMachine

monitor_address = sys.argv[2] if len(sys.argv) > 2 else None
vm = QEMUMachine('../build/qemu-system-x86_64',
                  monitor_address=monitor_address)
vm.launch()

for x in range(int(sys.argv[1])):
     vm.qmp('blockdev-add', {'driver': 'null-co', 'node-name': f'x{x}'})

vm.qmp('query-named-block-nodes')
vm.shutdown()


./test.py 1000 /tmp/sock

   - works, but if use default behavior (socketpair) we get:

$ ./test.py 1000
Traceback (most recent call last):
   File "/home/vsementsov/work/src/qemu/master/python/./test.py", line 14, in <module>
     vm.qmp('query-named-block-nodes')
   File "/home/vsementsov/work/src/qemu/master/python/qemu/machine/machine.py", line 686, in qmp
     ret = self._qmp.cmd(cmd, args=qmp_args)
   File "/home/vsementsov/work/src/qemu/master/python/qemu/qmp/legacy.py", line 216, in cmd
     return self.cmd_obj(qmp_cmd)
   File "/home/vsementsov/work/src/qemu/master/python/qemu/qmp/legacy.py", line 190, in cmd_obj
     self._sync(
   File "/home/vsementsov/work/src/qemu/master/python/qemu/qmp/legacy.py", line 105, in _sync
     return self._aloop.run_until_complete(
   File "/usr/lib/python3.10/asyncio/base_events.py", line 646, in run_until_complete
     return future.result()
   File "/usr/lib/python3.10/asyncio/tasks.py", line 408, in wait_for
     return await fut
   File "/home/vsementsov/work/src/qemu/master/python/qemu/qmp/qmp_client.py", line 547, in _raw
     return await self._execute(msg, assign_id=assign_id)
   File "/home/vsementsov/work/src/qemu/master/python/qemu/qmp/qmp_client.py", line 496, in _execute
     return await self._reply(exec_id)
   File "/home/vsementsov/work/src/qemu/master/python/qemu/qmp/qmp_client.py", line 463, in _reply
     raise result
qemu.qmp.qmp_client.ExecInterruptedError: Disconnected
Exception ignored in: <function QEMUMonitorProtocol.__del__ at 0x7f8708283eb0>
Traceback (most recent call last):
   File "/home/vsementsov/work/src/qemu/master/python/qemu/qmp/legacy.py", line 318, in __del__
   File "/home/vsementsov/work/src/qemu/master/python/qemu/qmp/legacy.py", line 289, in close
   File "/home/vsementsov/work/src/qemu/master/python/qemu/qmp/legacy.py", line 105, in _sync
   File "/usr/lib/python3.10/asyncio/base_events.py", line 646, in run_until_complete
   File "/usr/lib/python3.10/asyncio/tasks.py", line 408, in wait_for
   File "/home/vsementsov/work/src/qemu/master/python/qemu/qmp/protocol.py", line 413, in disconnect
   File "/home/vsementsov/work/src/qemu/master/python/qemu/qmp/protocol.py", line 725, in _wait_disconnect
   File "/home/vsementsov/work/src/qemu/master/python/qemu/qmp/protocol.py", line 876, in _bh_loop_forever
   File "/home/vsementsov/work/src/qemu/master/python/qemu/qmp/protocol.py", line 914, in _bh_recv_message
   File "/home/vsementsov/work/src/qemu/master/python/qemu/qmp/protocol.py", line 1015, in _recv
   File "/home/vsementsov/work/src/qemu/master/python/qemu/qmp/qmp_client.py", line 402, in _do_recv
   File "/home/vsementsov/work/src/qemu/master/python/qemu/qmp/protocol.py", line 979, in _readline
   File "/usr/lib/python3.10/asyncio/streams.py", line 534, in readline
ValueError: Separator is not found, and chunk exceed the limit



./test.py 100
   - works well.


On 11.01.23 11:01, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> When no monitor address is given, establish the QMP communication through
> a socketpair() (API is also supported on Windows since Python 3.5)
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>   python/qemu/machine/machine.py | 24 ++++++++++++++++--------
>   1 file changed, 16 insertions(+), 8 deletions(-)
> 
> diff --git a/python/qemu/machine/machine.py b/python/qemu/machine/machine.py
> index 748a0d807c..5b2e499e68 100644
> --- a/python/qemu/machine/machine.py
> +++ b/python/qemu/machine/machine.py
> @@ -158,17 +158,13 @@ def __init__(self,
>           self._qmp_timer = qmp_timer
>   
>           self._name = name or f"qemu-{os.getpid()}-{id(self):02x}"
> +        self._sock_pair: Optional[Tuple[socket.socket, socket.socket]] = None
>           self._temp_dir: Optional[str] = None
>           self._base_temp_dir = base_temp_dir
>           self._sock_dir = sock_dir
>           self._log_dir = log_dir
>   
> -        if monitor_address is not None:
> -            self._monitor_address = monitor_address
> -        else:
> -            self._monitor_address = os.path.join(
> -                self.sock_dir, f"{self._name}-monitor.sock"
> -            )
> +        self._monitor_address = monitor_address
>   
>           self._console_log_path = console_log
>           if self._console_log_path:
> @@ -303,7 +299,11 @@ def _base_args(self) -> List[str]:
>           args = ['-display', 'none', '-vga', 'none']
>   
>           if self._qmp_set:
> -            if isinstance(self._monitor_address, tuple):
> +            if self._sock_pair:
> +                fd = self._sock_pair[0].fileno()
> +                os.set_inheritable(fd, True)
> +                moncdev = f"socket,id=mon,fd={fd}"
> +            elif isinstance(self._monitor_address, tuple):
>                   moncdev = "socket,id=mon,host={},port={}".format(
>                       *self._monitor_address
>                   )
> @@ -337,10 +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(
> -                self._monitor_address,
> +                address=monitor_address,
> +                sock=sock,
>                   server=True,
>                   nickname=self._name
>               )
> @@ -360,6 +367,7 @@ def _pre_launch(self) -> None:
>           ))
>   
>       def _post_launch(self) -> None:
> +        self._sock_pair[0].close()
>           if self._qmp_connection:
>               self._qmp.accept(self._qmp_timer)
>   

-- 
Best regards,
Vladimir

Re: [PATCH v3 3/3] python/qemu/machine: use socketpair() for QMP by default

[Daniel P. Berrangé]

On Fri, Mar 17, 2023 at 10:36:37PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> Hi!
> 
> By my investigation, this commit (bd4c0ef409140bd1be393407c04005ac077d4574) breaks long qmp output again.

> 
> ./test.py 1000 /tmp/sock
> 
>   - works, but if use default behavior (socketpair) we get:
> 
> $ ./test.py 1000
> Traceback (most recent call last):

snip

>   File "/usr/lib/python3.10/asyncio/streams.py", line 534, in readline
> ValueError: Separator is not found, and chunk exceed the limit

After going off in the weeds I realized this message is the key bit. We
failed to pass the raised recv limit to asyncio when using a pre-opened
socket. Since the QMP reply was greater than 64kb asyncio raised an
exception. This was a pre-existing latent bug, exposed with the patch
to enable use of socketpair(). I've CC'd you on a patch to fix this.

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