Message ID | 20230111080101.969151-4-marcandre.lureau@redhat.com |
---|---|
State | New |
Headers | show |
Series | python/qemu/machine: fix potential hang in QMP accept | expand |
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 >
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.
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
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) >
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
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)