[v3,11/15] qemu_iotests: allow valgrind to read/delete the generated log file

Commit Message

Emanuele Giuseppe Esposito April 14, 2021, 5:03 p.m. UTC

When using -valgrind on the script tests, it generates a log file
in $TEST_DIR that is either read (if valgrind finds problems) or
otherwise deleted. Provide the same exact behavior when using
-valgrind on the python tests.

Signed-off-by: Emanuele Giuseppe Esposito <[e-mail]>
---
 tests/qemu-iotests/iotests.py | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

Comments

Max Reitz April 30, 2021, 1:17 p.m. UTC | #1

On 14.04.21 19:03, Emanuele Giuseppe Esposito wrote:
> When using -valgrind on the script tests, it generates a log file
> in $TEST_DIR that is either read (if valgrind finds problems) or
> otherwise deleted. Provide the same exact behavior when using
> -valgrind on the python tests.
> 
> Signed-off-by: Emanuele Giuseppe Esposito <[e-mail]>
> ---
>   tests/qemu-iotests/iotests.py | 20 ++++++++++++++++++++
>   1 file changed, 20 insertions(+)
> 
> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
> index 94597433fa..aef67e3a86 100644
> --- a/tests/qemu-iotests/iotests.py
> +++ b/tests/qemu-iotests/iotests.py
> @@ -600,6 +600,26 @@ def __init__(self, path_suffix=''):
>                            sock_dir=sock_dir)
>           self._num_drives = 0
>   
> +    def subprocess_check_valgrind(self, valgrind) -> None:

A type annotation would be nice.  (I.e. List[str], or we make it a bool 
and the caller uses bool(qemu_valgrind).)

> +

I’d drop this empty line.

> +        if not valgrind:
> +            return
> +
> +        valgrind_filename =  test_dir + "/" + str(self._popen.pid) + ".valgrind"

mypy (iotest 297) complains that _popen is Optional[], so .pid might not 
exist.  Perhaps this should be safeguarded in the "if not valgrind" 
condition (i.e. "if not valgrind or not self._popen").

Also, pylint complains about the line being too long (79 is the maximum 
length).  Can be fixed with an f-string:

valgrind_filename = f"{test_dir}/{self._popen.pid}.valgrind"

> +
> +        if self.exitcode() == 99:
> +            with open(valgrind_filename) as f:
> +                content = f.readlines()
> +            for line in content:
> +                print(line, end ="")

'end=""' would be better, I think.  (flake8 complains.)

Also, would this be better as:

with open(valgrind_filename) as f:
     for line in f.readlines():
         print(line, end="")

?

(Or just

with open(valgrind_filename) as f:
     print(f.read())

– wouldn’t that work, too?)

Max

> +            print("")
> +        else:
> +            os.remove(valgrind_filename)
> +
> +    def _post_shutdown(self) -> None:
> +        super()._post_shutdown()
> +        self.subprocess_check_valgrind(qemu_valgrind)
> +
>       def add_object(self, opts):
>           self._args.append('-object')
>           self._args.append(opts)
>

Patch

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 94597433fa..aef67e3a86 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -600,6 +600,26 @@  def __init__(self, path_suffix=''):
                          sock_dir=sock_dir)
         self._num_drives = 0
 
+    def subprocess_check_valgrind(self, valgrind) -> None:
+
+        if not valgrind:
+            return
+
+        valgrind_filename =  test_dir + "/" + str(self._popen.pid) + ".valgrind"
+
+        if self.exitcode() == 99:
+            with open(valgrind_filename) as f:
+                content = f.readlines()
+            for line in content:
+                print(line, end ="")
+            print("")
+        else:
+            os.remove(valgrind_filename)
+
+    def _post_shutdown(self) -> None:
+        super()._post_shutdown()
+        self.subprocess_check_valgrind(qemu_valgrind)
+
     def add_object(self, opts):
         self._args.append('-object')
         self._args.append(opts)