* [PATCH 0/4] python/machine: use socketpair() for console socket
@ 2023-07-20 13:04 John Snow
2023-07-20 13:04 ` [PATCH 1/4] python/machine: move socket setup out of _base_args property John Snow
` (4 more replies)
0 siblings, 5 replies; 12+ messages in thread
From: John Snow @ 2023-07-20 13:04 UTC (permalink / raw)
To: qemu-devel
Cc: John Snow, qemu-block, Beraldo Leal, Hanna Reitz, Kevin Wolf,
Peter Maydell, Daniel Berrange, Cleber Rosa
Like we did for the QMP socket, use socketpair() for the console socket
so that hopefully there isn't a race condition during early boot where
data might get dropped on the floor.
"lightly tested"; passes local tests and gitlab CI. Doesn't seem to make
anything worse.
John Snow (4):
python/machine: move socket setup out of _base_args property
python/console_socket: accept existing FD in initializer
python/machine: use socketpair() for console connections
python/machine: remove unused console socket configuration arguments
python/qemu/machine/console_socket.py | 11 +++---
python/qemu/machine/machine.py | 40 +++++++++-------------
python/qemu/machine/qtest.py | 6 ++--
tests/qemu-iotests/tests/copy-before-write | 3 +-
4 files changed, 27 insertions(+), 33 deletions(-)
--
2.41.0
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 1/4] python/machine: move socket setup out of _base_args property
2023-07-20 13:04 [PATCH 0/4] python/machine: use socketpair() for console socket John Snow
@ 2023-07-20 13:04 ` John Snow
2023-07-20 13:55 ` Daniel P. Berrangé
2023-07-20 13:04 ` [PATCH 2/4] python/console_socket: accept existing FD in initializer John Snow
` (3 subsequent siblings)
4 siblings, 1 reply; 12+ messages in thread
From: John Snow @ 2023-07-20 13:04 UTC (permalink / raw)
To: qemu-devel
Cc: John Snow, qemu-block, Beraldo Leal, Hanna Reitz, Kevin Wolf,
Peter Maydell, Daniel Berrange, Cleber Rosa
This property isn't meant to do much else besides return a list of
strings, so move this setup back out into _pre_launch().
Signed-off-by: John Snow <jsnow@redhat.com>
---
python/qemu/machine/machine.py | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/python/qemu/machine/machine.py b/python/qemu/machine/machine.py
index c16a0b6fed..8be0f684fe 100644
--- a/python/qemu/machine/machine.py
+++ b/python/qemu/machine/machine.py
@@ -300,9 +300,7 @@ def _base_args(self) -> List[str]:
if self._qmp_set:
if self._sock_pair:
- fd = self._sock_pair[0].fileno()
- os.set_inheritable(fd, True)
- moncdev = f"socket,id=mon,fd={fd}"
+ moncdev = f"socket,id=mon,fd={self._sock_pair[0].fileno()}"
elif isinstance(self._monitor_address, tuple):
moncdev = "socket,id=mon,host={},port={}".format(
*self._monitor_address
@@ -339,6 +337,7 @@ def _pre_launch(self) -> None:
if self._qmp_set:
if self._monitor_address is None:
self._sock_pair = socket.socketpair()
+ os.set_inheritable(self._sock_pair[0].fileno(), True)
sock = self._sock_pair[1]
if isinstance(self._monitor_address, str):
self._remove_files.append(self._monitor_address)
--
2.41.0
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH 2/4] python/console_socket: accept existing FD in initializer
2023-07-20 13:04 [PATCH 0/4] python/machine: use socketpair() for console socket John Snow
2023-07-20 13:04 ` [PATCH 1/4] python/machine: move socket setup out of _base_args property John Snow
@ 2023-07-20 13:04 ` John Snow
2023-07-20 14:01 ` Daniel P. Berrangé
2023-07-20 13:04 ` [PATCH 3/4] python/machine: use socketpair() for console connections John Snow
` (2 subsequent siblings)
4 siblings, 1 reply; 12+ messages in thread
From: John Snow @ 2023-07-20 13:04 UTC (permalink / raw)
To: qemu-devel
Cc: John Snow, qemu-block, Beraldo Leal, Hanna Reitz, Kevin Wolf,
Peter Maydell, Daniel Berrange, Cleber Rosa
Useful if we want to use ConsoleSocket() for a socket created by
socketpair().
Signed-off-by: John Snow <jsnow@redhat.com>
---
python/qemu/machine/console_socket.py | 11 +++++++----
1 file changed, 7 insertions(+), 4 deletions(-)
diff --git a/python/qemu/machine/console_socket.py b/python/qemu/machine/console_socket.py
index 4e28ba9bb2..42bfa12411 100644
--- a/python/qemu/machine/console_socket.py
+++ b/python/qemu/machine/console_socket.py
@@ -17,7 +17,7 @@
import socket
import threading
import time
-from typing import Deque, Optional
+from typing import Deque, Optional, Union
class ConsoleSocket(socket.socket):
@@ -30,13 +30,16 @@ class ConsoleSocket(socket.socket):
Optionally a file path can be passed in and we will also
dump the characters to this file for debugging purposes.
"""
- def __init__(self, address: str, file: Optional[str] = None,
+ def __init__(self, address: Union[str, int], file: Optional[str] = None,
drain: bool = False):
self._recv_timeout_sec = 300.0
self._sleep_time = 0.5
self._buffer: Deque[int] = deque()
- socket.socket.__init__(self, socket.AF_UNIX, socket.SOCK_STREAM)
- self.connect(address)
+ if isinstance(address, str):
+ socket.socket.__init__(self, socket.AF_UNIX, socket.SOCK_STREAM)
+ self.connect(address)
+ else:
+ socket.socket.__init__(self, fileno=address)
self._logfile = None
if file:
# pylint: disable=consider-using-with
--
2.41.0
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH 3/4] python/machine: use socketpair() for console connections
2023-07-20 13:04 [PATCH 0/4] python/machine: use socketpair() for console socket John Snow
2023-07-20 13:04 ` [PATCH 1/4] python/machine: move socket setup out of _base_args property John Snow
2023-07-20 13:04 ` [PATCH 2/4] python/console_socket: accept existing FD in initializer John Snow
@ 2023-07-20 13:04 ` John Snow
2023-07-20 14:02 ` Daniel P. Berrangé
2023-07-20 13:04 ` [PATCH 4/4] python/machine: remove unused console socket configuration arguments John Snow
2023-07-20 13:45 ` [PATCH 0/4] python/machine: use socketpair() for console socket Peter Maydell
4 siblings, 1 reply; 12+ messages in thread
From: John Snow @ 2023-07-20 13:04 UTC (permalink / raw)
To: qemu-devel
Cc: John Snow, qemu-block, Beraldo Leal, Hanna Reitz, Kevin Wolf,
Peter Maydell, Daniel Berrange, Cleber Rosa
Create a socketpair for the console output. This should help eliminate
race conditions around console text early in the boot process that might
otherwise have been dropped on the floor before being able to connect to
QEMU under "server,nowait".
Signed-off-by: John Snow <jsnow@redhat.com>
---
python/qemu/machine/machine.py | 17 ++++++++++++++---
1 file changed, 14 insertions(+), 3 deletions(-)
diff --git a/python/qemu/machine/machine.py b/python/qemu/machine/machine.py
index 8be0f684fe..ef9b2dc02e 100644
--- a/python/qemu/machine/machine.py
+++ b/python/qemu/machine/machine.py
@@ -159,6 +159,8 @@ def __init__(self,
self._name = name or f"{id(self):x}"
self._sock_pair: Optional[Tuple[socket.socket, socket.socket]] = None
+ self._cons_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
@@ -315,8 +317,9 @@ def _base_args(self) -> List[str]:
for _ in range(self._console_index):
args.extend(['-serial', 'null'])
if self._console_set:
- chardev = ('socket,id=console,path=%s,server=on,wait=off' %
- self._console_address)
+ assert self._cons_sock_pair is not None
+ fd = self._cons_sock_pair[0].fileno()
+ chardev = f"socket,id=console,fd={fd}"
args.extend(['-chardev', chardev])
if self._console_device_type is None:
args.extend(['-serial', 'chardev:console'])
@@ -351,6 +354,10 @@ def _pre_launch(self) -> None:
nickname=self._name
)
+ if self._console_set:
+ self._cons_sock_pair = socket.socketpair()
+ os.set_inheritable(self._cons_sock_pair[0].fileno(), True)
+
# NOTE: Make sure any opened resources are *definitely* freed in
# _post_shutdown()!
# pylint: disable=consider-using-with
@@ -873,8 +880,12 @@ def console_socket(self) -> socket.socket:
Returns a socket connected to the console
"""
if self._console_socket is None:
+ if not self._console_set:
+ raise QEMUMachineError(
+ "Attempt to access console socket with no connection")
+ assert self._cons_sock_pair is not None
self._console_socket = console_socket.ConsoleSocket(
- self._console_address,
+ self._cons_sock_pair[1].fileno(),
file=self._console_log_path,
drain=self._drain_console)
return self._console_socket
--
2.41.0
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH 4/4] python/machine: remove unused console socket configuration arguments
2023-07-20 13:04 [PATCH 0/4] python/machine: use socketpair() for console socket John Snow
` (2 preceding siblings ...)
2023-07-20 13:04 ` [PATCH 3/4] python/machine: use socketpair() for console connections John Snow
@ 2023-07-20 13:04 ` John Snow
2023-07-20 14:04 ` Daniel P. Berrangé
2023-07-20 13:45 ` [PATCH 0/4] python/machine: use socketpair() for console socket Peter Maydell
4 siblings, 1 reply; 12+ messages in thread
From: John Snow @ 2023-07-20 13:04 UTC (permalink / raw)
To: qemu-devel
Cc: John Snow, qemu-block, Beraldo Leal, Hanna Reitz, Kevin Wolf,
Peter Maydell, Daniel Berrange, Cleber Rosa
By using a socketpair for the console, we don't need the sock_dir
argument for the base class anymore, remove it.
The qtest subclass still uses the argument for the qtest socket for now.
Signed-off-by: John Snow <jsnow@redhat.com>
---
python/qemu/machine/machine.py | 18 ------------------
python/qemu/machine/qtest.py | 6 +++---
tests/qemu-iotests/tests/copy-before-write | 3 +--
3 files changed, 4 insertions(+), 23 deletions(-)
diff --git a/python/qemu/machine/machine.py b/python/qemu/machine/machine.py
index ef9b2dc02e..350aa8bb26 100644
--- a/python/qemu/machine/machine.py
+++ b/python/qemu/machine/machine.py
@@ -127,7 +127,6 @@ def __init__(self,
name: Optional[str] = None,
base_temp_dir: str = "/var/tmp",
monitor_address: Optional[SocketAddrT] = None,
- sock_dir: Optional[str] = None,
drain_console: bool = False,
console_log: Optional[str] = None,
log_dir: Optional[str] = None,
@@ -141,7 +140,6 @@ def __init__(self,
@param name: prefix for socket and log file names (default: qemu-PID)
@param base_temp_dir: default location where temp files are created
@param monitor_address: address for QMP monitor
- @param sock_dir: where to create socket (defaults to base_temp_dir)
@param drain_console: (optional) True to drain console socket to buffer
@param console_log: (optional) path to console log file
@param log_dir: where to create and keep log files
@@ -163,7 +161,6 @@ def __init__(self,
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
self._monitor_address = monitor_address
@@ -189,9 +186,6 @@ def __init__(self,
self._console_index = 0
self._console_set = False
self._console_device_type: Optional[str] = None
- self._console_address = os.path.join(
- self.sock_dir, f"{self._name}.con"
- )
self._console_socket: Optional[socket.socket] = None
self._remove_files: List[str] = []
self._user_killed = False
@@ -334,9 +328,6 @@ def args(self) -> List[str]:
return self._args
def _pre_launch(self) -> None:
- if self._console_set:
- self._remove_files.append(self._console_address)
-
if self._qmp_set:
if self._monitor_address is None:
self._sock_pair = socket.socketpair()
@@ -900,15 +891,6 @@ def temp_dir(self) -> str:
dir=self._base_temp_dir)
return self._temp_dir
- @property
- def sock_dir(self) -> str:
- """
- Returns the directory used for sockfiles by this machine.
- """
- if self._sock_dir:
- return self._sock_dir
- return self.temp_dir
-
@property
def log_dir(self) -> str:
"""
diff --git a/python/qemu/machine/qtest.py b/python/qemu/machine/qtest.py
index 1c46138bd0..22f8045ef6 100644
--- a/python/qemu/machine/qtest.py
+++ b/python/qemu/machine/qtest.py
@@ -115,8 +115,8 @@ def __init__(self,
wrapper: Sequence[str] = (),
name: Optional[str] = None,
base_temp_dir: str = "/var/tmp",
- sock_dir: Optional[str] = None,
- qmp_timer: Optional[float] = None):
+ qmp_timer: Optional[float] = None,
+ sock_dir: Optional[str] = None):
# pylint: disable=too-many-arguments
if name is None:
@@ -125,7 +125,7 @@ def __init__(self,
sock_dir = base_temp_dir
super().__init__(binary, args, wrapper=wrapper, name=name,
base_temp_dir=base_temp_dir,
- sock_dir=sock_dir, qmp_timer=qmp_timer)
+ qmp_timer=qmp_timer)
self._qtest: Optional[QEMUQtestProtocol] = None
self._qtest_path = os.path.join(sock_dir, name + "-qtest.sock")
diff --git a/tests/qemu-iotests/tests/copy-before-write b/tests/qemu-iotests/tests/copy-before-write
index 2ffe092b31..d3987db942 100755
--- a/tests/qemu-iotests/tests/copy-before-write
+++ b/tests/qemu-iotests/tests/copy-before-write
@@ -44,8 +44,7 @@ class TestCbwError(iotests.QMPTestCase):
opts = ['-nodefaults', '-display', 'none', '-machine', 'none']
self.vm = QEMUMachine(iotests.qemu_prog, opts,
- base_temp_dir=iotests.test_dir,
- sock_dir=iotests.sock_dir)
+ base_temp_dir=iotests.test_dir)
self.vm.launch()
def do_cbw_error(self, on_cbw_error):
--
2.41.0
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH 0/4] python/machine: use socketpair() for console socket
2023-07-20 13:04 [PATCH 0/4] python/machine: use socketpair() for console socket John Snow
` (3 preceding siblings ...)
2023-07-20 13:04 ` [PATCH 4/4] python/machine: remove unused console socket configuration arguments John Snow
@ 2023-07-20 13:45 ` Peter Maydell
4 siblings, 0 replies; 12+ messages in thread
From: Peter Maydell @ 2023-07-20 13:45 UTC (permalink / raw)
To: John Snow
Cc: qemu-devel, qemu-block, Beraldo Leal, Hanna Reitz, Kevin Wolf,
Daniel Berrange, Cleber Rosa
On Thu, 20 Jul 2023 at 14:04, John Snow <jsnow@redhat.com> wrote:
>
> Like we did for the QMP socket, use socketpair() for the console socket
> so that hopefully there isn't a race condition during early boot where
> data might get dropped on the floor.
>
> "lightly tested"; passes local tests and gitlab CI. Doesn't seem to make
> anything worse.
I tried this on the s390 linux box and the test failed because
of a python exception:
__init__() got an unexpected keyword argument 'sock_dir'
$ QEMU_TEST_FLAKY_TESTS=1 ./build/aarch64/tests/venv/bin/avocado run
./build/aarch64/tests/avocado/machine_aarch64_sbsaref.py:Aarch64SbsarefMachine.test_sbsaref_edk2_firmware
JOB ID : 8392ba37b5a825ed75278f85f686364d181c01d3
JOB LOG : /home/linux1/avocado/job-results/job-2023-07-20T13.41-8392ba3/job.log
(1/1) ./build/aarch64/tests/avocado/machine_aarch64_sbsaref.py:Aarch64SbsarefMachine.test_sbsaref_edk2_firmware:
ERROR: __init__() got an unexpected keyword argument 'sock_dir' (3.64
s)
RESULTS : PASS 0 | ERROR 1 | FAIL 0 | SKIP 0 | WARN 0 | INTERRUPT 0
| CANCEL 0
JOB TIME : 6.92 s
Backtrace etc from in the job.log:
2023-07-20 13:41:49,125 stacktrace L0041 ERROR| Reproduced
traceback from:
/home/linux1/qemu/build/aarch64/tests/venv/lib/python3.8/site-package
s/avocado/core/test.py:770
2023-07-20 13:41:49,147 stacktrace L0045 ERROR| Traceback (most
recent call last):
2023-07-20 13:41:49,147 stacktrace L0045 ERROR| File
"/home/linux1/qemu/build/aarch64/tests/venv/lib/python3.8/site-packages/avocado/core/decorators.py",
line 90, in wrapper
2023-07-20 13:41:49,147 stacktrace L0045 ERROR| return
function(obj, *args, **kwargs)
2023-07-20 13:41:49,147 stacktrace L0045 ERROR| File
"/home/linux1/qemu/build/aarch64/tests/avocado/machine_aarch64_sbsaref.py",
line 84, in test_sbsaref_edk2_firmware
2023-07-20 13:41:49,147 stacktrace L0045 ERROR| self.fetch_firmware()
2023-07-20 13:41:49,147 stacktrace L0045 ERROR| File
"/home/linux1/qemu/build/aarch64/tests/avocado/machine_aarch64_sbsaref.py",
line 66, in fetch_firmware
2023-07-20 13:41:49,147 stacktrace L0045 ERROR| self.vm.set_console()
2023-07-20 13:41:49,147 stacktrace L0045 ERROR| File
"/home/linux1/qemu/build/aarch64/tests/avocado/avocado_qemu/__init__.py",
line 348, in vm
2023-07-20 13:41:49,147 stacktrace L0045 ERROR| return
self.get_vm(name='default')
2023-07-20 13:41:49,147 stacktrace L0045 ERROR| File
"/home/linux1/qemu/build/aarch64/tests/avocado/avocado_qemu/__init__.py",
line 354, in get_vm
2023-07-20 13:41:49,147 stacktrace L0045 ERROR|
self._vms[name] = self._new_vm(name, *args)
2023-07-20 13:41:49,147 stacktrace L0045 ERROR| File
"/home/linux1/qemu/build/aarch64/tests/avocado/avocado_qemu/__init__.py",
line 324, in _new_vm
2023-07-20 13:41:49,147 stacktrace L0045 ERROR| vm =
QEMUMachine(self.qemu_bin, base_temp_dir=self.workdir,
2023-07-20 13:41:49,147 stacktrace L0045 ERROR| TypeError:
__init__() got an unexpected keyword argument 'sock_dir'
2023-07-20 13:41:49,147 stacktrace L0046 ERROR|
2023-07-20 13:41:49,147 test L0775 DEBUG| Local variables:
2023-07-20 13:41:49,160 test L0778 DEBUG| -> obj <class
'machine_aarch64_sbsaref.Aarch64SbsarefMachine'>:
1-./build/aarch64/tests/avocado/machine_aarch64_sbsaref.py:Aarch64SbsarefMachine.test_sbsaref_edk2_firmware
2023-07-20 13:41:49,160 test L0778 DEBUG| -> args <class
'tuple'>: ()
2023-07-20 13:41:49,160 test L0778 DEBUG| -> kwargs
<class 'dict'>: {}
2023-07-20 13:41:49,160 test L0778 DEBUG| -> condition
<class 'str'>: 1
2023-07-20 13:41:49,160 test L0778 DEBUG| -> function
<class 'function'>: <function
Aarch64SbsarefMachine.test_sbsaref_edk2_firmware at 0x3ff814d9700>
2023-07-20 13:41:49,160 test L0778 DEBUG| -> message
<class 'str'>: Test is not reliable
2023-07-20 13:41:49,160 test L0778 DEBUG| -> negate
<class 'bool'>: True
thanks
-- PMM
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/4] python/machine: move socket setup out of _base_args property
2023-07-20 13:04 ` [PATCH 1/4] python/machine: move socket setup out of _base_args property John Snow
@ 2023-07-20 13:55 ` Daniel P. Berrangé
0 siblings, 0 replies; 12+ messages in thread
From: Daniel P. Berrangé @ 2023-07-20 13:55 UTC (permalink / raw)
To: John Snow
Cc: qemu-devel, qemu-block, Beraldo Leal, Hanna Reitz, Kevin Wolf,
Peter Maydell, Cleber Rosa
On Thu, Jul 20, 2023 at 09:04:45AM -0400, John Snow wrote:
> This property isn't meant to do much else besides return a list of
> strings, so move this setup back out into _pre_launch().
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
> python/qemu/machine/machine.py | 5 ++---
> 1 file changed, 2 insertions(+), 3 deletions(-)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
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 :|
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/4] python/console_socket: accept existing FD in initializer
2023-07-20 13:04 ` [PATCH 2/4] python/console_socket: accept existing FD in initializer John Snow
@ 2023-07-20 14:01 ` Daniel P. Berrangé
2023-07-20 14:35 ` John Snow
0 siblings, 1 reply; 12+ messages in thread
From: Daniel P. Berrangé @ 2023-07-20 14:01 UTC (permalink / raw)
To: John Snow
Cc: qemu-devel, qemu-block, Beraldo Leal, Hanna Reitz, Kevin Wolf,
Peter Maydell, Cleber Rosa
On Thu, Jul 20, 2023 at 09:04:46AM -0400, John Snow wrote:
> Useful if we want to use ConsoleSocket() for a socket created by
> socketpair().
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
> python/qemu/machine/console_socket.py | 11 +++++++----
> 1 file changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/python/qemu/machine/console_socket.py b/python/qemu/machine/console_socket.py
> index 4e28ba9bb2..42bfa12411 100644
> --- a/python/qemu/machine/console_socket.py
> +++ b/python/qemu/machine/console_socket.py
> @@ -17,7 +17,7 @@
> import socket
> import threading
> import time
> -from typing import Deque, Optional
> +from typing import Deque, Optional, Union
>
>
> class ConsoleSocket(socket.socket):
> @@ -30,13 +30,16 @@ class ConsoleSocket(socket.socket):
> Optionally a file path can be passed in and we will also
> dump the characters to this file for debugging purposes.
> """
> - def __init__(self, address: str, file: Optional[str] = None,
> + def __init__(self, address: Union[str, int], file: Optional[str] = None,
> drain: bool = False):
IMHO calling the pre-opened FD an "address" is pushing the
interpretation a bit.
It also makes the behaviour non-obvious from a caller. Seeing a
caller, you have to work backwards to find out what type it has,
to figure out the semantics of the method call.
IOW, I'd prefer to see
address: Optional[str], sock_fd: Optional[int]
and then just do a check
if (address is not None and sock_fd is not None) or
(address is None and sock_fd is None):
raise Exception("either 'address' or 'sock_fd' is required")
thus when you see
ConsoleSocket(sock_fd=xxx)
it is now obvious it has different behaviour to
ConsoleSocket(address=yyy)
> self._recv_timeout_sec = 300.0
> self._sleep_time = 0.5
> self._buffer: Deque[int] = deque()
> - socket.socket.__init__(self, socket.AF_UNIX, socket.SOCK_STREAM)
> - self.connect(address)
> + if isinstance(address, str):
> + socket.socket.__init__(self, socket.AF_UNIX, socket.SOCK_STREAM)
> + self.connect(address)
> + else:
> + socket.socket.__init__(self, fileno=address)
> self._logfile = None
> if file:
> # pylint: disable=consider-using-with
> --
> 2.41.0
>
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 :|
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 3/4] python/machine: use socketpair() for console connections
2023-07-20 13:04 ` [PATCH 3/4] python/machine: use socketpair() for console connections John Snow
@ 2023-07-20 14:02 ` Daniel P. Berrangé
0 siblings, 0 replies; 12+ messages in thread
From: Daniel P. Berrangé @ 2023-07-20 14:02 UTC (permalink / raw)
To: John Snow
Cc: qemu-devel, qemu-block, Beraldo Leal, Hanna Reitz, Kevin Wolf,
Peter Maydell, Cleber Rosa
On Thu, Jul 20, 2023 at 09:04:47AM -0400, John Snow wrote:
> Create a socketpair for the console output. This should help eliminate
> race conditions around console text early in the boot process that might
> otherwise have been dropped on the floor before being able to connect to
> QEMU under "server,nowait".
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
> python/qemu/machine/machine.py | 17 ++++++++++++++---
> 1 file changed, 14 insertions(+), 3 deletions(-)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
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 :|
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 4/4] python/machine: remove unused console socket configuration arguments
2023-07-20 13:04 ` [PATCH 4/4] python/machine: remove unused console socket configuration arguments John Snow
@ 2023-07-20 14:04 ` Daniel P. Berrangé
2023-07-20 14:29 ` John Snow
0 siblings, 1 reply; 12+ messages in thread
From: Daniel P. Berrangé @ 2023-07-20 14:04 UTC (permalink / raw)
To: John Snow
Cc: qemu-devel, qemu-block, Beraldo Leal, Hanna Reitz, Kevin Wolf,
Peter Maydell, Cleber Rosa
On Thu, Jul 20, 2023 at 09:04:48AM -0400, John Snow wrote:
> By using a socketpair for the console, we don't need the sock_dir
> argument for the base class anymore, remove it.
>
> The qtest subclass still uses the argument for the qtest socket for now.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
> python/qemu/machine/machine.py | 18 ------------------
> python/qemu/machine/qtest.py | 6 +++---
> tests/qemu-iotests/tests/copy-before-write | 3 +--
> 3 files changed, 4 insertions(+), 23 deletions(-)
A couple of callers to be updated to remove 'sock_dir=':
$ git grep 'sock_dir=' tests
tests/avocado/acpi-bits.py: sock_dir=sock_dir, qmp_timer=qmp_timer)
tests/avocado/avocado_qemu/__init__.py: sock_dir=self._sd.name, log_dir=self.logdir)
tests/qemu-iotests/iotests.py: sock_dir=sock_dir, qmp_timer=timer)
tests/qemu-iotests/tests/copy-before-write: sock_dir=iotests.sock_dir)
presume the avocado_qemu/__init__.py one is what caused the
failure Peter reported.
>
> diff --git a/python/qemu/machine/machine.py b/python/qemu/machine/machine.py
> index ef9b2dc02e..350aa8bb26 100644
> --- a/python/qemu/machine/machine.py
> +++ b/python/qemu/machine/machine.py
> @@ -127,7 +127,6 @@ def __init__(self,
> name: Optional[str] = None,
> base_temp_dir: str = "/var/tmp",
> monitor_address: Optional[SocketAddrT] = None,
> - sock_dir: Optional[str] = None,
> drain_console: bool = False,
> console_log: Optional[str] = None,
> log_dir: Optional[str] = None,
> @@ -141,7 +140,6 @@ def __init__(self,
> @param name: prefix for socket and log file names (default: qemu-PID)
> @param base_temp_dir: default location where temp files are created
> @param monitor_address: address for QMP monitor
> - @param sock_dir: where to create socket (defaults to base_temp_dir)
> @param drain_console: (optional) True to drain console socket to buffer
> @param console_log: (optional) path to console log file
> @param log_dir: where to create and keep log files
> @@ -163,7 +161,6 @@ def __init__(self,
> 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
>
> self._monitor_address = monitor_address
> @@ -189,9 +186,6 @@ def __init__(self,
> self._console_index = 0
> self._console_set = False
> self._console_device_type: Optional[str] = None
> - self._console_address = os.path.join(
> - self.sock_dir, f"{self._name}.con"
> - )
> self._console_socket: Optional[socket.socket] = None
> self._remove_files: List[str] = []
> self._user_killed = False
> @@ -334,9 +328,6 @@ def args(self) -> List[str]:
> return self._args
>
> def _pre_launch(self) -> None:
> - if self._console_set:
> - self._remove_files.append(self._console_address)
> -
> if self._qmp_set:
> if self._monitor_address is None:
> self._sock_pair = socket.socketpair()
> @@ -900,15 +891,6 @@ def temp_dir(self) -> str:
> dir=self._base_temp_dir)
> return self._temp_dir
>
> - @property
> - def sock_dir(self) -> str:
> - """
> - Returns the directory used for sockfiles by this machine.
> - """
> - if self._sock_dir:
> - return self._sock_dir
> - return self.temp_dir
> -
> @property
> def log_dir(self) -> str:
> """
> diff --git a/python/qemu/machine/qtest.py b/python/qemu/machine/qtest.py
> index 1c46138bd0..22f8045ef6 100644
> --- a/python/qemu/machine/qtest.py
> +++ b/python/qemu/machine/qtest.py
> @@ -115,8 +115,8 @@ def __init__(self,
> wrapper: Sequence[str] = (),
> name: Optional[str] = None,
> base_temp_dir: str = "/var/tmp",
> - sock_dir: Optional[str] = None,
> - qmp_timer: Optional[float] = None):
> + qmp_timer: Optional[float] = None,
> + sock_dir: Optional[str] = None):
> # pylint: disable=too-many-arguments
>
> if name is None:
> @@ -125,7 +125,7 @@ def __init__(self,
> sock_dir = base_temp_dir
> super().__init__(binary, args, wrapper=wrapper, name=name,
> base_temp_dir=base_temp_dir,
> - sock_dir=sock_dir, qmp_timer=qmp_timer)
> + qmp_timer=qmp_timer)
> self._qtest: Optional[QEMUQtestProtocol] = None
> self._qtest_path = os.path.join(sock_dir, name + "-qtest.sock")
>
> diff --git a/tests/qemu-iotests/tests/copy-before-write b/tests/qemu-iotests/tests/copy-before-write
> index 2ffe092b31..d3987db942 100755
> --- a/tests/qemu-iotests/tests/copy-before-write
> +++ b/tests/qemu-iotests/tests/copy-before-write
> @@ -44,8 +44,7 @@ class TestCbwError(iotests.QMPTestCase):
>
> opts = ['-nodefaults', '-display', 'none', '-machine', 'none']
> self.vm = QEMUMachine(iotests.qemu_prog, opts,
> - base_temp_dir=iotests.test_dir,
> - sock_dir=iotests.sock_dir)
> + base_temp_dir=iotests.test_dir)
> self.vm.launch()
>
> def do_cbw_error(self, on_cbw_error):
> --
> 2.41.0
>
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 :|
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 4/4] python/machine: remove unused console socket configuration arguments
2023-07-20 14:04 ` Daniel P. Berrangé
@ 2023-07-20 14:29 ` John Snow
0 siblings, 0 replies; 12+ messages in thread
From: John Snow @ 2023-07-20 14:29 UTC (permalink / raw)
To: Daniel P. Berrangé
Cc: qemu-devel, qemu-block, Beraldo Leal, Hanna Reitz, Kevin Wolf,
Peter Maydell, Cleber Rosa
On Thu, Jul 20, 2023 at 10:05 AM Daniel P. Berrangé <berrange@redhat.com> wrote:
>
> On Thu, Jul 20, 2023 at 09:04:48AM -0400, John Snow wrote:
> > By using a socketpair for the console, we don't need the sock_dir
> > argument for the base class anymore, remove it.
> >
> > The qtest subclass still uses the argument for the qtest socket for now.
> >
> > Signed-off-by: John Snow <jsnow@redhat.com>
> > ---
> > python/qemu/machine/machine.py | 18 ------------------
> > python/qemu/machine/qtest.py | 6 +++---
> > tests/qemu-iotests/tests/copy-before-write | 3 +--
> > 3 files changed, 4 insertions(+), 23 deletions(-)
>
> A couple of callers to be updated to remove 'sock_dir=':
>
> $ git grep 'sock_dir=' tests
> tests/avocado/acpi-bits.py: sock_dir=sock_dir, qmp_timer=qmp_timer)
> tests/avocado/avocado_qemu/__init__.py: sock_dir=self._sd.name, log_dir=self.logdir)
> tests/qemu-iotests/iotests.py: sock_dir=sock_dir, qmp_timer=timer)
> tests/qemu-iotests/tests/copy-before-write: sock_dir=iotests.sock_dir)
>
> presume the avocado_qemu/__init__.py one is what caused the
> failure Peter reported.
>
Yep, missed a spot, sorry :( I tested avocado after patch 3 but not here.
While I'm at it, though, I am testing the same treatment for the qtest
extension and just removing sock_dir from *everything*, since we don't
need that workaround anymore if we aren't creating named sockets.
...And if I get rid of *that*, I can get rid of some other temp
directory management stuff too.
> >
> > diff --git a/python/qemu/machine/machine.py b/python/qemu/machine/machine.py
> > index ef9b2dc02e..350aa8bb26 100644
> > --- a/python/qemu/machine/machine.py
> > +++ b/python/qemu/machine/machine.py
> > @@ -127,7 +127,6 @@ def __init__(self,
> > name: Optional[str] = None,
> > base_temp_dir: str = "/var/tmp",
> > monitor_address: Optional[SocketAddrT] = None,
> > - sock_dir: Optional[str] = None,
> > drain_console: bool = False,
> > console_log: Optional[str] = None,
> > log_dir: Optional[str] = None,
> > @@ -141,7 +140,6 @@ def __init__(self,
> > @param name: prefix for socket and log file names (default: qemu-PID)
> > @param base_temp_dir: default location where temp files are created
> > @param monitor_address: address for QMP monitor
> > - @param sock_dir: where to create socket (defaults to base_temp_dir)
> > @param drain_console: (optional) True to drain console socket to buffer
> > @param console_log: (optional) path to console log file
> > @param log_dir: where to create and keep log files
> > @@ -163,7 +161,6 @@ def __init__(self,
> > 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
> >
> > self._monitor_address = monitor_address
> > @@ -189,9 +186,6 @@ def __init__(self,
> > self._console_index = 0
> > self._console_set = False
> > self._console_device_type: Optional[str] = None
> > - self._console_address = os.path.join(
> > - self.sock_dir, f"{self._name}.con"
> > - )
> > self._console_socket: Optional[socket.socket] = None
> > self._remove_files: List[str] = []
> > self._user_killed = False
> > @@ -334,9 +328,6 @@ def args(self) -> List[str]:
> > return self._args
> >
> > def _pre_launch(self) -> None:
> > - if self._console_set:
> > - self._remove_files.append(self._console_address)
> > -
> > if self._qmp_set:
> > if self._monitor_address is None:
> > self._sock_pair = socket.socketpair()
> > @@ -900,15 +891,6 @@ def temp_dir(self) -> str:
> > dir=self._base_temp_dir)
> > return self._temp_dir
> >
> > - @property
> > - def sock_dir(self) -> str:
> > - """
> > - Returns the directory used for sockfiles by this machine.
> > - """
> > - if self._sock_dir:
> > - return self._sock_dir
> > - return self.temp_dir
> > -
> > @property
> > def log_dir(self) -> str:
> > """
> > diff --git a/python/qemu/machine/qtest.py b/python/qemu/machine/qtest.py
> > index 1c46138bd0..22f8045ef6 100644
> > --- a/python/qemu/machine/qtest.py
> > +++ b/python/qemu/machine/qtest.py
> > @@ -115,8 +115,8 @@ def __init__(self,
> > wrapper: Sequence[str] = (),
> > name: Optional[str] = None,
> > base_temp_dir: str = "/var/tmp",
> > - sock_dir: Optional[str] = None,
> > - qmp_timer: Optional[float] = None):
> > + qmp_timer: Optional[float] = None,
> > + sock_dir: Optional[str] = None):
> > # pylint: disable=too-many-arguments
> >
> > if name is None:
> > @@ -125,7 +125,7 @@ def __init__(self,
> > sock_dir = base_temp_dir
> > super().__init__(binary, args, wrapper=wrapper, name=name,
> > base_temp_dir=base_temp_dir,
> > - sock_dir=sock_dir, qmp_timer=qmp_timer)
> > + qmp_timer=qmp_timer)
> > self._qtest: Optional[QEMUQtestProtocol] = None
> > self._qtest_path = os.path.join(sock_dir, name + "-qtest.sock")
> >
> > diff --git a/tests/qemu-iotests/tests/copy-before-write b/tests/qemu-iotests/tests/copy-before-write
> > index 2ffe092b31..d3987db942 100755
> > --- a/tests/qemu-iotests/tests/copy-before-write
> > +++ b/tests/qemu-iotests/tests/copy-before-write
> > @@ -44,8 +44,7 @@ class TestCbwError(iotests.QMPTestCase):
> >
> > opts = ['-nodefaults', '-display', 'none', '-machine', 'none']
> > self.vm = QEMUMachine(iotests.qemu_prog, opts,
> > - base_temp_dir=iotests.test_dir,
> > - sock_dir=iotests.sock_dir)
> > + base_temp_dir=iotests.test_dir)
> > self.vm.launch()
> >
> > def do_cbw_error(self, on_cbw_error):
> > --
> > 2.41.0
> >
>
> 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 :|
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/4] python/console_socket: accept existing FD in initializer
2023-07-20 14:01 ` Daniel P. Berrangé
@ 2023-07-20 14:35 ` John Snow
0 siblings, 0 replies; 12+ messages in thread
From: John Snow @ 2023-07-20 14:35 UTC (permalink / raw)
To: Daniel P. Berrangé
Cc: qemu-devel, qemu-block, Beraldo Leal, Hanna Reitz, Kevin Wolf,
Peter Maydell, Cleber Rosa
On Thu, Jul 20, 2023 at 10:01 AM Daniel P. Berrangé <berrange@redhat.com> wrote:
>
> On Thu, Jul 20, 2023 at 09:04:46AM -0400, John Snow wrote:
> > Useful if we want to use ConsoleSocket() for a socket created by
> > socketpair().
> >
> > Signed-off-by: John Snow <jsnow@redhat.com>
> > ---
> > python/qemu/machine/console_socket.py | 11 +++++++----
> > 1 file changed, 7 insertions(+), 4 deletions(-)
> >
> > diff --git a/python/qemu/machine/console_socket.py b/python/qemu/machine/console_socket.py
> > index 4e28ba9bb2..42bfa12411 100644
> > --- a/python/qemu/machine/console_socket.py
> > +++ b/python/qemu/machine/console_socket.py
> > @@ -17,7 +17,7 @@
> > import socket
> > import threading
> > import time
> > -from typing import Deque, Optional
> > +from typing import Deque, Optional, Union
> >
> >
> > class ConsoleSocket(socket.socket):
> > @@ -30,13 +30,16 @@ class ConsoleSocket(socket.socket):
> > Optionally a file path can be passed in and we will also
> > dump the characters to this file for debugging purposes.
> > """
> > - def __init__(self, address: str, file: Optional[str] = None,
> > + def __init__(self, address: Union[str, int], file: Optional[str] = None,
> > drain: bool = False):
>
> IMHO calling the pre-opened FD an "address" is pushing the
> interpretation a bit.
>
You're right.
> It also makes the behaviour non-obvious from a caller. Seeing a
> caller, you have to work backwards to find out what type it has,
> to figure out the semantics of the method call.
>
> IOW, I'd prefer to see
>
> address: Optional[str], sock_fd: Optional[int]
>
> and then just do a check
>
> if (address is not None and sock_fd is not None) or
> (address is None and sock_fd is None):
> raise Exception("either 'address' or 'sock_fd' is required")
>
> thus when you see
>
> ConsoleSocket(sock_fd=xxx)
>
> it is now obvious it has different behaviour to
>
> ConsoleSocket(address=yyy)
>
Yeah, that's just a little harder to type - in the sense that it
appears as though you could omit either argument by just observing the
signature. One thing I like about "one mandatory argument that takes
many forms" is that it's obvious you need to give it *something* from
the signature.
You're right that the name is now very silly, though.
The "obvious it has different behavior" is a good argument, I'll change it.
--js
>
> > self._recv_timeout_sec = 300.0
> > self._sleep_time = 0.5
> > self._buffer: Deque[int] = deque()
> > - socket.socket.__init__(self, socket.AF_UNIX, socket.SOCK_STREAM)
> > - self.connect(address)
> > + if isinstance(address, str):
> > + socket.socket.__init__(self, socket.AF_UNIX, socket.SOCK_STREAM)
> > + self.connect(address)
> > + else:
> > + socket.socket.__init__(self, fileno=address)
> > self._logfile = None
> > if file:
> > # pylint: disable=consider-using-with
> > --
> > 2.41.0
> >
>
> 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 :|
>
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2023-07-20 14:36 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-07-20 13:04 [PATCH 0/4] python/machine: use socketpair() for console socket John Snow
2023-07-20 13:04 ` [PATCH 1/4] python/machine: move socket setup out of _base_args property John Snow
2023-07-20 13:55 ` Daniel P. Berrangé
2023-07-20 13:04 ` [PATCH 2/4] python/console_socket: accept existing FD in initializer John Snow
2023-07-20 14:01 ` Daniel P. Berrangé
2023-07-20 14:35 ` John Snow
2023-07-20 13:04 ` [PATCH 3/4] python/machine: use socketpair() for console connections John Snow
2023-07-20 14:02 ` Daniel P. Berrangé
2023-07-20 13:04 ` [PATCH 4/4] python/machine: remove unused console socket configuration arguments John Snow
2023-07-20 14:04 ` Daniel P. Berrangé
2023-07-20 14:29 ` John Snow
2023-07-20 13:45 ` [PATCH 0/4] python/machine: use socketpair() for console socket Peter Maydell
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).