| Message ID | 20210211220146.2525771-3-crosa@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Python / Acceptance Tests: improve logging | expand |
On 2/11/21 11:01 PM, Cleber Rosa wrote: > Each instance of qemu.machine.QEMUMachine currently has a "test > directory", which may not have any relation to a "test", and it's > really a temporary directory. > > Users instantiating the QEMUMachine class will be able to set the > location of the directory that will *contain* the QEMUMachine unique > temporary directory, so that parameter name has been changed from > test_dir to base_temp_dir. > > A property has been added to allow users to access it without using > private attributes, and with that, the directory is created on first > use of the property. > > Signed-off-by: Cleber Rosa <crosa@redhat.com> > --- > python/qemu/machine.py | 24 ++++++++++++++++-------- > python/qemu/qtest.py | 6 +++--- > tests/acceptance/virtio-gpu.py | 2 +- > tests/qemu-iotests/iotests.py | 2 +- > 4 files changed, 21 insertions(+), 13 deletions(-) > > diff --git a/python/qemu/machine.py b/python/qemu/machine.py > index 6e44bda337..b379fcbe72 100644 > --- a/python/qemu/machine.py > +++ b/python/qemu/machine.py > @@ -84,7 +84,7 @@ class QEMUMachine: > args: Sequence[str] = (), > wrapper: Sequence[str] = (), > name: Optional[str] = None, > - test_dir: str = "/var/tmp", > + base_temp_dir: str = "/var/tmp", Not this patch fault, but I see we use /var/tmp since commit 66613974468 ("scripts: refactor the VM class in iotests for reuse"). Can we use an OS agnostic method to get temp storage directory instead?
On Fri, Feb 12, 2021 at 12:35:26AM +0100, Philippe Mathieu-Daudé wrote: > On 2/11/21 11:01 PM, Cleber Rosa wrote: > > Each instance of qemu.machine.QEMUMachine currently has a "test > > directory", which may not have any relation to a "test", and it's > > really a temporary directory. > > > > Users instantiating the QEMUMachine class will be able to set the > > location of the directory that will *contain* the QEMUMachine unique > > temporary directory, so that parameter name has been changed from > > test_dir to base_temp_dir. > > > > A property has been added to allow users to access it without using > > private attributes, and with that, the directory is created on first > > use of the property. > > > > Signed-off-by: Cleber Rosa <crosa@redhat.com> > > --- > > python/qemu/machine.py | 24 ++++++++++++++++-------- > > python/qemu/qtest.py | 6 +++--- > > tests/acceptance/virtio-gpu.py | 2 +- > > tests/qemu-iotests/iotests.py | 2 +- > > 4 files changed, 21 insertions(+), 13 deletions(-) > > > > diff --git a/python/qemu/machine.py b/python/qemu/machine.py > > index 6e44bda337..b379fcbe72 100644 > > --- a/python/qemu/machine.py > > +++ b/python/qemu/machine.py > > @@ -84,7 +84,7 @@ class QEMUMachine: > > args: Sequence[str] = (), > > wrapper: Sequence[str] = (), > > name: Optional[str] = None, > > - test_dir: str = "/var/tmp", > > + base_temp_dir: str = "/var/tmp", > > Not this patch fault, but I see we use /var/tmp since commit > 66613974468 ("scripts: refactor the VM class in iotests for reuse"). > Can we use an OS agnostic method to get temp storage directory instead? > Sounds like a reasonable thing to do. I do remember a few issues with Python's tempfile.gettempdir() returning '/tmp' on most Linux/Unix, dur to the fact that '/tmp' is a tmpfs filesystem on most modern Linux systems. Anyway, I agree that hardcoding '/var/tmp' needs to be reconsidered. Cheers, - Cleber.
Hi, On 2/11/21 7:01 PM, Cleber Rosa wrote: > Each instance of qemu.machine.QEMUMachine currently has a "test > directory", which may not have any relation to a "test", and it's > really a temporary directory. > > Users instantiating the QEMUMachine class will be able to set the > location of the directory that will *contain* the QEMUMachine unique > temporary directory, so that parameter name has been changed from > test_dir to base_temp_dir. > > A property has been added to allow users to access it without using > private attributes, and with that, the directory is created on first > use of the property. > > Signed-off-by: Cleber Rosa <crosa@redhat.com> > --- > python/qemu/machine.py | 24 ++++++++++++++++-------- > python/qemu/qtest.py | 6 +++--- > tests/acceptance/virtio-gpu.py | 2 +- > tests/qemu-iotests/iotests.py | 2 +- > 4 files changed, 21 insertions(+), 13 deletions(-) > > diff --git a/python/qemu/machine.py b/python/qemu/machine.py > index 6e44bda337..b379fcbe72 100644 > --- a/python/qemu/machine.py > +++ b/python/qemu/machine.py > @@ -84,7 +84,7 @@ class QEMUMachine: > args: Sequence[str] = (), > wrapper: Sequence[str] = (), > name: Optional[str] = None, > - test_dir: str = "/var/tmp", > + base_temp_dir: str = "/var/tmp", > monitor_address: Optional[SocketAddrT] = None, > socket_scm_helper: Optional[str] = None, > sock_dir: Optional[str] = None, > @@ -97,10 +97,10 @@ class QEMUMachine: > @param args: list of extra arguments > @param wrapper: list of arguments used as prefix to qemu binary > @param name: prefix for socket and log file names (default: qemu-PID) > - @param test_dir: where to create socket and log file > + @param base_temp_dir: default location where temporary files are created > @param monitor_address: address for QMP monitor > @param socket_scm_helper: helper program, required for send_fd_scm() > - @param sock_dir: where to create socket (overrides test_dir for sock) > + @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 > @note: Qemu process is not started until launch() is used. > @@ -112,8 +112,8 @@ class QEMUMachine: > self._wrapper = wrapper > > self._name = name or "qemu-%d" % os.getpid() > - self._test_dir = test_dir > - self._sock_dir = sock_dir or self._test_dir > + self._base_temp_dir = base_temp_dir > + self._sock_dir = sock_dir or self._base_temp_dir > self._socket_scm_helper = socket_scm_helper > > if monitor_address is not None: > @@ -303,9 +303,7 @@ class QEMUMachine: > return args > > def _pre_launch(self) -> None: > - self._temp_dir = tempfile.mkdtemp(prefix="qemu-machine-", > - dir=self._test_dir) > - self._qemu_log_path = os.path.join(self._temp_dir, self._name + ".log") > + self._qemu_log_path = os.path.join(self.temp_dir, self._name + ".log") > self._qemu_log_file = open(self._qemu_log_path, 'wb') > > if self._console_set: > @@ -744,3 +742,13 @@ class QEMUMachine: > file=self._console_log_path, > drain=self._drain_console) > return self._console_socket > + > + @property > + def temp_dir(self) -> str: > + """ > + Returns a temporary directory to be used for this machine > + """ > + if self._temp_dir is None: > + self._temp_dir = tempfile.mkdtemp(prefix="qemu-machine-", > + dir=self._base_temp_dir) > + return self._temp_dir > diff --git a/python/qemu/qtest.py b/python/qemu/qtest.py > index 39a0cf62fe..78b97d13cf 100644 > --- a/python/qemu/qtest.py > +++ b/python/qemu/qtest.py > @@ -112,14 +112,14 @@ class QEMUQtestMachine(QEMUMachine): > binary: str, > args: Sequence[str] = (), > name: Optional[str] = None, > - test_dir: str = "/var/tmp", > + base_temp_dir: str = "/var/tmp", In qtest.QEMUQtestMachine.__init__(), the argument named 'test_dir' still make sense, right? - Wainer > socket_scm_helper: Optional[str] = None, > sock_dir: Optional[str] = None): > if name is None: > name = "qemu-%d" % os.getpid() > if sock_dir is None: > - sock_dir = test_dir > - super().__init__(binary, args, name=name, test_dir=test_dir, > + sock_dir = base_temp_dir > + super().__init__(binary, args, name=name, base_temp_dir=base_temp_dir, > socket_scm_helper=socket_scm_helper, > sock_dir=sock_dir) > self._qtest: Optional[QEMUQtestProtocol] = None > diff --git a/tests/acceptance/virtio-gpu.py b/tests/acceptance/virtio-gpu.py > index 211f02932f..8d689eb820 100644 > --- a/tests/acceptance/virtio-gpu.py > +++ b/tests/acceptance/virtio-gpu.py > @@ -119,7 +119,7 @@ class VirtioGPUx86(Test): > os.set_inheritable(vug_sock.fileno(), True) > > self._vug_log_path = os.path.join( > - self.vm._test_dir, "vhost-user-gpu.log" > + self.vm.temp_dir, "vhost-user-gpu.log" > ) > self._vug_log_file = open(self._vug_log_path, "wb") > print(self._vug_log_path) > diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py > index 00be68eca3..b02a3dc092 100644 > --- a/tests/qemu-iotests/iotests.py > +++ b/tests/qemu-iotests/iotests.py > @@ -562,7 +562,7 @@ class VM(qtest.QEMUQtestMachine): > def __init__(self, path_suffix=''): > name = "qemu%s-%d" % (path_suffix, os.getpid()) > super().__init__(qemu_prog, qemu_opts, name=name, > - test_dir=test_dir, > + base_temp_dir=test_dir, > socket_scm_helper=socket_scm_helper, > sock_dir=sock_dir) > self._num_drives = 0
On 2/11/21 5:01 PM, Cleber Rosa wrote: > Each instance of qemu.machine.QEMUMachine currently has a "test > directory", which may not have any relation to a "test", and it's > really a temporary directory. > > Users instantiating the QEMUMachine class will be able to set the > location of the directory that will *contain* the QEMUMachine unique > temporary directory, so that parameter name has been changed from > test_dir to base_temp_dir. > Yeah, makes sense. It's a bad name. > A property has been added to allow users to access it without using > private attributes, OK > and with that, the directory is created on first > use of the property. > Less sure if I want this. > Signed-off-by: Cleber Rosa <crosa@redhat.com> > --- > python/qemu/machine.py | 24 ++++++++++++++++-------- > python/qemu/qtest.py | 6 +++--- > tests/acceptance/virtio-gpu.py | 2 +- > tests/qemu-iotests/iotests.py | 2 +- > 4 files changed, 21 insertions(+), 13 deletions(-) > > diff --git a/python/qemu/machine.py b/python/qemu/machine.py > index 6e44bda337..b379fcbe72 100644 > --- a/python/qemu/machine.py > +++ b/python/qemu/machine.py > @@ -84,7 +84,7 @@ class QEMUMachine: > args: Sequence[str] = (), > wrapper: Sequence[str] = (), > name: Optional[str] = None, > - test_dir: str = "/var/tmp", > + base_temp_dir: str = "/var/tmp", > monitor_address: Optional[SocketAddrT] = None, > socket_scm_helper: Optional[str] = None, > sock_dir: Optional[str] = None, > @@ -97,10 +97,10 @@ class QEMUMachine: > @param args: list of extra arguments > @param wrapper: list of arguments used as prefix to qemu binary > @param name: prefix for socket and log file names (default: qemu-PID) > - @param test_dir: where to create socket and log file > + @param base_temp_dir: default location where temporary files are created > @param monitor_address: address for QMP monitor > @param socket_scm_helper: helper program, required for send_fd_scm() > - @param sock_dir: where to create socket (overrides test_dir for sock) > + @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 > @note: Qemu process is not started until launch() is used. > @@ -112,8 +112,8 @@ class QEMUMachine: > self._wrapper = wrapper > > self._name = name or "qemu-%d" % os.getpid() > - self._test_dir = test_dir > - self._sock_dir = sock_dir or self._test_dir > + self._base_temp_dir = base_temp_dir > + self._sock_dir = sock_dir or self._base_temp_dir > self._socket_scm_helper = socket_scm_helper > > if monitor_address is not None: > @@ -303,9 +303,7 @@ class QEMUMachine: > return args > > def _pre_launch(self) -> None: > - self._temp_dir = tempfile.mkdtemp(prefix="qemu-machine-", > - dir=self._test_dir) > - self._qemu_log_path = os.path.join(self._temp_dir, self._name + ".log") > + self._qemu_log_path = os.path.join(self.temp_dir, self._name + ".log") > self._qemu_log_file = open(self._qemu_log_path, 'wb') > > if self._console_set: > @@ -744,3 +742,13 @@ class QEMUMachine: > file=self._console_log_path, > drain=self._drain_console) > return self._console_socket > + > + @property > + def temp_dir(self) -> str: > + """ > + Returns a temporary directory to be used for this machine > + """ > + if self._temp_dir is None: > + self._temp_dir = tempfile.mkdtemp(prefix="qemu-machine-", > + dir=self._base_temp_dir) > + return self._temp_dir Since this property has a side effect that will outlive the process, I think it ought to be a function (or ditch the side-effect.) The docstring should state that the directory will be created when this property is first accessed. (I realize you do it here because until we create the directory, we cannot return any particular value. Internal cleanup routines will need to be careful NOT to call temp_dir, though!) > diff --git a/python/qemu/qtest.py b/python/qemu/qtest.py > index 39a0cf62fe..78b97d13cf 100644 > --- a/python/qemu/qtest.py > +++ b/python/qemu/qtest.py > @@ -112,14 +112,14 @@ class QEMUQtestMachine(QEMUMachine): > binary: str, > args: Sequence[str] = (), > name: Optional[str] = None, > - test_dir: str = "/var/tmp", > + base_temp_dir: str = "/var/tmp", > socket_scm_helper: Optional[str] = None, > sock_dir: Optional[str] = None): > if name is None: > name = "qemu-%d" % os.getpid() > if sock_dir is None: > - sock_dir = test_dir > - super().__init__(binary, args, name=name, test_dir=test_dir, > + sock_dir = base_temp_dir > + super().__init__(binary, args, name=name, base_temp_dir=base_temp_dir, > socket_scm_helper=socket_scm_helper, > sock_dir=sock_dir) > self._qtest: Optional[QEMUQtestProtocol] = None > diff --git a/tests/acceptance/virtio-gpu.py b/tests/acceptance/virtio-gpu.py > index 211f02932f..8d689eb820 100644 > --- a/tests/acceptance/virtio-gpu.py > +++ b/tests/acceptance/virtio-gpu.py > @@ -119,7 +119,7 @@ class VirtioGPUx86(Test): > os.set_inheritable(vug_sock.fileno(), True) > > self._vug_log_path = os.path.join( > - self.vm._test_dir, "vhost-user-gpu.log" > + self.vm.temp_dir, "vhost-user-gpu.log" > ) > self._vug_log_file = open(self._vug_log_path, "wb") > print(self._vug_log_path) > diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py > index 00be68eca3..b02a3dc092 100644 > --- a/tests/qemu-iotests/iotests.py > +++ b/tests/qemu-iotests/iotests.py > @@ -562,7 +562,7 @@ class VM(qtest.QEMUQtestMachine): > def __init__(self, path_suffix=''): > name = "qemu%s-%d" % (path_suffix, os.getpid()) > super().__init__(qemu_prog, qemu_opts, name=name, > - test_dir=test_dir, > + base_temp_dir=test_dir, > socket_scm_helper=socket_scm_helper, > sock_dir=sock_dir) > self._num_drives = 0 > Seems OK otherwise, at a glance.
On 2/15/21 1:50 PM, Wainer dos Santos Moschetta wrote: > > In qtest.QEMUQtestMachine.__init__(), the argument named 'test_dir' > still make sense, right? > > - Wainer It might upset pylint/mypy to rename parameters in the initializer for a parent class. If we rename it in the base class, we should rename it in the descendants, too. (I say "might" because I have not yet worked out under the exact conditions that mypy will give you LSP warnings for initializer methods. It definitely doesn't always seem to, but I have run afoul of it enough times that I try to avoid it as a matter of habit now.)
On 2/11/21 6:35 PM, Philippe Mathieu-Daudé wrote: > Not this patch fault, but I see we use /var/tmp since commit > 66613974468 ("scripts: refactor the VM class in iotests for reuse"). > Can we use an OS agnostic method to get temp storage directory instead? > As a follow-up. Feel free to add wish-list things and other small nuisances to https://gitlab.com/jsnow/qemu/-/issues where I will, some day, migrate them to the real tracker when we get it set up. For now, that's just where I'm trying to keep track of my own python to-dos. --js
On 2/15/21 7:27 PM, John Snow wrote: > On 2/15/21 1:50 PM, Wainer dos Santos Moschetta wrote: >> >> In qtest.QEMUQtestMachine.__init__(), the argument named 'test_dir' >> still make sense, right? >> >> - Wainer > > It might upset pylint/mypy to rename parameters in the initializer for > a parent class. If we rename it in the base class, we should rename it > in the descendants, too. > > (I say "might" because I have not yet worked out under the exact > conditions that mypy will give you LSP warnings for initializer > methods. It definitely doesn't always seem to, but I have run afoul of > it enough times that I try to avoid it as a matter of habit now.) Thanks for the explanation, John! So Cleber just got another: Reviewed-by: Wainer dos Santos Moschetta <wainersm@redhat.com> > > >
diff --git a/python/qemu/machine.py b/python/qemu/machine.py index 6e44bda337..b379fcbe72 100644 --- a/python/qemu/machine.py +++ b/python/qemu/machine.py @@ -84,7 +84,7 @@ class QEMUMachine: args: Sequence[str] = (), wrapper: Sequence[str] = (), name: Optional[str] = None, - test_dir: str = "/var/tmp", + base_temp_dir: str = "/var/tmp", monitor_address: Optional[SocketAddrT] = None, socket_scm_helper: Optional[str] = None, sock_dir: Optional[str] = None, @@ -97,10 +97,10 @@ class QEMUMachine: @param args: list of extra arguments @param wrapper: list of arguments used as prefix to qemu binary @param name: prefix for socket and log file names (default: qemu-PID) - @param test_dir: where to create socket and log file + @param base_temp_dir: default location where temporary files are created @param monitor_address: address for QMP monitor @param socket_scm_helper: helper program, required for send_fd_scm() - @param sock_dir: where to create socket (overrides test_dir for sock) + @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 @note: Qemu process is not started until launch() is used. @@ -112,8 +112,8 @@ class QEMUMachine: self._wrapper = wrapper self._name = name or "qemu-%d" % os.getpid() - self._test_dir = test_dir - self._sock_dir = sock_dir or self._test_dir + self._base_temp_dir = base_temp_dir + self._sock_dir = sock_dir or self._base_temp_dir self._socket_scm_helper = socket_scm_helper if monitor_address is not None: @@ -303,9 +303,7 @@ class QEMUMachine: return args def _pre_launch(self) -> None: - self._temp_dir = tempfile.mkdtemp(prefix="qemu-machine-", - dir=self._test_dir) - self._qemu_log_path = os.path.join(self._temp_dir, self._name + ".log") + self._qemu_log_path = os.path.join(self.temp_dir, self._name + ".log") self._qemu_log_file = open(self._qemu_log_path, 'wb') if self._console_set: @@ -744,3 +742,13 @@ class QEMUMachine: file=self._console_log_path, drain=self._drain_console) return self._console_socket + + @property + def temp_dir(self) -> str: + """ + Returns a temporary directory to be used for this machine + """ + if self._temp_dir is None: + self._temp_dir = tempfile.mkdtemp(prefix="qemu-machine-", + dir=self._base_temp_dir) + return self._temp_dir diff --git a/python/qemu/qtest.py b/python/qemu/qtest.py index 39a0cf62fe..78b97d13cf 100644 --- a/python/qemu/qtest.py +++ b/python/qemu/qtest.py @@ -112,14 +112,14 @@ class QEMUQtestMachine(QEMUMachine): binary: str, args: Sequence[str] = (), name: Optional[str] = None, - test_dir: str = "/var/tmp", + base_temp_dir: str = "/var/tmp", socket_scm_helper: Optional[str] = None, sock_dir: Optional[str] = None): if name is None: name = "qemu-%d" % os.getpid() if sock_dir is None: - sock_dir = test_dir - super().__init__(binary, args, name=name, test_dir=test_dir, + sock_dir = base_temp_dir + super().__init__(binary, args, name=name, base_temp_dir=base_temp_dir, socket_scm_helper=socket_scm_helper, sock_dir=sock_dir) self._qtest: Optional[QEMUQtestProtocol] = None diff --git a/tests/acceptance/virtio-gpu.py b/tests/acceptance/virtio-gpu.py index 211f02932f..8d689eb820 100644 --- a/tests/acceptance/virtio-gpu.py +++ b/tests/acceptance/virtio-gpu.py @@ -119,7 +119,7 @@ class VirtioGPUx86(Test): os.set_inheritable(vug_sock.fileno(), True) self._vug_log_path = os.path.join( - self.vm._test_dir, "vhost-user-gpu.log" + self.vm.temp_dir, "vhost-user-gpu.log" ) self._vug_log_file = open(self._vug_log_path, "wb") print(self._vug_log_path) diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py index 00be68eca3..b02a3dc092 100644 --- a/tests/qemu-iotests/iotests.py +++ b/tests/qemu-iotests/iotests.py @@ -562,7 +562,7 @@ class VM(qtest.QEMUQtestMachine): def __init__(self, path_suffix=''): name = "qemu%s-%d" % (path_suffix, os.getpid()) super().__init__(qemu_prog, qemu_opts, name=name, - test_dir=test_dir, + base_temp_dir=test_dir, socket_scm_helper=socket_scm_helper, sock_dir=sock_dir) self._num_drives = 0
Each instance of qemu.machine.QEMUMachine currently has a "test directory", which may not have any relation to a "test", and it's really a temporary directory. Users instantiating the QEMUMachine class will be able to set the location of the directory that will *contain* the QEMUMachine unique temporary directory, so that parameter name has been changed from test_dir to base_temp_dir. A property has been added to allow users to access it without using private attributes, and with that, the directory is created on first use of the property. Signed-off-by: Cleber Rosa <crosa@redhat.com> --- python/qemu/machine.py | 24 ++++++++++++++++-------- python/qemu/qtest.py | 6 +++--- tests/acceptance/virtio-gpu.py | 2 +- tests/qemu-iotests/iotests.py | 2 +- 4 files changed, 21 insertions(+), 13 deletions(-)