| Message ID | 20230725180337.2937292-7-jsnow@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | python/machine: use socketpair() for console socket | expand |
On Tue, Jul 25, 2023 at 02:03:37PM -0400, John Snow wrote: > By using a socketpair for all of the sockets managed by the VM class and > its extensions, we don't need the sock_dir argument anymore, so remove > it. > > We only added this argument so that we could specify a second, shorter > temporary directory for cases where the temp/log dirs were "too long" as > a socket name on macOS. We don't need it for this class now. In one > case, avocado testing takes over responsibility for creating an > appropriate sockdir. > > Signed-off-by: John Snow <jsnow@redhat.com> > --- > python/qemu/machine/machine.py | 18 ------------------ > python/qemu/machine/qtest.py | 5 +---- > tests/avocado/acpi-bits.py | 5 +---- > tests/avocado/avocado_qemu/__init__.py | 2 +- > tests/avocado/machine_aspeed.py | 5 ++++- > tests/qemu-iotests/iotests.py | 2 +- > tests/qemu-iotests/tests/copy-before-write | 3 +-- > 7 files changed, 9 insertions(+), 31 deletions(-) Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> With regards, Daniel
On Tue, Jul 25, 2023 at 2:26 PM Daniel P. Berrangé <berrange@redhat.com> wrote: > > On Tue, Jul 25, 2023 at 02:03:37PM -0400, John Snow wrote: > > By using a socketpair for all of the sockets managed by the VM class and > > its extensions, we don't need the sock_dir argument anymore, so remove > > it. > > > > We only added this argument so that we could specify a second, shorter > > temporary directory for cases where the temp/log dirs were "too long" as > > a socket name on macOS. We don't need it for this class now. In one > > case, avocado testing takes over responsibility for creating an > > appropriate sockdir. > > > > Signed-off-by: John Snow <jsnow@redhat.com> > > --- > > python/qemu/machine/machine.py | 18 ------------------ > > python/qemu/machine/qtest.py | 5 +---- > > tests/avocado/acpi-bits.py | 5 +---- > > tests/avocado/avocado_qemu/__init__.py | 2 +- > > tests/avocado/machine_aspeed.py | 5 ++++- > > tests/qemu-iotests/iotests.py | 2 +- > > tests/qemu-iotests/tests/copy-before-write | 3 +-- > > 7 files changed, 9 insertions(+), 31 deletions(-) > > Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> > Thanks! I don't know if we want this for *this* cycle or not, it's "only testing code" and it should hopefully be harmless. If it makes the tests more reliable, it might be worth it. I don't have strong feelings one way or the other, we've lived without it for so long as-is. I'll see what Peter says. --js > 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 :| >
On Tue, Jul 25, 2023 at 02:33:36PM -0400, John Snow wrote: > On Tue, Jul 25, 2023 at 2:26 PM Daniel P. Berrangé <berrange@redhat.com> wrote: > > > > On Tue, Jul 25, 2023 at 02:03:37PM -0400, John Snow wrote: > > > By using a socketpair for all of the sockets managed by the VM class and > > > its extensions, we don't need the sock_dir argument anymore, so remove > > > it. > > > > > > We only added this argument so that we could specify a second, shorter > > > temporary directory for cases where the temp/log dirs were "too long" as > > > a socket name on macOS. We don't need it for this class now. In one > > > case, avocado testing takes over responsibility for creating an > > > appropriate sockdir. > > > > > > Signed-off-by: John Snow <jsnow@redhat.com> > > > --- > > > python/qemu/machine/machine.py | 18 ------------------ > > > python/qemu/machine/qtest.py | 5 +---- > > > tests/avocado/acpi-bits.py | 5 +---- > > > tests/avocado/avocado_qemu/__init__.py | 2 +- > > > tests/avocado/machine_aspeed.py | 5 ++++- > > > tests/qemu-iotests/iotests.py | 2 +- > > > tests/qemu-iotests/tests/copy-before-write | 3 +-- > > > 7 files changed, 9 insertions(+), 31 deletions(-) > > > > Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> > > > > Thanks! > > I don't know if we want this for *this* cycle or not, it's "only > testing code" and it should hopefully be harmless. If it makes the > tests more reliable, it might be worth it. I don't have strong > feelings one way or the other, we've lived without it for so long > as-is. > > I'll see what Peter says. Although it does affect end users, the biggest impact is our own CI. Once the release is out it would help CI on stable trees, but the big win is CI on master. I'd verge towards skipping this during freeze and applying to master after release. Then propose cherry-pick to stable once it has had some soak time in our real CI. With regards, Daniel
diff --git a/python/qemu/machine/machine.py b/python/qemu/machine/machine.py index 09f214c95c..1dd2de6da8 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() @@ -918,15 +909,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 8180d3ab01..4f5ede85b2 100644 --- a/python/qemu/machine/qtest.py +++ b/python/qemu/machine/qtest.py @@ -135,17 +135,14 @@ 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): # pylint: disable=too-many-arguments if name is None: name = "qemu-%d" % os.getpid() - if sock_dir is None: - 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_sock_pair: Optional[ Tuple[socket.socket, socket.socket]] = None diff --git a/tests/avocado/acpi-bits.py b/tests/avocado/acpi-bits.py index 3ed286dcbd..bc2b29671e 100644 --- a/tests/avocado/acpi-bits.py +++ b/tests/avocado/acpi-bits.py @@ -92,17 +92,14 @@ def __init__(self, base_temp_dir: str = "/var/tmp", debugcon_log: str = "debugcon-log.txt", debugcon_addr: str = "0x403", - sock_dir: Optional[str] = None, qmp_timer: Optional[float] = None): # pylint: disable=too-many-arguments if name is None: name = "qemu-bits-%d" % os.getpid() - if sock_dir is None: - 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.debugcon_log = debugcon_log self.debugcon_addr = debugcon_addr self.base_temp_dir = base_temp_dir diff --git a/tests/avocado/avocado_qemu/__init__.py b/tests/avocado/avocado_qemu/__init__.py index 33090903f1..cfdaf9dab7 100644 --- a/tests/avocado/avocado_qemu/__init__.py +++ b/tests/avocado/avocado_qemu/__init__.py @@ -322,7 +322,7 @@ def require_multiprocess(self): def _new_vm(self, name, *args): self._sd = tempfile.TemporaryDirectory(prefix="qemu_") vm = QEMUMachine(self.qemu_bin, base_temp_dir=self.workdir, - sock_dir=self._sd.name, log_dir=self.logdir) + log_dir=self.logdir) self.log.debug('QEMUMachine "%s" created', name) self.log.debug('QEMUMachine "%s" temp_dir: %s', name, vm.temp_dir) self.log.debug('QEMUMachine "%s" log_dir: %s', name, vm.log_dir) diff --git a/tests/avocado/machine_aspeed.py b/tests/avocado/machine_aspeed.py index 724ee72c02..3e8a3b0da7 100644 --- a/tests/avocado/machine_aspeed.py +++ b/tests/avocado/machine_aspeed.py @@ -247,7 +247,10 @@ def test_arm_ast2600_evb_buildroot_tpm(self): image_path = self.fetch_asset(image_url, asset_hash=image_hash, algorithm='sha256') - socket = os.path.join(self.vm.sock_dir, 'swtpm-socket') + # force creation of VM object, which also defines self._sd + vm = self.vm + + socket = os.path.join(self._sd.name, 'swtpm-socket') subprocess.run(['swtpm', 'socket', '-d', '--tpm2', '--tpmstate', f'dir={self.vm.temp_dir}', diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py index ef66fbd62b..145c682713 100644 --- a/tests/qemu-iotests/iotests.py +++ b/tests/qemu-iotests/iotests.py @@ -823,7 +823,7 @@ def __init__(self, path_suffix=''): super().__init__(qemu_prog, qemu_opts, wrapper=wrapper, name=name, base_temp_dir=test_dir, - sock_dir=sock_dir, qmp_timer=timer) + qmp_timer=timer) self._num_drives = 0 def _post_shutdown(self) -> None: 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):
By using a socketpair for all of the sockets managed by the VM class and its extensions, we don't need the sock_dir argument anymore, so remove it. We only added this argument so that we could specify a second, shorter temporary directory for cases where the temp/log dirs were "too long" as a socket name on macOS. We don't need it for this class now. In one case, avocado testing takes over responsibility for creating an appropriate sockdir. Signed-off-by: John Snow <jsnow@redhat.com> --- python/qemu/machine/machine.py | 18 ------------------ python/qemu/machine/qtest.py | 5 +---- tests/avocado/acpi-bits.py | 5 +---- tests/avocado/avocado_qemu/__init__.py | 2 +- tests/avocado/machine_aspeed.py | 5 ++++- tests/qemu-iotests/iotests.py | 2 +- tests/qemu-iotests/tests/copy-before-write | 3 +-- 7 files changed, 9 insertions(+), 31 deletions(-)