Re: [PATCH 2/6] Python: expose QEMUMachine's temporary directory

[John Snow <jsnow@redhat.com>]

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.