[PATCH 0/6] Python / Acceptance Tests: improve logging

[PATCH 0/6] Python / Acceptance Tests: improve logging

[Cleber Rosa]

The location and amount of information kept while using QEMUMachine in
Acceptance Tests is currently not optimal.

This improves the situation by using the Test's log directory (an
Avocado standard feature) as the default location to keep logs,
instead of the temporary directory currently used.

Users will be able to find "qemu-$PID.log" files under the test log
directories, containing all the stdout/stderr generated by the QEMU
binary.

Cleber Rosa (6):
  Python: close the log file kept by QEMUMachine before reading it
  Python: expose QEMUMachine's temporary directory
  Acceptance Tests: use the job work directory for created VMs
  Acceptance Tests: log information when creating QEMUMachine
  Acceptance Tests: distinguish between temp and logs dir
  tests/acceptance/virtio-gpu.py: preserve virtio-user-gpu log

 python/qemu/machine.py                    | 42 +++++++++++++++++------
 python/qemu/qtest.py                      |  6 ++--
 tests/acceptance/avocado_qemu/__init__.py | 10 ++++--
 tests/acceptance/virtio-gpu.py            |  5 +--
 tests/qemu-iotests/iotests.py             |  2 +-
 5 files changed, 45 insertions(+), 20 deletions(-)

-- 
2.25.4

[PATCH 1/6] Python: close the log file kept by QEMUMachine before reading it

[Cleber Rosa]

Closing a file that is open for writing, and then reading from it
sounds like a better idea than the opposite, given that the content
will be flushed.

Reference: https://docs.python.org/3/library/io.html#io.IOBase.close
Signed-off-by: Cleber Rosa <crosa@redhat.com>
---
 python/qemu/machine.py | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/python/qemu/machine.py b/python/qemu/machine.py
index 7a40f4604b..6e44bda337 100644
--- a/python/qemu/machine.py
+++ b/python/qemu/machine.py
@@ -337,12 +337,12 @@ class QEMUMachine:
             self._qmp.close()
             self._qmp_connection = None
 
-        self._load_io_log()
-
         if self._qemu_log_file is not None:
             self._qemu_log_file.close()
             self._qemu_log_file = None
 
+        self._load_io_log()
+
         self._qemu_log_path = None
 
         if self._temp_dir is not None:
-- 
2.25.4

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

[Cleber Rosa]

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",
                  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
-- 
2.25.4

[PATCH 3/6] Acceptance Tests: use the job work directory for created VMs

[Cleber Rosa]

The QEMUMachine uses a base temporary directory for all temporary
needs.  By setting it to the Avocado's workdir, it's possible to
keep the temporary files during debugging sessions much more
easily by setting the "--keep-tmp" command line option.

Reference: https://avocado-framework.readthedocs.io/en/85.0/api/test/avocado.html#avocado.Test.workdir
Reference: https://avocado-framework.readthedocs.io/en/85.0/config/index.html#run-keep-tmp
Signed-off-by: Cleber Rosa <crosa@redhat.com>
---
 tests/acceptance/avocado_qemu/__init__.py | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/tests/acceptance/avocado_qemu/__init__.py b/tests/acceptance/avocado_qemu/__init__.py
index bf54e419da..b7ab836455 100644
--- a/tests/acceptance/avocado_qemu/__init__.py
+++ b/tests/acceptance/avocado_qemu/__init__.py
@@ -172,7 +172,8 @@ class Test(avocado.Test):
 
     def _new_vm(self, *args):
         self._sd = tempfile.TemporaryDirectory(prefix="avo_qemu_sock_")
-        vm = QEMUMachine(self.qemu_bin, sock_dir=self._sd.name)
+        vm = QEMUMachine(self.qemu_bin, base_temp_dir=self.workdir,
+                         sock_dir=self._sd.name)
         if args:
             vm.add_args(*args)
         return vm
-- 
2.25.4

[PATCH 4/6] Acceptance Tests: log information when creating QEMUMachine

[Cleber Rosa]

Including its base temporary directory, given that information useful
for debugging can be put there.

Signed-off-by: Cleber Rosa <crosa@redhat.com>
---
 tests/acceptance/avocado_qemu/__init__.py | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/tests/acceptance/avocado_qemu/__init__.py b/tests/acceptance/avocado_qemu/__init__.py
index b7ab836455..94b78fd7c8 100644
--- a/tests/acceptance/avocado_qemu/__init__.py
+++ b/tests/acceptance/avocado_qemu/__init__.py
@@ -170,10 +170,12 @@ class Test(avocado.Test):
         if self.qemu_bin is None:
             self.cancel("No QEMU binary defined or found in the build tree")
 
-    def _new_vm(self, *args):
+    def _new_vm(self, name, *args):
         self._sd = tempfile.TemporaryDirectory(prefix="avo_qemu_sock_")
         vm = QEMUMachine(self.qemu_bin, base_temp_dir=self.workdir,
                          sock_dir=self._sd.name)
+        self.log.debug('QEMUMachine "%s" created', name)
+        self.log.debug('QEMUMachine "%s" temp_dir: %s', name, vm.temp_dir)
         if args:
             vm.add_args(*args)
         return vm
@@ -186,7 +188,7 @@ class Test(avocado.Test):
         if not name:
             name = str(uuid.uuid4())
         if self._vms.get(name) is None:
-            self._vms[name] = self._new_vm(*args)
+            self._vms[name] = self._new_vm(name, *args)
             if self.machine is not None:
                 self._vms[name].set_machine(self.machine)
         return self._vms[name]
-- 
2.25.4

[PATCH 5/6] Acceptance Tests: distinguish between temp and logs dir

[Cleber Rosa]

Logs can be very important to debug issues, and currently QEMUMachine
instances will remove logs that are created under the temporary
directories.

With this change, the stdout and stderr generated by the QEMU process
started by QEMUMachine will always be kept along the test results
directory.

Signed-off-by: Cleber Rosa <crosa@redhat.com>
---
 python/qemu/machine.py                    | 16 ++++++++++++++--
 tests/acceptance/avocado_qemu/__init__.py |  3 ++-
 2 files changed, 16 insertions(+), 3 deletions(-)

diff --git a/python/qemu/machine.py b/python/qemu/machine.py
index b379fcbe72..1f4119e2b4 100644
--- a/python/qemu/machine.py
+++ b/python/qemu/machine.py
@@ -89,7 +89,8 @@ class QEMUMachine:
                  socket_scm_helper: Optional[str] = None,
                  sock_dir: Optional[str] = None,
                  drain_console: bool = False,
-                 console_log: Optional[str] = None):
+                 console_log: Optional[str] = None,
+                 log_dir: Optional[str] = None):
         '''
         Initialize a QEMUMachine
 
@@ -103,6 +104,7 @@ class QEMUMachine:
         @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
         @note: Qemu process is not started until launch() is used.
         '''
         # Direct user configuration
@@ -114,6 +116,7 @@ class QEMUMachine:
         self._name = name or "qemu-%d" % os.getpid()
         self._base_temp_dir = base_temp_dir
         self._sock_dir = sock_dir or self._base_temp_dir
+        self._log_dir = log_dir
         self._socket_scm_helper = socket_scm_helper
 
         if monitor_address is not None:
@@ -303,7 +306,7 @@ class QEMUMachine:
         return args
 
     def _pre_launch(self) -> None:
-        self._qemu_log_path = os.path.join(self.temp_dir, self._name + ".log")
+        self._qemu_log_path = os.path.join(self.log_dir, self._name + ".log")
         self._qemu_log_file = open(self._qemu_log_path, 'wb')
 
         if self._console_set:
@@ -752,3 +755,12 @@ class QEMUMachine:
             self._temp_dir = tempfile.mkdtemp(prefix="qemu-machine-",
                                               dir=self._base_temp_dir)
         return self._temp_dir
+
+    @property
+    def log_dir(self) -> str:
+        """
+        Returns a directory to be used for writing logs
+        """
+        if self._log_dir is None:
+            return self.temp_dir
+        return self._log_dir
diff --git a/tests/acceptance/avocado_qemu/__init__.py b/tests/acceptance/avocado_qemu/__init__.py
index 94b78fd7c8..ac9be1eb66 100644
--- a/tests/acceptance/avocado_qemu/__init__.py
+++ b/tests/acceptance/avocado_qemu/__init__.py
@@ -173,9 +173,10 @@ class Test(avocado.Test):
     def _new_vm(self, name, *args):
         self._sd = tempfile.TemporaryDirectory(prefix="avo_qemu_sock_")
         vm = QEMUMachine(self.qemu_bin, base_temp_dir=self.workdir,
-                         sock_dir=self._sd.name)
+                         sock_dir=self._sd.name, 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)
         if args:
             vm.add_args(*args)
         return vm
-- 
2.25.4

[PATCH 6/6] tests/acceptance/virtio-gpu.py: preserve virtio-user-gpu log

[Cleber Rosa]

At location already prepared for keeping the test's log files.

While at it, log info about its location (in the main test log
file), instead of printing it out.

Reference: https://avocado-framework.readthedocs.io/en/85.0/api/test/avocado.html#avocado.Test.logdir
Signed-off-by: Cleber Rosa <crosa@redhat.com>
---
 tests/acceptance/virtio-gpu.py | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/tests/acceptance/virtio-gpu.py b/tests/acceptance/virtio-gpu.py
index 8d689eb820..ab1a4c1a71 100644
--- a/tests/acceptance/virtio-gpu.py
+++ b/tests/acceptance/virtio-gpu.py
@@ -119,10 +119,11 @@ class VirtioGPUx86(Test):
         os.set_inheritable(vug_sock.fileno(), True)
 
         self._vug_log_path = os.path.join(
-            self.vm.temp_dir, "vhost-user-gpu.log"
+            self.logdir, "vhost-user-gpu.log"
         )
         self._vug_log_file = open(self._vug_log_path, "wb")
-        print(self._vug_log_path)
+        self.log.info('Complete vhost-user-gpu.log file can be '
+                      'found at %s', self._vug_log_path)
 
         vugp = subprocess.Popen(
             [vug, "--virgl", "--fd=%d" % vug_sock.fileno()],
-- 
2.25.4

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

[Philippe Mathieu-Daudé]

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?

Re: [PATCH 6/6] tests/acceptance/virtio-gpu.py: preserve virtio-user-gpu log

[Philippe Mathieu-Daudé]

On 2/11/21 11:01 PM, Cleber Rosa wrote:

"Preserve log" ...
> At location already prepared for keeping the test's log files.
> 
> While at it, log info about its location (in the main test log
> file), instead of printing it out.
> 
> Reference: https://avocado-framework.readthedocs.io/en/85.0/api/test/avocado.html#avocado.Test.logdir
> Signed-off-by: Cleber Rosa <crosa@redhat.com>
> ---
>  tests/acceptance/virtio-gpu.py | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)

Using full sentence:
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

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

[Cleber Rosa]

[-- Attachment #1: Type: text/plain, Size: 2073 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 1/6] Python: close the log file kept by QEMUMachine before reading it

[Wainer dos Santos Moschetta]

Hi,

On 2/11/21 7:01 PM, Cleber Rosa wrote:
> Closing a file that is open for writing, and then reading from it
> sounds like a better idea than the opposite, given that the content
> will be flushed.
>
> Reference: https://docs.python.org/3/library/io.html#io.IOBase.close
> Signed-off-by: Cleber Rosa <crosa@redhat.com>
> ---
>   python/qemu/machine.py | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/python/qemu/machine.py b/python/qemu/machine.py
> index 7a40f4604b..6e44bda337 100644
> --- a/python/qemu/machine.py
> +++ b/python/qemu/machine.py
> @@ -337,12 +337,12 @@ class QEMUMachine:
>               self._qmp.close()
>               self._qmp_connection = None
>   
> -        self._load_io_log()
> -
>           if self._qemu_log_file is not None:
>               self._qemu_log_file.close()
>               self._qemu_log_file = None
>   
> +        self._load_io_log()
> +


IMO it's a too fragile fix. It needs the `self._qemu_log_file.close()` 
being called before `self._load_io_log()` but a future change can make 
this condition unmet again. Maybe you could document that in the code. 
Or change the `_load_io_log()` to close the log file if it is open (also 
document it in code).

- Wainer

>           self._qemu_log_path = None
>   
>           if self._temp_dir is not None:

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

[Wainer dos Santos Moschetta]

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

Re: [PATCH 3/6] Acceptance Tests: use the job work directory for created VMs

[Wainer dos Santos Moschetta]

On 2/11/21 7:01 PM, Cleber Rosa wrote:
> The QEMUMachine uses a base temporary directory for all temporary
> needs.  By setting it to the Avocado's workdir, it's possible to
> keep the temporary files during debugging sessions much more
> easily by setting the "--keep-tmp" command line option.
>
> Reference: https://avocado-framework.readthedocs.io/en/85.0/api/test/avocado.html#avocado.Test.workdir
> Reference: https://avocado-framework.readthedocs.io/en/85.0/config/index.html#run-keep-tmp
> Signed-off-by: Cleber Rosa <crosa@redhat.com>
> ---
>   tests/acceptance/avocado_qemu/__init__.py | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)

The changes look good to me, so:

Reviewed-by: Wainer dos Santos Moschetta <wainersm@redhat.com>

But I got confused, the patch's subject states "use the job work 
directory" while the documentation of `workdir` says it is a "writable 
directory that exists during the entire test execution (...)". In the 
end is it a job or test work directory?

- Wainer

>
> diff --git a/tests/acceptance/avocado_qemu/__init__.py b/tests/acceptance/avocado_qemu/__init__.py
> index bf54e419da..b7ab836455 100644
> --- a/tests/acceptance/avocado_qemu/__init__.py
> +++ b/tests/acceptance/avocado_qemu/__init__.py
> @@ -172,7 +172,8 @@ class Test(avocado.Test):
>   
>       def _new_vm(self, *args):
>           self._sd = tempfile.TemporaryDirectory(prefix="avo_qemu_sock_")
> -        vm = QEMUMachine(self.qemu_bin, sock_dir=self._sd.name)
> +        vm = QEMUMachine(self.qemu_bin, base_temp_dir=self.workdir,
> +                         sock_dir=self._sd.name)
>           if args:
>               vm.add_args(*args)
>           return vm

Re: [PATCH 4/6] Acceptance Tests: log information when creating QEMUMachine

[Wainer dos Santos Moschetta]

On 2/11/21 7:01 PM, Cleber Rosa wrote:
> Including its base temporary directory, given that information useful
> for debugging can be put there.
>
> Signed-off-by: Cleber Rosa <crosa@redhat.com>
> ---
>   tests/acceptance/avocado_qemu/__init__.py | 6 ++++--
>   1 file changed, 4 insertions(+), 2 deletions(-)


Reviewed-by: Wainer dos Santos Moschetta <wainersm@redhat.com>


>
> diff --git a/tests/acceptance/avocado_qemu/__init__.py b/tests/acceptance/avocado_qemu/__init__.py
> index b7ab836455..94b78fd7c8 100644
> --- a/tests/acceptance/avocado_qemu/__init__.py
> +++ b/tests/acceptance/avocado_qemu/__init__.py
> @@ -170,10 +170,12 @@ class Test(avocado.Test):
>           if self.qemu_bin is None:
>               self.cancel("No QEMU binary defined or found in the build tree")
>   
> -    def _new_vm(self, *args):
> +    def _new_vm(self, name, *args):
>           self._sd = tempfile.TemporaryDirectory(prefix="avo_qemu_sock_")
>           vm = QEMUMachine(self.qemu_bin, base_temp_dir=self.workdir,
>                            sock_dir=self._sd.name)
> +        self.log.debug('QEMUMachine "%s" created', name)
> +        self.log.debug('QEMUMachine "%s" temp_dir: %s', name, vm.temp_dir)
>           if args:
>               vm.add_args(*args)
>           return vm
> @@ -186,7 +188,7 @@ class Test(avocado.Test):
>           if not name:
>               name = str(uuid.uuid4())
>           if self._vms.get(name) is None:
> -            self._vms[name] = self._new_vm(*args)
> +            self._vms[name] = self._new_vm(name, *args)
>               if self.machine is not None:
>                   self._vms[name].set_machine(self.machine)
>           return self._vms[name]

Re: [PATCH 5/6] Acceptance Tests: distinguish between temp and logs dir

[Wainer dos Santos Moschetta]

Hi,

On 2/11/21 7:01 PM, Cleber Rosa wrote:
> Logs can be very important to debug issues, and currently QEMUMachine
> instances will remove logs that are created under the temporary
> directories.
>
> With this change, the stdout and stderr generated by the QEMU process
> started by QEMUMachine will always be kept along the test results
> directory.
>
> Signed-off-by: Cleber Rosa <crosa@redhat.com>
> ---
>   python/qemu/machine.py                    | 16 ++++++++++++++--
>   tests/acceptance/avocado_qemu/__init__.py |  3 ++-
>   2 files changed, 16 insertions(+), 3 deletions(-)
>
> diff --git a/python/qemu/machine.py b/python/qemu/machine.py
> index b379fcbe72..1f4119e2b4 100644
> --- a/python/qemu/machine.py
> +++ b/python/qemu/machine.py
> @@ -89,7 +89,8 @@ class QEMUMachine:
>                    socket_scm_helper: Optional[str] = None,
>                    sock_dir: Optional[str] = None,
>                    drain_console: bool = False,
> -                 console_log: Optional[str] = None):
> +                 console_log: Optional[str] = None,
> +                 log_dir: Optional[str] = None):
>           '''
>           Initialize a QEMUMachine
>   
> @@ -103,6 +104,7 @@ class QEMUMachine:
>           @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

"(optional) where to create and keep (...)".

You could also say it defaults to the temp dir, thus logs aren't kept 
guaranteed.

Otherwise,

Reviewed-by: Wainer dos Santos Moschetta <wainersm@redhat.com>

>           @note: Qemu process is not started until launch() is used.
>           '''
>           # Direct user configuration
> @@ -114,6 +116,7 @@ class QEMUMachine:
>           self._name = name or "qemu-%d" % os.getpid()
>           self._base_temp_dir = base_temp_dir
>           self._sock_dir = sock_dir or self._base_temp_dir
> +        self._log_dir = log_dir
>           self._socket_scm_helper = socket_scm_helper
>   
>           if monitor_address is not None:
> @@ -303,7 +306,7 @@ class QEMUMachine:
>           return args
>   
>       def _pre_launch(self) -> None:
> -        self._qemu_log_path = os.path.join(self.temp_dir, self._name + ".log")
> +        self._qemu_log_path = os.path.join(self.log_dir, self._name + ".log")
>           self._qemu_log_file = open(self._qemu_log_path, 'wb')
>   
>           if self._console_set:
> @@ -752,3 +755,12 @@ class QEMUMachine:
>               self._temp_dir = tempfile.mkdtemp(prefix="qemu-machine-",
>                                                 dir=self._base_temp_dir)
>           return self._temp_dir
> +
> +    @property
> +    def log_dir(self) -> str:
> +        """
> +        Returns a directory to be used for writing logs
> +        """
> +        if self._log_dir is None:
> +            return self.temp_dir
> +        return self._log_dir
> diff --git a/tests/acceptance/avocado_qemu/__init__.py b/tests/acceptance/avocado_qemu/__init__.py
> index 94b78fd7c8..ac9be1eb66 100644
> --- a/tests/acceptance/avocado_qemu/__init__.py
> +++ b/tests/acceptance/avocado_qemu/__init__.py
> @@ -173,9 +173,10 @@ class Test(avocado.Test):
>       def _new_vm(self, name, *args):
>           self._sd = tempfile.TemporaryDirectory(prefix="avo_qemu_sock_")
>           vm = QEMUMachine(self.qemu_bin, base_temp_dir=self.workdir,
> -                         sock_dir=self._sd.name)
> +                         sock_dir=self._sd.name, 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)
>           if args:
>               vm.add_args(*args)
>           return vm

Re: [PATCH 6/6] tests/acceptance/virtio-gpu.py: preserve virtio-user-gpu log

[Wainer dos Santos Moschetta]

On 2/11/21 7:01 PM, Cleber Rosa wrote:
> At location already prepared for keeping the test's log files.
>
> While at it, log info about its location (in the main test log
> file), instead of printing it out.
>
> Reference: https://avocado-framework.readthedocs.io/en/85.0/api/test/avocado.html#avocado.Test.logdir
> Signed-off-by: Cleber Rosa <crosa@redhat.com>
> ---
>   tests/acceptance/virtio-gpu.py | 5 +++--
>   1 file changed, 3 insertions(+), 2 deletions(-)


Reviewed-by: Wainer dos Santos Moschetta <wainersm@redhat.com>


>
> diff --git a/tests/acceptance/virtio-gpu.py b/tests/acceptance/virtio-gpu.py
> index 8d689eb820..ab1a4c1a71 100644
> --- a/tests/acceptance/virtio-gpu.py
> +++ b/tests/acceptance/virtio-gpu.py
> @@ -119,10 +119,11 @@ class VirtioGPUx86(Test):
>           os.set_inheritable(vug_sock.fileno(), True)
>   
>           self._vug_log_path = os.path.join(
> -            self.vm.temp_dir, "vhost-user-gpu.log"
> +            self.logdir, "vhost-user-gpu.log"
>           )
>           self._vug_log_file = open(self._vug_log_path, "wb")
> -        print(self._vug_log_path)
> +        self.log.info('Complete vhost-user-gpu.log file can be '
> +                      'found at %s', self._vug_log_path)
>   
>           vugp = subprocess.Popen(
>               [vug, "--virgl", "--fd=%d" % vug_sock.fileno()],

Re: [PATCH 1/6] Python: close the log file kept by QEMUMachine before reading it

[John Snow]

On 2/11/21 5:01 PM, Cleber Rosa wrote:
> Closing a file that is open for writing, and then reading from it
> sounds like a better idea than the opposite, given that the content
> will be flushed.
> 
> Reference: https://docs.python.org/3/library/io.html#io.IOBase.close
> Signed-off-by: Cleber Rosa <crosa@redhat.com>
> ---
>   python/qemu/machine.py | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/python/qemu/machine.py b/python/qemu/machine.py
> index 7a40f4604b..6e44bda337 100644
> --- a/python/qemu/machine.py
> +++ b/python/qemu/machine.py
> @@ -337,12 +337,12 @@ class QEMUMachine:

Is there a way to improve context for python functions? What method is 
this in? etc.

>               self._qmp.close()
>               self._qmp_connection = None
>   
> -        self._load_io_log()
> -
>           if self._qemu_log_file is not None:
>               self._qemu_log_file.close()
>               self._qemu_log_file = None
>   
> +        self._load_io_log()
> +
>           self._qemu_log_path = None
>   
>           if self._temp_dir is not None:
> 

Yeh, seems fine, though as wainer points out the interdependencies 
between _load_io_log, _qemu_log_file and _qemu_log_path are not all 
strictly clear, so it seems fragile.

But, this is more correct than it was, so:

Reviewed-by: John Snow <jsnow@redhat.com>

Re: [PATCH 1/6] Python: close the log file kept by QEMUMachine before reading it

[Eric Blake]

On 2/15/21 4:04 PM, John Snow wrote:
> On 2/11/21 5:01 PM, Cleber Rosa wrote:
>> Closing a file that is open for writing, and then reading from it
>> sounds like a better idea than the opposite, given that the content
>> will be flushed.
>>
>> Reference: https://docs.python.org/3/library/io.html#io.IOBase.close
>> Signed-off-by: Cleber Rosa <crosa@redhat.com>
>> ---
>>   python/qemu/machine.py | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/python/qemu/machine.py b/python/qemu/machine.py
>> index 7a40f4604b..6e44bda337 100644
>> --- a/python/qemu/machine.py
>> +++ b/python/qemu/machine.py
>> @@ -337,12 +337,12 @@ class QEMUMachine:
> 
> Is there a way to improve context for python functions? What method is
> this in? etc.

'man gitattributes' suggests that having this line in .gitattributes
would help:
*.py diff=python

/me goes to play with it...

Does this look better to you?  It certainly does to me!  I'll go ahead
and propose the .gitattributes change as a formal patch...

 --- a/python/qemu/machine.py
 +++ b/python/qemu/machine.py
-@@ -337,12 +337,12 @@ class QEMUMachine:
+@@ -337,12 +337,12 @@ def _post_shutdown(self) -> None:
              self._qmp.close()

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

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

[John Snow]

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.

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

[John Snow]

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.)

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

[John Snow]

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

Re: [PATCH 3/6] Acceptance Tests: use the job work directory for created VMs

[John Snow]

On 2/11/21 5:01 PM, Cleber Rosa wrote:
> The QEMUMachine uses a base temporary directory for all temporary
> needs.  By setting it to the Avocado's workdir, it's possible to
> keep the temporary files during debugging sessions much more
> easily by setting the "--keep-tmp" command line option.
> 

Makes sense, kinda-sorta.

Is this a band-aid?

QEMUMachine has this gem in _post_shutdown():

         if self._temp_dir is not None:
             shutil.rmtree(self._temp_dir)
             self._temp_dir = None

we probably do want a way to adjust the deletion policy that QEMUMachine 
has. Kevin and I discussed this (extremely briefly) in January. One note 
is that the QEMU logs are read into memory when the process closes, so 
iotests et al have a chance to show you those logs on failure cases. Not 
relevant here for your purposes.

Meanwhile, we could also change the behavior of QEMUMachine, and create 
a temp dir deletion policy tunable:

(1) Always delete
(2) Never delete
(3) Delete on success (keep on failure)
(4) Delete on success and anticipated failures.

(About #4: QEMUMachine has a condition where it will not report 
AbornalShutdown if .kill() is called and the retcode is observed to be 
-SIGKILL. We treat this as a kind of success.)

These avocado tests could then just use a "never delete" policy, use the 
avocado runner's working dir, and then never worry about the cleanup.

iotests could do something similar with a temporary directory 
established by the top-level runner that we could modify the behavior of 
with --keep-files[ <on-failure|always>] or similar.

It might be time to just make this less stupidly annoying for all users 
of the library once and for all.

> Reference: https://avocado-framework.readthedocs.io/en/85.0/api/test/avocado.html#avocado.Test.workdir
> Reference: https://avocado-framework.readthedocs.io/en/85.0/config/index.html#run-keep-tmp
> Signed-off-by: Cleber Rosa <crosa@redhat.com>
> ---
>   tests/acceptance/avocado_qemu/__init__.py | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/tests/acceptance/avocado_qemu/__init__.py b/tests/acceptance/avocado_qemu/__init__.py
> index bf54e419da..b7ab836455 100644
> --- a/tests/acceptance/avocado_qemu/__init__.py
> +++ b/tests/acceptance/avocado_qemu/__init__.py
> @@ -172,7 +172,8 @@ class Test(avocado.Test):
>   
>       def _new_vm(self, *args):
>           self._sd = tempfile.TemporaryDirectory(prefix="avo_qemu_sock_")
> -        vm = QEMUMachine(self.qemu_bin, sock_dir=self._sd.name)
> +        vm = QEMUMachine(self.qemu_bin, base_temp_dir=self.workdir,
> +                         sock_dir=self._sd.name)
>           if args:
>               vm.add_args(*args)
>           return vm
> 

But, you know, absent all that extra work, sure:

Reviewed-by: John Snow <jsnow@redhat.com>

Re: [PATCH 1/6] Python: close the log file kept by QEMUMachine before reading it

[Cleber Rosa]

[-- Attachment #1: Type: text/plain, Size: 1610 bytes --]

On Mon, Feb 15, 2021 at 03:30:16PM -0300, Wainer dos Santos Moschetta wrote:
> Hi,
> 
> On 2/11/21 7:01 PM, Cleber Rosa wrote:
> > Closing a file that is open for writing, and then reading from it
> > sounds like a better idea than the opposite, given that the content
> > will be flushed.
> > 
> > Reference: https://docs.python.org/3/library/io.html#io.IOBase.close
> > Signed-off-by: Cleber Rosa <crosa@redhat.com>
> > ---
> >   python/qemu/machine.py | 4 ++--
> >   1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/python/qemu/machine.py b/python/qemu/machine.py
> > index 7a40f4604b..6e44bda337 100644
> > --- a/python/qemu/machine.py
> > +++ b/python/qemu/machine.py
> > @@ -337,12 +337,12 @@ class QEMUMachine:
> >               self._qmp.close()
> >               self._qmp_connection = None
> > -        self._load_io_log()
> > -
> >           if self._qemu_log_file is not None:
> >               self._qemu_log_file.close()
> >               self._qemu_log_file = None
> > +        self._load_io_log()
> > +
> 
> 
> IMO it's a too fragile fix. It needs the `self._qemu_log_file.close()` being
> called before `self._load_io_log()` but a future change can make this
> condition unmet again. Maybe you could document that in the code. Or change
> the `_load_io_log()` to close the log file if it is open (also document it
> in code).
> 
> - Wainer
>

I'm glad you see this is needed... and then something else.  I'll investigate
making this more robust as time allows it.

Question is: do you ack/nack this change?

Cheers,
- Cleber.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 1/6] Python: close the log file kept by QEMUMachine before reading it

[Cleber Rosa]

[-- Attachment #1: Type: text/plain, Size: 1614 bytes --]

On Mon, Feb 15, 2021 at 05:04:24PM -0500, John Snow wrote:
> On 2/11/21 5:01 PM, Cleber Rosa wrote:
> > Closing a file that is open for writing, and then reading from it
> > sounds like a better idea than the opposite, given that the content
> > will be flushed.
> > 
> > Reference: https://docs.python.org/3/library/io.html#io.IOBase.close
> > Signed-off-by: Cleber Rosa <crosa@redhat.com>
> > ---
> >   python/qemu/machine.py | 4 ++--
> >   1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/python/qemu/machine.py b/python/qemu/machine.py
> > index 7a40f4604b..6e44bda337 100644
> > --- a/python/qemu/machine.py
> > +++ b/python/qemu/machine.py
> > @@ -337,12 +337,12 @@ class QEMUMachine:
> 
> Is there a way to improve context for python functions? What method is this
> in? etc.
> 
> >               self._qmp.close()
> >               self._qmp_connection = None
> > -        self._load_io_log()
> > -
> >           if self._qemu_log_file is not None:
> >               self._qemu_log_file.close()
> >               self._qemu_log_file = None
> > +        self._load_io_log()
> > +
> >           self._qemu_log_path = None
> >           if self._temp_dir is not None:
> > 
> 
> Yeh, seems fine, though as wainer points out the interdependencies between
> _load_io_log, _qemu_log_file and _qemu_log_path are not all strictly clear,
> so it seems fragile.
>

Yep, agreed.  This was a first, conservative change.  Expect more later.

> But, this is more correct than it was, so:
> 
> Reviewed-by: John Snow <jsnow@redhat.com>

Thanks,
- Cleber

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 1/6] Python: close the log file kept by QEMUMachine before reading it

[Wainer dos Santos Moschetta]

On 2/15/21 11:34 PM, Cleber Rosa wrote:
> On Mon, Feb 15, 2021 at 03:30:16PM -0300, Wainer dos Santos Moschetta wrote:
>> Hi,
>>
>> On 2/11/21 7:01 PM, Cleber Rosa wrote:
>>> Closing a file that is open for writing, and then reading from it
>>> sounds like a better idea than the opposite, given that the content
>>> will be flushed.
>>>
>>> Reference: https://docs.python.org/3/library/io.html#io.IOBase.close
>>> Signed-off-by: Cleber Rosa <crosa@redhat.com>
>>> ---
>>>    python/qemu/machine.py | 4 ++--
>>>    1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/python/qemu/machine.py b/python/qemu/machine.py
>>> index 7a40f4604b..6e44bda337 100644
>>> --- a/python/qemu/machine.py
>>> +++ b/python/qemu/machine.py
>>> @@ -337,12 +337,12 @@ class QEMUMachine:
>>>                self._qmp.close()
>>>                self._qmp_connection = None
>>> -        self._load_io_log()
>>> -
>>>            if self._qemu_log_file is not None:
>>>                self._qemu_log_file.close()
>>>                self._qemu_log_file = None
>>> +        self._load_io_log()
>>> +
>>
>> IMO it's a too fragile fix. It needs the `self._qemu_log_file.close()` being
>> called before `self._load_io_log()` but a future change can make this
>> condition unmet again. Maybe you could document that in the code. Or change
>> the `_load_io_log()` to close the log file if it is open (also document it
>> in code).
>>
>> - Wainer
>>
> I'm glad you see this is needed... and then something else.  I'll investigate
> making this more robust as time allows it.
>
> Question is: do you ack/nack this change?

hmm... /me thinking hmmm... okay, good deal. :)

Acked-by: Wainer dos Santos Moschetta <wainersm@redhat.com>


>
> Cheers,
> - Cleber.

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

[Wainer dos Santos Moschetta]

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>


>
>
>