[PATCH v3 00/15] qemu_iotests: improve debugging options

[PATCH v3 00/15] qemu_iotests: improve debugging options

[Emanuele Giuseppe Esposito]

This series adds the option to attach gdbserver and valgrind
to the QEMU binary running in qemu_iotests.
It also allows to redirect QEMU binaries output of the python tests
to the stdout, instead of a log file.

Patches 1-6 introduce the -gdb option to both python and bash tests, 
7-10 extend the already existing -valgrind flag to work also on 
python tests, and patch 11 introduces -p to enable logging to stdout.

In particular, patches 1,2,4,8 focus on extending the QMP socket timers
when using gdb/valgrind, otherwise the python tests will fail due to
delays in the QMP responses.

This series is tested on the previous serie
"qemu-iotests: quality of life improvements"
but independent from it, so it can be applied separately.

Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
---
v3:
- Introduce the class field _qmp_timer instead of a function parameter
in the _post_launch() function [John]
- style and cleanup fixes in iotests.py [Paolo]


Emanuele Giuseppe Esposito (15):
  python: qemu: add timer parameter for qmp.accept socket
  python: qemu: pass the wrapper field from QEMUQtestmachine to
    QEMUMachine
  docs/devel/testing: add debug section to the QEMU iotests chapter
  qemu-iotests: add option to attach gdbserver
  qemu-iotests: delay QMP socket timers
  qemu_iotests: insert gdbserver command line as wrapper for qemu binary
  qemu-iotests: add gdbserver option to script tests too
  docs/devel/testing: add -gdb option to the debugging section of QEMU
    iotests
  qemu_iotests: extend the check script to support valgrind for python
    tests
  qemu_iotests: extent QMP socket timeout when using valgrind
  qemu_iotests: allow valgrind to read/delete the generated log file
  qemu_iotests: insert valgrind command line as wrapper for qemu binary
  docs/devel/testing: add -valgrind option to the debug section of QEMU
    iotests
  qemu_iotests: add option to show qemu binary logs on stdout
  docs/devel/testing: add -p option to the debug section of QEMU iotests

 docs/devel/testing.rst        | 26 ++++++++++++++++
 python/qemu/machine.py        |  6 +++-
 python/qemu/qtest.py          |  4 ++-
 tests/qemu-iotests/check      | 14 ++++++---
 tests/qemu-iotests/common.rc  |  8 ++++-
 tests/qemu-iotests/iotests.py | 58 +++++++++++++++++++++++++++++++++--
 tests/qemu-iotests/testenv.py | 21 +++++++++++--
 7 files changed, 125 insertions(+), 12 deletions(-)

-- 
2.30.2

[PATCH v3 01/15] python: qemu: add timer parameter for qmp.accept socket

[Emanuele Giuseppe Esposito]

Add a new _qmp_timer field to the QEMUMachine class.
The default timer is 15 sec, as per the default in the
qmp accept() function.

Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
---
 python/qemu/machine.py | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/python/qemu/machine.py b/python/qemu/machine.py
index 6e44bda337..12752142c9 100644
--- a/python/qemu/machine.py
+++ b/python/qemu/machine.py
@@ -110,6 +110,7 @@ def __init__(self,
         self._binary = binary
         self._args = list(args)
         self._wrapper = wrapper
+        self._qmp_timer = 15.0
 
         self._name = name or "qemu-%d" % os.getpid()
         self._test_dir = test_dir
@@ -323,7 +324,7 @@ def _pre_launch(self) -> None:
 
     def _post_launch(self) -> None:
         if self._qmp_connection:
-            self._qmp.accept()
+            self._qmp.accept(self._qmp_timer)
 
     def _post_shutdown(self) -> None:
         """
-- 
2.30.2

Re: [PATCH v3 01/15] python: qemu: add timer parameter for qmp.accept socket

[Max Reitz]

On 14.04.21 19:03, Emanuele Giuseppe Esposito wrote:
> Add a new _qmp_timer field to the QEMUMachine class.
> The default timer is 15 sec, as per the default in the
> qmp accept() function.
> 
> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> ---
>   python/qemu/machine.py | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)

Reviewed-by: Max Reitz <mreitz@redhat.com>

(I would have preferred for the commit message to tell me why we want 
this (I suspect some later patch is going to use it), but oh well.)

Re: [PATCH v3 01/15] python: qemu: add timer parameter for qmp.accept socket

[John Snow]

On 4/14/21 1:03 PM, Emanuele Giuseppe Esposito wrote:
> Add a new _qmp_timer field to the QEMUMachine class.
> The default timer is 15 sec, as per the default in the
> qmp accept() function.

Fine enough for now.

What's the exact need for this change, exactly? Is it just to prevent 
any unbounded blocking waits? I assume this came up in development or 
you'd not have added it.

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

> 
> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> ---
>   python/qemu/machine.py | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/python/qemu/machine.py b/python/qemu/machine.py
> index 6e44bda337..12752142c9 100644
> --- a/python/qemu/machine.py
> +++ b/python/qemu/machine.py
> @@ -110,6 +110,7 @@ def __init__(self,
>           self._binary = binary
>           self._args = list(args)
>           self._wrapper = wrapper
> +        self._qmp_timer = 15.0
>   
>           self._name = name or "qemu-%d" % os.getpid()
>           self._test_dir = test_dir
> @@ -323,7 +324,7 @@ def _pre_launch(self) -> None:
>   
>       def _post_launch(self) -> None:
>           if self._qmp_connection:
> -            self._qmp.accept()
> +            self._qmp.accept(self._qmp_timer)
>   
>       def _post_shutdown(self) -> None:
>           """
> 

Re: [PATCH v3 01/15] python: qemu: add timer parameter for qmp.accept socket

[Emanuele Giuseppe Esposito]

On 13/05/2021 19:54, John Snow wrote:
> On 4/14/21 1:03 PM, Emanuele Giuseppe Esposito wrote:
>> Add a new _qmp_timer field to the QEMUMachine class.
>> The default timer is 15 sec, as per the default in the
>> qmp accept() function.
> 
> Fine enough for now.
> 
> What's the exact need for this change, exactly? Is it just to prevent 
> any unbounded blocking waits? I assume this came up in development or 
> you'd not have added it.

The default was already 15 sec, but since we now want to make the wait 
unbounded if we use gdbserver or valgrind and we do it by always passing 
the _qmp_timer to the .accept function, we need to set a default value.
Yes, it came up while testing with gdb.

> 
> Reviewed-by: John Snow <jsnow@redhat.com>
> 
>>
>> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
>> ---
>>   python/qemu/machine.py | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/python/qemu/machine.py b/python/qemu/machine.py
>> index 6e44bda337..12752142c9 100644
>> --- a/python/qemu/machine.py
>> +++ b/python/qemu/machine.py
>> @@ -110,6 +110,7 @@ def __init__(self,
>>           self._binary = binary
>>           self._args = list(args)
>>           self._wrapper = wrapper
>> +        self._qmp_timer = 15.0
>>           self._name = name or "qemu-%d" % os.getpid()
>>           self._test_dir = test_dir
>> @@ -323,7 +324,7 @@ def _pre_launch(self) -> None:
>>       def _post_launch(self) -> None:
>>           if self._qmp_connection:
>> -            self._qmp.accept()
>> +            self._qmp.accept(self._qmp_timer)
>>       def _post_shutdown(self) -> None:
>>           """
>>
> 

[PATCH v3 02/15] python: qemu: pass the wrapper field from QEMUQtestmachine to QEMUMachine

[Emanuele Giuseppe Esposito]

Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
---
 python/qemu/qtest.py | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/python/qemu/qtest.py b/python/qemu/qtest.py
index 39a0cf62fe..c18eae96c6 100644
--- a/python/qemu/qtest.py
+++ b/python/qemu/qtest.py
@@ -111,6 +111,7 @@ class QEMUQtestMachine(QEMUMachine):
     def __init__(self,
                  binary: str,
                  args: Sequence[str] = (),
+                 wrapper: Sequence[str] = (),
                  name: Optional[str] = None,
                  test_dir: str = "/var/tmp",
                  socket_scm_helper: Optional[str] = None,
@@ -119,7 +120,8 @@ def __init__(self,
             name = "qemu-%d" % os.getpid()
         if sock_dir is None:
             sock_dir = test_dir
-        super().__init__(binary, args, name=name, test_dir=test_dir,
+        super().__init__(binary, args, wrapper=wrapper, name=name,
+                         test_dir=test_dir,
                          socket_scm_helper=socket_scm_helper,
                          sock_dir=sock_dir)
         self._qtest: Optional[QEMUQtestProtocol] = None
-- 
2.30.2

Re: [PATCH v3 02/15] python: qemu: pass the wrapper field from QEMUQtestmachine to QEMUMachine

[Max Reitz]

On 14.04.21 19:03, Emanuele Giuseppe Esposito wrote:
> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> ---
>   python/qemu/qtest.py | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/python/qemu/qtest.py b/python/qemu/qtest.py
> index 39a0cf62fe..c18eae96c6 100644
> --- a/python/qemu/qtest.py
> +++ b/python/qemu/qtest.py
> @@ -111,6 +111,7 @@ class QEMUQtestMachine(QEMUMachine):
>       def __init__(self,
>                    binary: str,
>                    args: Sequence[str] = (),
> +                 wrapper: Sequence[str] = (),
>                    name: Optional[str] = None,
>                    test_dir: str = "/var/tmp",
>                    socket_scm_helper: Optional[str] = None,
> @@ -119,7 +120,8 @@ def __init__(self,
>               name = "qemu-%d" % os.getpid()
>           if sock_dir is None:
>               sock_dir = test_dir
> -        super().__init__(binary, args, name=name, test_dir=test_dir,
> +        super().__init__(binary, args, wrapper=wrapper, name=name,
> +                         test_dir=test_dir,
>                            socket_scm_helper=socket_scm_helper,
>                            sock_dir=sock_dir)
>           self._qtest: Optional[QEMUQtestProtocol] = None

Reviewed-by: Max Reitz <mreitz@redhat.com>

Re: [PATCH v3 02/15] python: qemu: pass the wrapper field from QEMUQtestmachine to QEMUMachine

[John Snow]

On 4/14/21 1:03 PM, Emanuele Giuseppe Esposito wrote:
> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>

Assuming it doesn't make the linter explode, which I didn't run:

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

> ---
>   python/qemu/qtest.py | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/python/qemu/qtest.py b/python/qemu/qtest.py
> index 39a0cf62fe..c18eae96c6 100644
> --- a/python/qemu/qtest.py
> +++ b/python/qemu/qtest.py
> @@ -111,6 +111,7 @@ class QEMUQtestMachine(QEMUMachine):
>       def __init__(self,
>                    binary: str,
>                    args: Sequence[str] = (),
> +                 wrapper: Sequence[str] = (),
>                    name: Optional[str] = None,
>                    test_dir: str = "/var/tmp",
>                    socket_scm_helper: Optional[str] = None,
> @@ -119,7 +120,8 @@ def __init__(self,
>               name = "qemu-%d" % os.getpid()
>           if sock_dir is None:
>               sock_dir = test_dir
> -        super().__init__(binary, args, name=name, test_dir=test_dir,
> +        super().__init__(binary, args, wrapper=wrapper, name=name,
> +                         test_dir=test_dir,
>                            socket_scm_helper=socket_scm_helper,
>                            sock_dir=sock_dir)
>           self._qtest: Optional[QEMUQtestProtocol] = None
> 

[PATCH v3 03/15] docs/devel/testing: add debug section to the QEMU iotests chapter

[Emanuele Giuseppe Esposito]

Introduce the "Debugging a test case" section, in preparation
to the additional flags that will be added in the next patches.

Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
---
 docs/devel/testing.rst | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/docs/devel/testing.rst b/docs/devel/testing.rst
index 1434a50cc4..b7e2370e7e 100644
--- a/docs/devel/testing.rst
+++ b/docs/devel/testing.rst
@@ -224,6 +224,14 @@ another application on the host may have locked the file, possibly leading to a
 test failure.  If using such devices are explicitly desired, consider adding
 ``locking=off`` option to disable image locking.
 
+Debugging a test case
+-----------------------
+QEMU iotests offers some options to debug a failing test, that can be
+given as options to the ``check`` script:
+
+* ``-d`` (debug) just increases the logging verbosity, showing
+  for example the QMP commands and answers.
+
 Test case groups
 ----------------
 
-- 
2.30.2

Re: [PATCH v3 03/15] docs/devel/testing: add debug section to the QEMU iotests chapter

[Max Reitz]

On 14.04.21 19:03, Emanuele Giuseppe Esposito wrote:
> Introduce the "Debugging a test case" section, in preparation
> to the additional flags that will be added in the next patches.
> 
> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> ---
>   docs/devel/testing.rst | 8 ++++++++
>   1 file changed, 8 insertions(+)
> 
> diff --git a/docs/devel/testing.rst b/docs/devel/testing.rst
> index 1434a50cc4..b7e2370e7e 100644
> --- a/docs/devel/testing.rst
> +++ b/docs/devel/testing.rst
> @@ -224,6 +224,14 @@ another application on the host may have locked the file, possibly leading to a
>   test failure.  If using such devices are explicitly desired, consider adding
>   ``locking=off`` option to disable image locking.
>   
> +Debugging a test case
> +-----------------------
> +QEMU iotests offers some options to debug a failing test, that can be

-,

Max

> +given as options to the ``check`` script:
> +
> +* ``-d`` (debug) just increases the logging verbosity, showing
> +  for example the QMP commands and answers.
> +
>   Test case groups
>   ----------------
>   
> 

Re: [PATCH v3 03/15] docs/devel/testing: add debug section to the QEMU iotests chapter

[Paolo Bonzini]

On 30/04/21 13:23, Max Reitz wrote:
>>
>> +-----------------------
>> +QEMU iotests offers some options to debug a failing test, that can be
>> +given as options to the ``check`` script: 
> 
> -,

Even better: "The following options to the ``check`` script can be 
useful when debugging a failing test:".

Paolo

[PATCH v3 04/15] qemu-iotests: add option to attach gdbserver

[Emanuele Giuseppe Esposito]

Add -gdb flag and GDB_QEMU environmental variable
to python tests to attach a gdbserver to each qemu instance.

if -gdb is not provided but $GDB_QEMU is set, ignore the
environmental variable.

Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
---
 tests/qemu-iotests/check      |  6 +++++-
 tests/qemu-iotests/iotests.py |  4 ++++
 tests/qemu-iotests/testenv.py | 15 ++++++++++++---
 3 files changed, 21 insertions(+), 4 deletions(-)

diff --git a/tests/qemu-iotests/check b/tests/qemu-iotests/check
index d1c87ceaf1..6186495eee 100755
--- a/tests/qemu-iotests/check
+++ b/tests/qemu-iotests/check
@@ -33,6 +33,9 @@ def make_argparser() -> argparse.ArgumentParser:
                    help='pretty print output for make check')
 
     p.add_argument('-d', dest='debug', action='store_true', help='debug')
+    p.add_argument('-gdb', action='store_true',
+                   help="start gdbserver with $GDB_QEMU options. \
+                         Default is localhost:12345")
     p.add_argument('-misalign', action='store_true',
                    help='misalign memory allocations')
     p.add_argument('--color', choices=['on', 'off', 'auto'],
@@ -112,7 +115,8 @@ if __name__ == '__main__':
     env = TestEnv(imgfmt=args.imgfmt, imgproto=args.imgproto,
                   aiomode=args.aiomode, cachemode=args.cachemode,
                   imgopts=args.imgopts, misalign=args.misalign,
-                  debug=args.debug, valgrind=args.valgrind)
+                  debug=args.debug, valgrind=args.valgrind,
+                  gdb=args.gdb)
 
     testfinder = TestFinder(test_dir=env.source_iotests)
 
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 90d0b62523..05d0dc0751 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -75,6 +75,10 @@
 qemu_prog = os.environ.get('QEMU_PROG', 'qemu')
 qemu_opts = os.environ.get('QEMU_OPTIONS', '').strip().split(' ')
 
+qemu_gdb = []
+if os.environ.get('GDB_QEMU'):
+    qemu_gdb = ['gdbserver'] + os.environ.get('GDB_QEMU').strip().split(' ')
+
 imgfmt = os.environ.get('IMGFMT', 'raw')
 imgproto = os.environ.get('IMGPROTO', 'file')
 output_dir = os.environ.get('OUTPUT_DIR', '.')
diff --git a/tests/qemu-iotests/testenv.py b/tests/qemu-iotests/testenv.py
index 1fbec854c1..e131ff42cb 100644
--- a/tests/qemu-iotests/testenv.py
+++ b/tests/qemu-iotests/testenv.py
@@ -72,7 +72,8 @@ class TestEnv(ContextManager['TestEnv']):
                      'QEMU_NBD_OPTIONS', 'IMGOPTS', 'IMGFMT', 'IMGPROTO',
                      'AIOMODE', 'CACHEMODE', 'VALGRIND_QEMU',
                      'CACHEMODE_IS_DEFAULT', 'IMGFMT_GENERIC', 'IMGOPTSSYNTAX',
-                     'IMGKEYSECRET', 'QEMU_DEFAULT_MACHINE', 'MALLOC_PERTURB_']
+                     'IMGKEYSECRET', 'QEMU_DEFAULT_MACHINE', 'MALLOC_PERTURB_',
+                     'GDB_QEMU']
 
     def get_env(self) -> Dict[str, str]:
         env = {}
@@ -163,7 +164,8 @@ def __init__(self, imgfmt: str, imgproto: str, aiomode: str,
                  imgopts: Optional[str] = None,
                  misalign: bool = False,
                  debug: bool = False,
-                 valgrind: bool = False) -> None:
+                 valgrind: bool = False,
+                 gdb: bool = False) -> None:
         self.imgfmt = imgfmt
         self.imgproto = imgproto
         self.aiomode = aiomode
@@ -171,6 +173,11 @@ def __init__(self, imgfmt: str, imgproto: str, aiomode: str,
         self.misalign = misalign
         self.debug = debug
 
+        if gdb:
+            self.gdb_qemu = os.environ.get('GDB_QEMU', 'localhost:12345')
+        elif 'GDB_QEMU' in os.environ:
+            del os.environ['GDB_QEMU']
+
         if valgrind:
             self.valgrind_qemu = 'y'
 
@@ -268,7 +275,9 @@ def print_env(self) -> None:
 PLATFORM      -- {platform}
 TEST_DIR      -- {TEST_DIR}
 SOCK_DIR      -- {SOCK_DIR}
-SOCKET_SCM_HELPER -- {SOCKET_SCM_HELPER}"""
+SOCKET_SCM_HELPER -- {SOCKET_SCM_HELPER}
+GDB_QEMU      -- "{GDB_QEMU}"
+"""
 
         args = collections.defaultdict(str, self.get_env())
 
-- 
2.30.2

Re: [PATCH v3 04/15] qemu-iotests: add option to attach gdbserver

[Max Reitz]

On 14.04.21 19:03, Emanuele Giuseppe Esposito wrote:
> Add -gdb flag and GDB_QEMU environmental variable
> to python tests to attach a gdbserver to each qemu instance.

Well, this patch doesn’t do this, but OK.

Out of interest: Why gdbserver and not “just” gdb?  On Fedora, those are 
separate packages, so I don’t have gdbserver installed, that’s why I’m 
asking.

(I’ve also never used gdbserver before.  From what I can tell, it’s 
basically just a limited version of gdb so it only serves as a server.)

> if -gdb is not provided but $GDB_QEMU is set, ignore the
> environmental variable.

s/environmental/environment/

> 
> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> ---
>   tests/qemu-iotests/check      |  6 +++++-
>   tests/qemu-iotests/iotests.py |  4 ++++
>   tests/qemu-iotests/testenv.py | 15 ++++++++++++---
>   3 files changed, 21 insertions(+), 4 deletions(-)
> 
> diff --git a/tests/qemu-iotests/check b/tests/qemu-iotests/check
> index d1c87ceaf1..6186495eee 100755
> --- a/tests/qemu-iotests/check
> +++ b/tests/qemu-iotests/check
> @@ -33,6 +33,9 @@ def make_argparser() -> argparse.ArgumentParser:
>                      help='pretty print output for make check')
>   
>       p.add_argument('-d', dest='debug', action='store_true', help='debug')
> +    p.add_argument('-gdb', action='store_true',
> +                   help="start gdbserver with $GDB_QEMU options. \
> +                         Default is localhost:12345")

That makes it sound like this were the default for the `-gdb` option. 
Since `-gdb` is just a switch, it doesn’t have a default (apart from 
being off by default).

So I’d rephrase this at least to “The default options are 
'localhost:12345'”.  Or maybe “start gdbserver with $GDB_QEMU options 
('localhost:12345' if $GDB_QEMU is empty)”.

Also, $GDB_QEMU as a name is a bit strange, because it does not specify 
which gdb to use; it just gives the options to use for gdb. 
$GDB_QEMU_OPTIONS would be more in line with the naming of the rest of 
the environment variables (or just $GDB_OPTIONS).

Max

>       p.add_argument('-misalign', action='store_true',
>                      help='misalign memory allocations')
>       p.add_argument('--color', choices=['on', 'off', 'auto'],

Re: [PATCH v3 04/15] qemu-iotests: add option to attach gdbserver

[Emanuele Giuseppe Esposito]

On 30/04/2021 13:38, Max Reitz wrote:
> On 14.04.21 19:03, Emanuele Giuseppe Esposito wrote:
>> Add -gdb flag and GDB_QEMU environmental variable
>> to python tests to attach a gdbserver to each qemu instance.
> 
> Well, this patch doesn’t do this, but OK.

Maybe "define" rather than "add"? In the sense of defining the "-gdb" 
option, which is what it actually does.

> 
> Out of interest: Why gdbserver and not “just” gdb?  On Fedora, those are 
> separate packages, so I don’t have gdbserver installed, that’s why I’m 
> asking.

As far as I have tried, using only gdb with ./check is very hard to use, 
because the stdout is filtered out by the script.
So invoking gdb attaches it to QEMU, but it is not possible to start 
execution (run command) or interact with it, because of the python 
script filtering. This leaves the test hanging.

gdbserver is just something that a gdb client can attach to (for 
example, in another console or even in another host) for example with 
the command
# gdb -iex "target remote localhost:12345" . This provides a nice and 
separate gdb monitor to the client.

Emanuele
> 
> (I’ve also never used gdbserver before.  From what I can tell, it’s 
> basically just a limited version of gdb so it only serves as a server.)
> 
>> if -gdb is not provided but $GDB_QEMU is set, ignore the
>> environmental variable.
> 
> s/environmental/environment/
> 
>>
>> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
>> ---
>>   tests/qemu-iotests/check      |  6 +++++-
>>   tests/qemu-iotests/iotests.py |  4 ++++
>>   tests/qemu-iotests/testenv.py | 15 ++++++++++++---
>>   3 files changed, 21 insertions(+), 4 deletions(-)
>>
>> diff --git a/tests/qemu-iotests/check b/tests/qemu-iotests/check
>> index d1c87ceaf1..6186495eee 100755
>> --- a/tests/qemu-iotests/check
>> +++ b/tests/qemu-iotests/check
>> @@ -33,6 +33,9 @@ def make_argparser() -> argparse.ArgumentParser:
>>                      help='pretty print output for make check')
>>       p.add_argument('-d', dest='debug', action='store_true', 
>> help='debug')
>> +    p.add_argument('-gdb', action='store_true',
>> +                   help="start gdbserver with $GDB_QEMU options. \
>> +                         Default is localhost:12345")
> 
> That makes it sound like this were the default for the `-gdb` option. 
> Since `-gdb` is just a switch, it doesn’t have a default (apart from 
> being off by default).
> 
> So I’d rephrase this at least to “The default options are 
> 'localhost:12345'”.  Or maybe “start gdbserver with $GDB_QEMU options 
> ('localhost:12345' if $GDB_QEMU is empty)”.
> 
> Also, $GDB_QEMU as a name is a bit strange, because it does not specify 
> which gdb to use; it just gives the options to use for gdb. 
> $GDB_QEMU_OPTIONS would be more in line with the naming of the rest of 
> the environment variables (or just $GDB_OPTIONS).
> 
> Max
> 
>>       p.add_argument('-misalign', action='store_true',
>>                      help='misalign memory allocations')
>>       p.add_argument('--color', choices=['on', 'off', 'auto'],
> 

Re: [PATCH v3 04/15] qemu-iotests: add option to attach gdbserver

[Max Reitz]

On 30.04.21 23:03, Emanuele Giuseppe Esposito wrote:
> 
> 
> On 30/04/2021 13:38, Max Reitz wrote:
>> On 14.04.21 19:03, Emanuele Giuseppe Esposito wrote:
>>> Add -gdb flag and GDB_QEMU environmental variable
>>> to python tests to attach a gdbserver to each qemu instance.
>>
>> Well, this patch doesn’t do this, but OK.
> 
> Maybe "define" rather than "add"? In the sense of defining the "-gdb" 
> option, which is what it actually does.

That’s possible, but I think better would be to contrast it with what 
this patch doesn’t do, but what one could think when reading this 
description.

I.e. to say “Add/define -gdb flag [...] to each qemu instance.  This 
patch only adds and parses this flag, it does not yet add the 
implementation for it.”

>> Out of interest: Why gdbserver and not “just” gdb?  On Fedora, those 
>> are separate packages, so I don’t have gdbserver installed, that’s why 
>> I’m asking.
> 
> As far as I have tried, using only gdb with ./check is very hard to use, 
> because the stdout is filtered out by the script.
> So invoking gdb attaches it to QEMU, but it is not possible to start 
> execution (run command) or interact with it, because of the python 
> script filtering. This leaves the test hanging.
> 
> gdbserver is just something that a gdb client can attach to (for 
> example, in another console or even in another host) for example with 
> the command
> # gdb -iex "target remote localhost:12345" . This provides a nice and 
> separate gdb monitor to the client.

All right.  I thought gdb could be used as a server, too, but...  Looks 
like it can’t.  (Like, I thought, you could do something like
“gdb -ex 'listen localhost:12345' $cmd”.  But seems like there is no 
server built into gdb proper.)

Max

Re: [PATCH v3 04/15] qemu-iotests: add option to attach gdbserver

[Max Reitz]

On 14.04.21 19:03, Emanuele Giuseppe Esposito wrote:
> Add -gdb flag and GDB_QEMU environmental variable
> to python tests to attach a gdbserver to each qemu instance.
> 
> if -gdb is not provided but $GDB_QEMU is set, ignore the
> environmental variable.
> 
> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> ---
>   tests/qemu-iotests/check      |  6 +++++-
>   tests/qemu-iotests/iotests.py |  4 ++++
>   tests/qemu-iotests/testenv.py | 15 ++++++++++++---
>   3 files changed, 21 insertions(+), 4 deletions(-)
> 
> diff --git a/tests/qemu-iotests/check b/tests/qemu-iotests/check
> index d1c87ceaf1..6186495eee 100755
> --- a/tests/qemu-iotests/check
> +++ b/tests/qemu-iotests/check
> @@ -33,6 +33,9 @@ def make_argparser() -> argparse.ArgumentParser:
>                      help='pretty print output for make check')
>   
>       p.add_argument('-d', dest='debug', action='store_true', help='debug')
> +    p.add_argument('-gdb', action='store_true',
> +                   help="start gdbserver with $GDB_QEMU options. \
> +                         Default is localhost:12345")
>       p.add_argument('-misalign', action='store_true',
>                      help='misalign memory allocations')
>       p.add_argument('--color', choices=['on', 'off', 'auto'],
> @@ -112,7 +115,8 @@ if __name__ == '__main__':
>       env = TestEnv(imgfmt=args.imgfmt, imgproto=args.imgproto,
>                     aiomode=args.aiomode, cachemode=args.cachemode,
>                     imgopts=args.imgopts, misalign=args.misalign,
> -                  debug=args.debug, valgrind=args.valgrind)
> +                  debug=args.debug, valgrind=args.valgrind,
> +                  gdb=args.gdb)
>   
>       testfinder = TestFinder(test_dir=env.source_iotests)
>   
> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
> index 90d0b62523..05d0dc0751 100644
> --- a/tests/qemu-iotests/iotests.py
> +++ b/tests/qemu-iotests/iotests.py
> @@ -75,6 +75,10 @@
>   qemu_prog = os.environ.get('QEMU_PROG', 'qemu')
>   qemu_opts = os.environ.get('QEMU_OPTIONS', '').strip().split(' ')
>   
> +qemu_gdb = []
> +if os.environ.get('GDB_QEMU'):
> +    qemu_gdb = ['gdbserver'] + os.environ.get('GDB_QEMU').strip().split(' ')

os.environ.get('GDB_QEMU') returns an Option[str], so mypy complains 
about the .strip() (thus failing iotest 297).

(That can be fixed for example by either using os.environ['GDB_QEMU'] 
here, like most other places here do, or by something like:

gdb_qemu_env = os.environ.get('GDB_QEMU')
if gdb_qemu_env:
     qemu_gdb = ['gdbserver'] + gdb_qemu_env.strip().split(' ')
)

Max

> +
>   imgfmt = os.environ.get('IMGFMT', 'raw')
>   imgproto = os.environ.get('IMGPROTO', 'file')
>   output_dir = os.environ.get('OUTPUT_DIR', '.')
> diff --git a/tests/qemu-iotests/testenv.py b/tests/qemu-iotests/testenv.py
> index 1fbec854c1..e131ff42cb 100644
> --- a/tests/qemu-iotests/testenv.py
> +++ b/tests/qemu-iotests/testenv.py
> @@ -72,7 +72,8 @@ class TestEnv(ContextManager['TestEnv']):
>                        'QEMU_NBD_OPTIONS', 'IMGOPTS', 'IMGFMT', 'IMGPROTO',
>                        'AIOMODE', 'CACHEMODE', 'VALGRIND_QEMU',
>                        'CACHEMODE_IS_DEFAULT', 'IMGFMT_GENERIC', 'IMGOPTSSYNTAX',
> -                     'IMGKEYSECRET', 'QEMU_DEFAULT_MACHINE', 'MALLOC_PERTURB_']
> +                     'IMGKEYSECRET', 'QEMU_DEFAULT_MACHINE', 'MALLOC_PERTURB_',
> +                     'GDB_QEMU']
>   
>       def get_env(self) -> Dict[str, str]:
>           env = {}
> @@ -163,7 +164,8 @@ def __init__(self, imgfmt: str, imgproto: str, aiomode: str,
>                    imgopts: Optional[str] = None,
>                    misalign: bool = False,
>                    debug: bool = False,
> -                 valgrind: bool = False) -> None:
> +                 valgrind: bool = False,
> +                 gdb: bool = False) -> None:
>           self.imgfmt = imgfmt
>           self.imgproto = imgproto
>           self.aiomode = aiomode
> @@ -171,6 +173,11 @@ def __init__(self, imgfmt: str, imgproto: str, aiomode: str,
>           self.misalign = misalign
>           self.debug = debug
>   
> +        if gdb:
> +            self.gdb_qemu = os.environ.get('GDB_QEMU', 'localhost:12345')
> +        elif 'GDB_QEMU' in os.environ:
> +            del os.environ['GDB_QEMU']
> +
>           if valgrind:
>               self.valgrind_qemu = 'y'
>   
> @@ -268,7 +275,9 @@ def print_env(self) -> None:
>   PLATFORM      -- {platform}
>   TEST_DIR      -- {TEST_DIR}
>   SOCK_DIR      -- {SOCK_DIR}
> -SOCKET_SCM_HELPER -- {SOCKET_SCM_HELPER}"""
> +SOCKET_SCM_HELPER -- {SOCKET_SCM_HELPER}
> +GDB_QEMU      -- "{GDB_QEMU}"
> +"""
>   
>           args = collections.defaultdict(str, self.get_env())
>   
> 

[PATCH v3 05/15] qemu-iotests: delay QMP socket timers

[Emanuele Giuseppe Esposito]

Attaching a gdbserver implies that the qmp socket
should wait indefinitely for an answer from QEMU.

Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
---
 python/qemu/machine.py        |  3 +++
 tests/qemu-iotests/iotests.py | 10 +++++++++-
 2 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/python/qemu/machine.py b/python/qemu/machine.py
index 12752142c9..d6142271c2 100644
--- a/python/qemu/machine.py
+++ b/python/qemu/machine.py
@@ -409,6 +409,9 @@ def _launch(self) -> None:
                                        stderr=subprocess.STDOUT,
                                        shell=False,
                                        close_fds=False)
+
+        if 'gdbserver' in self._wrapper:
+            self._qmp_timer = None
         self._post_launch()
 
     def _early_cleanup(self) -> None:
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 05d0dc0751..380527245e 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -478,7 +478,10 @@ def log(msg: Msg,
 
 class Timeout:
     def __init__(self, seconds, errmsg="Timeout"):
-        self.seconds = seconds
+        if qemu_gdb:
+            self.seconds = 3000
+        else:
+            self.seconds = seconds
         self.errmsg = errmsg
     def __enter__(self):
         signal.signal(signal.SIGALRM, self.timeout)
@@ -684,6 +687,11 @@ def qmp_to_opts(self, obj):
             output_list += [key + '=' + obj[key]]
         return ','.join(output_list)
 
+    def get_qmp_events(self, wait: bool = False) -> List[QMPMessage]:
+        if qemu_gdb:
+            wait = 0.0
+        return super().get_qmp_events(wait=wait)
+
     def get_qmp_events_filtered(self, wait=60.0):
         result = []
         for ev in self.get_qmp_events(wait=wait):
-- 
2.30.2

Re: [PATCH v3 05/15] qemu-iotests: delay QMP socket timers

[Max Reitz]

On 14.04.21 19:03, Emanuele Giuseppe Esposito wrote:
> Attaching a gdbserver implies that the qmp socket
> should wait indefinitely for an answer from QEMU.
> 
> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> ---
>   python/qemu/machine.py        |  3 +++
>   tests/qemu-iotests/iotests.py | 10 +++++++++-
>   2 files changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/python/qemu/machine.py b/python/qemu/machine.py
> index 12752142c9..d6142271c2 100644
> --- a/python/qemu/machine.py
> +++ b/python/qemu/machine.py
> @@ -409,6 +409,9 @@ def _launch(self) -> None:
>                                          stderr=subprocess.STDOUT,
>                                          shell=False,
>                                          close_fds=False)
> +
> +        if 'gdbserver' in self._wrapper:
> +            self._qmp_timer = None

Why doesn’t __init__() evaluate this?  This here doesn’t feel like the 
right place for it.  If we want to evaluate it here, self._qmp_timer 
shouldn’t exist, and instead the timeout should be a _post_launch() 
parameter.  (Which I would have nothing against, by the way.)

Also, mypy complains that this variable is a float, so iotest 297 (which 
runs mypy) fails.

>           self._post_launch()
>   
>       def _early_cleanup(self) -> None:
> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
> index 05d0dc0751..380527245e 100644
> --- a/tests/qemu-iotests/iotests.py
> +++ b/tests/qemu-iotests/iotests.py
> @@ -478,7 +478,10 @@ def log(msg: Msg,
>   
>   class Timeout:
>       def __init__(self, seconds, errmsg="Timeout"):
> -        self.seconds = seconds
> +        if qemu_gdb:
> +            self.seconds = 3000
> +        else:
> +            self.seconds = seconds

We might as well completely disable the timeout then, that would be more 
honest, I think.  (I.e. to make __enter__ and __exit__ no-ops.)

>           self.errmsg = errmsg
>       def __enter__(self):
>           signal.signal(signal.SIGALRM, self.timeout)
> @@ -684,6 +687,11 @@ def qmp_to_opts(self, obj):
>               output_list += [key + '=' + obj[key]]
>           return ','.join(output_list)
>   
> +    def get_qmp_events(self, wait: bool = False) -> List[QMPMessage]:
> +        if qemu_gdb:
> +            wait = 0.0

First, this is a bool.  I can see that qmp.py expects a
Union[bool, float], but machine.py expects just a bool.  (Also, mypy 
complains that this specific `wait` here is a `bool`.  You can see that 
by running iotest 297.)

Second, I don’t understand this.  If the caller wants to block waiting 
on an event, then that should have nothing to do with whether we have 
gdb running or not.  As far as I understand, setting wait to 0.0 is the 
same as wait = False, i.e. we don’t block and just return None 
immediately if there is no pending event.

Max

> +        return super().get_qmp_events(wait=wait)
> +
>       def get_qmp_events_filtered(self, wait=60.0):
>           result = []
>           for ev in self.get_qmp_events(wait=wait):
> 

Re: [PATCH v3 05/15] qemu-iotests: delay QMP socket timers

[Emanuele Giuseppe Esposito]

On 30/04/2021 13:59, Max Reitz wrote:
> On 14.04.21 19:03, Emanuele Giuseppe Esposito wrote:
>> Attaching a gdbserver implies that the qmp socket
>> should wait indefinitely for an answer from QEMU.
>>
>> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
>> ---
>>   python/qemu/machine.py        |  3 +++
>>   tests/qemu-iotests/iotests.py | 10 +++++++++-
>>   2 files changed, 12 insertions(+), 1 deletion(-)
>>
>> diff --git a/python/qemu/machine.py b/python/qemu/machine.py
>> index 12752142c9..d6142271c2 100644
>> --- a/python/qemu/machine.py
>> +++ b/python/qemu/machine.py
>> @@ -409,6 +409,9 @@ def _launch(self) -> None:
>>                                          stderr=subprocess.STDOUT,
>>                                          shell=False,
>>                                          close_fds=False)
>> +
>> +        if 'gdbserver' in self._wrapper:
>> +            self._qmp_timer = None
> 
> Why doesn’t __init__() evaluate this?  This here doesn’t feel like the 
> right place for it.  If we want to evaluate it here, self._qmp_timer 
> shouldn’t exist, and instead the timeout should be a _post_launch() 
> parameter.  (Which I would have nothing against, by the way.)

Uhm.. I got another comment in a previous version where for the "event" 
callbacks it was better a property than passing around a parameter. 
Which I honestly agree.

What should __init__() do? The check here is to see if the invocation 
has gdb (and a couple of patches ahead also valgrind), to remove the timer.
If I understand what you mean, you want something like
def __init__(self, timer):

But from my understanding, the class hierarchy is:
QEMUMachine: in machine.py
QEMUQtestMachine(QEMUMachine): in qtest.py
VM(qtest.QEMUQtestMachine): in iotests.py
VM() is then invoked in each test.

So this is not easily reachable by check.py, to pass the parameter into 
__init__

> 
> Also, mypy complains that this variable is a float, so iotest 297 (which 
> runs mypy) fails.

This and all mistakes of test 297 (mypy) is my fault: I did not have 
pylint-3 installed thus when testing it skipped the 297 test.

> 
>>           self._post_launch()
>>       def _early_cleanup(self) -> None:
>> diff --git a/tests/qemu-iotests/iotests.py 
>> b/tests/qemu-iotests/iotests.py
>> index 05d0dc0751..380527245e 100644
>> --- a/tests/qemu-iotests/iotests.py
>> +++ b/tests/qemu-iotests/iotests.py
>> @@ -478,7 +478,10 @@ def log(msg: Msg,
>>   class Timeout:
>>       def __init__(self, seconds, errmsg="Timeout"):
>> -        self.seconds = seconds
>> +        if qemu_gdb:
>> +            self.seconds = 3000
>> +        else:
>> +            self.seconds = seconds
> 
> We might as well completely disable the timeout then, that would be more 
> honest, I think.  (I.e. to make __enter__ and __exit__ no-ops.)
> 
>>           self.errmsg = errmsg
>>       def __enter__(self):
>>           signal.signal(signal.SIGALRM, self.timeout)
>> @@ -684,6 +687,11 @@ def qmp_to_opts(self, obj):
>>               output_list += [key + '=' + obj[key]]
>>           return ','.join(output_list)
>> +    def get_qmp_events(self, wait: bool = False) -> List[QMPMessage]:
>> +        if qemu_gdb:
>> +            wait = 0.0
> 
> First, this is a bool.  I can see that qmp.py expects a
> Union[bool, float], but machine.py expects just a bool.  (Also, mypy 
> complains that this specific `wait` here is a `bool`.  You can see that 
> by running iotest 297.)

I think here machine.py should reflect the qmp.py behavior and have an 
Union[bool, float] too.
> 
> Second, I don’t understand this.  If the caller wants to block waiting 
> on an event, then that should have nothing to do with whether we have 
> gdb running or not.  As far as I understand, setting wait to 0.0 is the 
> same as wait = False, i.e. we don’t block and just return None 
> immediately if there is no pending event.

You're right, this might not be needed here. The problem I had was that 
calling gdb and pausing at a breakpoint or something for a while would 
make the QMP socket timeout, thus aborting the whole test. In order to 
avoid that, I need to stop or delay timers.

I can't remember why I added this check here. At some point I am sure 
the test was failing because of socket timeout expiration, but I cannot 
reproduce the problem when commenting out this check above in 
get_qmp_events. The other check in patch 3 should be enough.

Emanuele
> 
> Max
> 
>> +        return super().get_qmp_events(wait=wait)
>> +
>>       def get_qmp_events_filtered(self, wait=60.0):
>>           result = []
>>           for ev in self.get_qmp_events(wait=wait):
>>
> 

Re: [PATCH v3 05/15] qemu-iotests: delay QMP socket timers

[Max Reitz]

On 30.04.21 23:03, Emanuele Giuseppe Esposito wrote:
> 
> 
> On 30/04/2021 13:59, Max Reitz wrote:
>> On 14.04.21 19:03, Emanuele Giuseppe Esposito wrote:
>>> Attaching a gdbserver implies that the qmp socket
>>> should wait indefinitely for an answer from QEMU.
>>>
>>> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
>>> ---
>>>   python/qemu/machine.py        |  3 +++
>>>   tests/qemu-iotests/iotests.py | 10 +++++++++-
>>>   2 files changed, 12 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/python/qemu/machine.py b/python/qemu/machine.py
>>> index 12752142c9..d6142271c2 100644
>>> --- a/python/qemu/machine.py
>>> +++ b/python/qemu/machine.py
>>> @@ -409,6 +409,9 @@ def _launch(self) -> None:
>>>                                          stderr=subprocess.STDOUT,
>>>                                          shell=False,
>>>                                          close_fds=False)
>>> +
>>> +        if 'gdbserver' in self._wrapper:
>>> +            self._qmp_timer = None
>>
>> Why doesn’t __init__() evaluate this?  This here doesn’t feel like the 
>> right place for it.  If we want to evaluate it here, self._qmp_timer 
>> shouldn’t exist, and instead the timeout should be a _post_launch() 
>> parameter.  (Which I would have nothing against, by the way.)
> 
> Uhm.. I got another comment in a previous version where for the "event" 
> callbacks it was better a property than passing around a parameter. 
> Which I honestly agree.

I think that comment was in the sense of providing a default value, 
which can be expressed by having a property that is set in __init__.

I don’t have anything against making this a property, but I also don’t 
have anything against making it a _post_launch() parameter.  I could 
even live with both, i.e. set _qmp_timer to 15 in __init__, then have a 
_post_launch parameter, and pass either self._qmp_timer or None if 
self._wrapper includes 'gdbserver'.

What I do mind is that I don’t understand why the property is modified 
here.  The value of self._qmp_timer is supposed to be 15 by default and 
None if self._wrapper includes 'gdbserver'.  It should thus be changed 
to None the moment self._wrapper is made to include 'gdbserver'. 
Because self._wrapper is set only in __init__, this should happen in 
__init__.

> What should __init__() do? The check here is to see if the invocation 
> has gdb (and a couple of patches ahead also valgrind), to remove the timer.
> If I understand what you mean, you want something like
> def __init__(self, timer):

Oh, no.  We can optionally do that perhaps later, but what I meant is 
just to put this in __init__() (without adding any parameters to it):

self._qmp_timer = 15.0 if 'gdbserver' not in self._wrapper else None

I think self._qmp_timer should always reflect what timeout we are going 
to use when a VM is launched.  So if the conditions influencing the 
timeout change, it should be updated immediately to reflect this.  The 
only condition we have right now is the content of self._wrapper, which 
is only set in __init__, so self._qmp_timer should be set once in 
__init__ and not changed afterwards.

That sounds academic, but imagine what would happen if we had a 
set_qmp_timer() method: The timout could be adjusted, but launch() would 
just ignore it and update the property, even though the conditions 
influencing the timout didn’t change between set_qmp_timer() and launch().

Or if we had a get_qmp_timer(); a caller would read a timeout of 15.0 
before launch(), even though the timeout is going to be None.

Therefore, I think a property should not be updated just before it is 
read, but instead when any condition that’s supposed to influence its 
value changes.


I suggested making it a parameter because updating a property when 
reading it sounds like it should be a parameter instead.  I.e., one 
would say

def __init__():
     self._qmp_timeout_default = 15.0

def post_launch(qmp_timeout):
     self._qmp.accept(qmp_timeout)

def launch(self):
     ...
     qmp_timeout = None if 'gdbserver' in self._wrapper \
                        else self._qmp_timout_default
     self.post_launch(qmp_timeout)


Which is basically the structure your patch has, which gave me the idea.

[...]

>>>           self._post_launch()
>>>       def _early_cleanup(self) -> None:
>>> diff --git a/tests/qemu-iotests/iotests.py 
>>> b/tests/qemu-iotests/iotests.py
>>> index 05d0dc0751..380527245e 100644
>>> --- a/tests/qemu-iotests/iotests.py
>>> +++ b/tests/qemu-iotests/iotests.py

[...]

>>> @@ -684,6 +687,11 @@ def qmp_to_opts(self, obj):
>>>               output_list += [key + '=' + obj[key]]
>>>           return ','.join(output_list)
>>> +    def get_qmp_events(self, wait: bool = False) -> List[QMPMessage]:
>>> +        if qemu_gdb:
>>> +            wait = 0.0

[...]

>>
>> Second, I don’t understand this.  If the caller wants to block waiting 
>> on an event, then that should have nothing to do with whether we have 
>> gdb running or not.  As far as I understand, setting wait to 0.0 is 
>> the same as wait = False, i.e. we don’t block and just return None 
>> immediately if there is no pending event.
> 
> You're right, this might not be needed here. The problem I had was that 
> calling gdb and pausing at a breakpoint or something for a while would 
> make the QMP socket timeout, thus aborting the whole test. In order to 
> avoid that, I need to stop or delay timers.
> 
> I can't remember why I added this check here. At some point I am sure 
> the test was failing because of socket timeout expiration, but I cannot 
> reproduce the problem when commenting out this check above in 
> get_qmp_events. The other check in patch 3 should be enough.

Hm, ok.  I’d guessed that you intended the wait=0.0 or wait=False to 
mean that we get an infinite timeout (i.e., no timeout), but that’s 
exactly why I didn’t get it.  wait=0.0 doesn’t give an infinite timeout, 
but instead basically times out immediately.

Max

Re: [PATCH v3 05/15] qemu-iotests: delay QMP socket timers

[John Snow]

On 5/3/21 11:02 AM, Max Reitz wrote:
> On 30.04.21 23:03, Emanuele Giuseppe Esposito wrote:
>>
>>
>> On 30/04/2021 13:59, Max Reitz wrote:
>>> On 14.04.21 19:03, Emanuele Giuseppe Esposito wrote:
>>>> Attaching a gdbserver implies that the qmp socket
>>>> should wait indefinitely for an answer from QEMU.
>>>>
>>>> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
>>>> ---
>>>>   python/qemu/machine.py        |  3 +++
>>>>   tests/qemu-iotests/iotests.py | 10 +++++++++-
>>>>   2 files changed, 12 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/python/qemu/machine.py b/python/qemu/machine.py
>>>> index 12752142c9..d6142271c2 100644
>>>> --- a/python/qemu/machine.py
>>>> +++ b/python/qemu/machine.py
>>>> @@ -409,6 +409,9 @@ def _launch(self) -> None:
>>>>                                          stderr=subprocess.STDOUT,
>>>>                                          shell=False,
>>>>                                          close_fds=False)
>>>> +
>>>> +        if 'gdbserver' in self._wrapper:
>>>> +            self._qmp_timer = None
>>>
>>> Why doesn’t __init__() evaluate this?  This here doesn’t feel like 
>>> the right place for it.  If we want to evaluate it here, 
>>> self._qmp_timer shouldn’t exist, and instead the timeout should be a 
>>> _post_launch() parameter.  (Which I would have nothing against, by 
>>> the way.)
>>
>> Uhm.. I got another comment in a previous version where for the 
>> "event" callbacks it was better a property than passing around a 
>> parameter. Which I honestly agree.
> 
> I think that comment was in the sense of providing a default value, 
> which can be expressed by having a property that is set in __init__.
> 

My comment was along the lines that "_post_launch()" is behaving as an 
event loop hook and not the sort of thing I want to pass parameters to. 
It's a private method, so the only possibility for someone passing a 
parameter to is another class method anyway.

We have a hierarchy of things that depend on the Machine class and I 
didn't want to start cascading optional parameters into the subclasses.

It was my intent that the information needed to run _post_launch() 
correctly should be known by the state of the object -- which I think 
should be true anyway.

> I don’t have anything against making this a property, but I also don’t 
> have anything against making it a _post_launch() parameter.  I could 
> even live with both, i.e. set _qmp_timer to 15 in __init__, then have a 
> _post_launch parameter, and pass either self._qmp_timer or None if 
> self._wrapper includes 'gdbserver'.
> 
> What I do mind is that I don’t understand why the property is modified 
> here.  The value of self._qmp_timer is supposed to be 15 by default and 
> None if self._wrapper includes 'gdbserver'.  It should thus be changed 
> to None the moment self._wrapper is made to include 'gdbserver'. Because 
> self._wrapper is set only in __init__, this should happen in __init__.
> 
>> What should __init__() do? The check here is to see if the invocation 
>> has gdb (and a couple of patches ahead also valgrind), to remove the 
>> timer.
>> If I understand what you mean, you want something like
>> def __init__(self, timer):
> 
> Oh, no.  We can optionally do that perhaps later, but what I meant is 
> just to put this in __init__() (without adding any parameters to it):
> 
> self._qmp_timer = 15.0 if 'gdbserver' not in self._wrapper else None
> 
> I think self._qmp_timer should always reflect what timeout we are going 
> to use when a VM is launched.  So if the conditions influencing the 
> timeout change, it should be updated immediately to reflect this.  The 
> only condition we have right now is the content of self._wrapper, which 
> is only set in __init__, so self._qmp_timer should be set once in 
> __init__ and not changed afterwards.
> 
> That sounds academic, but imagine what would happen if we had a 
> set_qmp_timer() method: The timout could be adjusted, but launch() would 
> just ignore it and update the property, even though the conditions 
> influencing the timout didn’t change between set_qmp_timer() and launch().
> 
> Or if we had a get_qmp_timer(); a caller would read a timeout of 15.0 
> before launch(), even though the timeout is going to be None.
> 
> Therefore, I think a property should not be updated just before it is 
> read, but instead when any condition that’s supposed to influence its 
> value changes.
> 

I agree with Max's reasoning here.

I am also not a fan of squishing magic into this class; changing class 
behavior based on introspection of wrapper arguments feels like a 
layering violation.

Maybe what you want is a subclass or a wrapper class that knows how to 
run QEMU using gdbserver, and changes some behaviors accordingly?

The factoring of Machine is quite bad already, admittedly, and is in 
need of a good spit-shine. Too many init parameters, too many state 
variables, too many methods that got patched in to support one specific 
use-case at one point or another. At a certain point, I begin to worry 
about how it's possible to audit how all of these one-off features 
behave and interact. It's getting complex.

Is it time to dream up a refactoring for how the Machine class behaves?

> 
> I suggested making it a parameter because updating a property when 
> reading it sounds like it should be a parameter instead.  I.e., one 
> would say
> 
> def __init__():
>      self._qmp_timeout_default = 15.0
> 
> def post_launch(qmp_timeout):
>      self._qmp.accept(qmp_timeout)
> 
> def launch(self):
>      ...
>      qmp_timeout = None if 'gdbserver' in self._wrapper \
>                         else self._qmp_timout_default
>      self.post_launch(qmp_timeout)
> 
> 
> Which is basically the structure your patch has, which gave me the idea.
> 
> [...]
> 
>>>>           self._post_launch()
>>>>       def _early_cleanup(self) -> None:
>>>> diff --git a/tests/qemu-iotests/iotests.py 
>>>> b/tests/qemu-iotests/iotests.py
>>>> index 05d0dc0751..380527245e 100644
>>>> --- a/tests/qemu-iotests/iotests.py
>>>> +++ b/tests/qemu-iotests/iotests.py
> 
> [...]
> 
>>>> @@ -684,6 +687,11 @@ def qmp_to_opts(self, obj):
>>>>               output_list += [key + '=' + obj[key]]
>>>>           return ','.join(output_list)
>>>> +    def get_qmp_events(self, wait: bool = False) -> List[QMPMessage]:
>>>> +        if qemu_gdb:
>>>> +            wait = 0.0
> 
> [...]
> 
>>>
>>> Second, I don’t understand this.  If the caller wants to block 
>>> waiting on an event, then that should have nothing to do with whether 
>>> we have gdb running or not.  As far as I understand, setting wait to 
>>> 0.0 is the same as wait = False, i.e. we don’t block and just return 
>>> None immediately if there is no pending event.
>>
>> You're right, this might not be needed here. The problem I had was 
>> that calling gdb and pausing at a breakpoint or something for a while 
>> would make the QMP socket timeout, thus aborting the whole test. In 
>> order to avoid that, I need to stop or delay timers.
>>
>> I can't remember why I added this check here. At some point I am sure 
>> the test was failing because of socket timeout expiration, but I 
>> cannot reproduce the problem when commenting out this check above in 
>> get_qmp_events. The other check in patch 3 should be enough.
> 
> Hm, ok.  I’d guessed that you intended the wait=0.0 or wait=False to 
> mean that we get an infinite timeout (i.e., no timeout), but that’s 
> exactly why I didn’t get it.  wait=0.0 doesn’t give an infinite timeout, 
> but instead basically times out immediately.
> 
> Max

Well, I suppose if we don't need it, then that makes things easier too :)

[PATCH v3 06/15] qemu_iotests: insert gdbserver command line as wrapper for qemu binary

[Emanuele Giuseppe Esposito]

Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
---
 tests/qemu-iotests/iotests.py | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 380527245e..4f3fb13915 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -582,7 +582,8 @@ 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,
+        super().__init__(qemu_prog, qemu_opts, wrapper=qemu_gdb,
+                         name=name,
                          test_dir=test_dir,
                          socket_scm_helper=socket_scm_helper,
                          sock_dir=sock_dir)
-- 
2.30.2

Re: [PATCH v3 06/15] qemu_iotests: insert gdbserver command line as wrapper for qemu binary

[Max Reitz]

On 14.04.21 19:03, Emanuele Giuseppe Esposito wrote:
> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> ---
>   tests/qemu-iotests/iotests.py | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)

Reviewed-by: Max Reitz <mreitz@redhat.com>

[PATCH v3 07/15] qemu-iotests: add gdbserver option to script tests too

[Emanuele Giuseppe Esposito]

The only limitation here is that running a script with gdbserver
will make the test output mismatch with the expected
results, making the test fail.

Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
---
 tests/qemu-iotests/common.rc | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/tests/qemu-iotests/common.rc b/tests/qemu-iotests/common.rc
index 65cdba5723..53a3310fee 100644
--- a/tests/qemu-iotests/common.rc
+++ b/tests/qemu-iotests/common.rc
@@ -166,8 +166,14 @@ _qemu_wrapper()
         if [ -n "${QEMU_NEED_PID}" ]; then
             echo $BASHPID > "${QEMU_TEST_DIR}/qemu-${_QEMU_HANDLE}.pid"
         fi
+
+        GDB="${QEMU_PROG}"
+        if [ ! -z ${GDB_QEMU} ]; then
+            GDB="gdbserver ${GDB_QEMU} ${GDB}"
+        fi
+
         VALGRIND_QEMU="${VALGRIND_QEMU_VM}" _qemu_proc_exec "${VALGRIND_LOGFILE}" \
-            "$QEMU_PROG" $QEMU_OPTIONS "$@"
+           $GDB $QEMU_OPTIONS "$@"
     )
     RETVAL=$?
     _qemu_proc_valgrind_log "${VALGRIND_LOGFILE}" $RETVAL
-- 
2.30.2

Re: [PATCH v3 07/15] qemu-iotests: add gdbserver option to script tests too

[Max Reitz]

On 14.04.21 19:03, Emanuele Giuseppe Esposito wrote:
> The only limitation here is that running a script with gdbserver
> will make the test output mismatch with the expected
> results, making the test fail.
> 
> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> ---
>   tests/qemu-iotests/common.rc | 8 +++++++-
>   1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/tests/qemu-iotests/common.rc b/tests/qemu-iotests/common.rc
> index 65cdba5723..53a3310fee 100644
> --- a/tests/qemu-iotests/common.rc
> +++ b/tests/qemu-iotests/common.rc
> @@ -166,8 +166,14 @@ _qemu_wrapper()
>           if [ -n "${QEMU_NEED_PID}" ]; then
>               echo $BASHPID > "${QEMU_TEST_DIR}/qemu-${_QEMU_HANDLE}.pid"
>           fi
> +
> +        GDB="${QEMU_PROG}"
> +        if [ ! -z ${GDB_QEMU} ]; then
> +            GDB="gdbserver ${GDB_QEMU} ${GDB}"
> +        fi
> +
>           VALGRIND_QEMU="${VALGRIND_QEMU_VM}" _qemu_proc_exec "${VALGRIND_LOGFILE}" \
> -            "$QEMU_PROG" $QEMU_OPTIONS "$@"
> +           $GDB $QEMU_OPTIONS "$@"

This looks strange, because now there is no qemu here.

Why not just fill $GDB with the GDB options, keeping it empty if no gdb 
is to be used?  Then we’d have

VALGRIND_QEMU=... ... \
     $GDB "$QEMU_PROG" $QEMU_OPTIONS "$@"

here.

(Also, the indentation changed.  I’d keep it aligned to four spaces.)

Max

>       )
>       RETVAL=$?
>       _qemu_proc_valgrind_log "${VALGRIND_LOGFILE}" $RETVAL
> 

[PATCH v3 08/15] docs/devel/testing: add -gdb option to the debugging section of QEMU iotests

[Emanuele Giuseppe Esposito]

Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
---
 docs/devel/testing.rst | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/docs/devel/testing.rst b/docs/devel/testing.rst
index b7e2370e7e..2ee77a057b 100644
--- a/docs/devel/testing.rst
+++ b/docs/devel/testing.rst
@@ -229,6 +229,13 @@ Debugging a test case
 QEMU iotests offers some options to debug a failing test, that can be
 given as options to the ``check`` script:
 
+* ``-gdb`` wraps ``gdbsever`` to the QEMU binary,
+  so it is possible to connect to it via gdb.
+  One way to do so is via ``gdb -iex "target remote $GDB_QEMU"``
+  The default address is ``localhost:12345``, and can be changed
+  by setting the ``$GDB_QEMU`` environmental variable.
+  The final command line will be ``gdbserver $GDB_QEMU $QEMU ...``
+
 * ``-d`` (debug) just increases the logging verbosity, showing
   for example the QMP commands and answers.
 
-- 
2.30.2

Re: [PATCH v3 08/15] docs/devel/testing: add -gdb option to the debugging section of QEMU iotests

[Max Reitz]

On 14.04.21 19:03, Emanuele Giuseppe Esposito wrote:
> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> ---
>   docs/devel/testing.rst | 7 +++++++
>   1 file changed, 7 insertions(+)
> 
> diff --git a/docs/devel/testing.rst b/docs/devel/testing.rst
> index b7e2370e7e..2ee77a057b 100644
> --- a/docs/devel/testing.rst
> +++ b/docs/devel/testing.rst
> @@ -229,6 +229,13 @@ Debugging a test case
>   QEMU iotests offers some options to debug a failing test, that can be
>   given as options to the ``check`` script:
>   
> +* ``-gdb`` wraps ``gdbsever`` to the QEMU binary,
> +  so it is possible to connect to it via gdb.
> +  One way to do so is via ``gdb -iex "target remote $GDB_QEMU"``
> +  The default address is ``localhost:12345``, and can be changed
> +  by setting the ``$GDB_QEMU`` environmental variable.

*environment variable

> +  The final command line will be ``gdbserver $GDB_QEMU $QEMU ...``
> +

I think the order in this explanation is ordered not quite right, 
because it uses $GDB_QEMU before explaining what it is.  (Also, I 
suppose $GDB_QEMU might contain other options than the socket address, 
so "target remote $GDB_QEMU" does not necessarily work.)  I’d 
reorder/change it to:

``-gdb`` wraps every QEMU invocation in a ``gdbserver``, which waits for 
a connection from a gdb client.  The options given to ``gdbserver`` 
(e.g. the address on which to listen for connections) are taken from the 
``$GDB_QEMU`` environment variable.  By default (if ``$GDB_QEMU`` is 
empty), it listens on ``localhost:12345``.
You can connect to it for example with ``gdb -iex "target remote 
$addr"``, where ``$addr`` is the address ``gdbserver`` listens on.

Max

>   * ``-d`` (debug) just increases the logging verbosity, showing
>     for example the QMP commands and answers.
>   
> 

[PATCH v3 09/15] qemu_iotests: extend the check script to support valgrind for python tests

[Emanuele Giuseppe Esposito]

Currently, the check script only parses the option and sets the
VALGRIND_QEMU environmental variable to "y".
Add another local python variable that prepares the command line,
identical to the one provided in the test scripts.

Because the python script does not know in advance the valgring
PID to assign to the log file name, use the "%p" flag in valgrind
log file name that automatically puts the process PID at runtime.

Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
---
 tests/qemu-iotests/check      |  7 ++++---
 tests/qemu-iotests/iotests.py | 11 +++++++++++
 tests/qemu-iotests/testenv.py |  1 +
 3 files changed, 16 insertions(+), 3 deletions(-)

diff --git a/tests/qemu-iotests/check b/tests/qemu-iotests/check
index 6186495eee..489178d9a4 100755
--- a/tests/qemu-iotests/check
+++ b/tests/qemu-iotests/check
@@ -36,6 +36,10 @@ def make_argparser() -> argparse.ArgumentParser:
     p.add_argument('-gdb', action='store_true',
                    help="start gdbserver with $GDB_QEMU options. \
                          Default is localhost:12345")
+    p.add_argument('-valgrind', action='store_true',
+                    help='use valgrind, sets VALGRIND_QEMU environment '
+                    'variable')
+
     p.add_argument('-misalign', action='store_true',
                    help='misalign memory allocations')
     p.add_argument('--color', choices=['on', 'off', 'auto'],
@@ -86,9 +90,6 @@ def make_argparser() -> argparse.ArgumentParser:
     g_bash.add_argument('-o', dest='imgopts',
                         help='options to pass to qemu-img create/convert, '
                         'sets IMGOPTS environment variable')
-    g_bash.add_argument('-valgrind', action='store_true',
-                        help='use valgrind, sets VALGRIND_QEMU environment '
-                        'variable')
 
     g_sel = p.add_argument_group('test selecting options',
                                  'The following options specify test set '
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 4f3fb13915..a2e8604674 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -96,6 +96,17 @@
     sys.stderr.write('Please run this test via the "check" script\n')
     sys.exit(os.EX_USAGE)
 
+qemu_valgrind = []
+if os.environ.get('VALGRIND_QEMU') == "y" and \
+    os.environ.get('NO_VALGRIND') != "y":
+    valgrind_logfile = "--log-file=" + test_dir.strip()
+    # %p allows to put the valgrind process PID, since
+    # we don't know it a priori (subprocess.Peopen is
+    # not yet invoked)
+    valgrind_logfile += "/%p.valgrind"
+
+    qemu_valgrind = ['valgrind', valgrind_logfile, '--error-exitcode=99']
+
 socket_scm_helper = os.environ.get('SOCKET_SCM_HELPER', 'socket_scm_helper')
 
 luks_default_secret_object = 'secret,id=keysec0,data=' + \
diff --git a/tests/qemu-iotests/testenv.py b/tests/qemu-iotests/testenv.py
index e131ff42cb..39ae7ace33 100644
--- a/tests/qemu-iotests/testenv.py
+++ b/tests/qemu-iotests/testenv.py
@@ -277,6 +277,7 @@ def print_env(self) -> None:
 SOCK_DIR      -- {SOCK_DIR}
 SOCKET_SCM_HELPER -- {SOCKET_SCM_HELPER}
 GDB_QEMU      -- "{GDB_QEMU}"
+VALGRIND_QEMU     -- "{VALGRIND_QEMU}"
 """
 
         args = collections.defaultdict(str, self.get_env())
-- 
2.30.2

Re: [PATCH v3 09/15] qemu_iotests: extend the check script to support valgrind for python tests

[Max Reitz]

On 14.04.21 19:03, Emanuele Giuseppe Esposito wrote:
> Currently, the check script only parses the option and sets the
> VALGRIND_QEMU environmental variable to "y".
> Add another local python variable that prepares the command line,
> identical to the one provided in the test scripts.
> 
> Because the python script does not know in advance the valgring
> PID to assign to the log file name, use the "%p" flag in valgrind
> log file name that automatically puts the process PID at runtime.
> 
> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> ---
>   tests/qemu-iotests/check      |  7 ++++---
>   tests/qemu-iotests/iotests.py | 11 +++++++++++
>   tests/qemu-iotests/testenv.py |  1 +
>   3 files changed, 16 insertions(+), 3 deletions(-)
> 
> diff --git a/tests/qemu-iotests/check b/tests/qemu-iotests/check
> index 6186495eee..489178d9a4 100755
> --- a/tests/qemu-iotests/check
> +++ b/tests/qemu-iotests/check
> @@ -36,6 +36,10 @@ def make_argparser() -> argparse.ArgumentParser:
>       p.add_argument('-gdb', action='store_true',
>                      help="start gdbserver with $GDB_QEMU options. \
>                            Default is localhost:12345")
> +    p.add_argument('-valgrind', action='store_true',
> +                    help='use valgrind, sets VALGRIND_QEMU environment '
> +                    'variable')
> +
>       p.add_argument('-misalign', action='store_true',
>                      help='misalign memory allocations')
>       p.add_argument('--color', choices=['on', 'off', 'auto'],
> @@ -86,9 +90,6 @@ def make_argparser() -> argparse.ArgumentParser:
>       g_bash.add_argument('-o', dest='imgopts',
>                           help='options to pass to qemu-img create/convert, '
>                           'sets IMGOPTS environment variable')
> -    g_bash.add_argument('-valgrind', action='store_true',
> -                        help='use valgrind, sets VALGRIND_QEMU environment '
> -                        'variable')
>   
>       g_sel = p.add_argument_group('test selecting options',
>                                    'The following options specify test set '
> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
> index 4f3fb13915..a2e8604674 100644
> --- a/tests/qemu-iotests/iotests.py
> +++ b/tests/qemu-iotests/iotests.py
> @@ -96,6 +96,17 @@
>       sys.stderr.write('Please run this test via the "check" script\n')
>       sys.exit(os.EX_USAGE)
>   
> +qemu_valgrind = []
> +if os.environ.get('VALGRIND_QEMU') == "y" and \
> +    os.environ.get('NO_VALGRIND') != "y":
> +    valgrind_logfile = "--log-file=" + test_dir.strip()
> +    # %p allows to put the valgrind process PID, since
> +    # we don't know it a priori (subprocess.Peopen is

*Popen

> +    # not yet invoked)
> +    valgrind_logfile += "/%p.valgrind"
> +
> +    qemu_valgrind = ['valgrind', valgrind_logfile, '--error-exitcode=99']
> +
>   socket_scm_helper = os.environ.get('SOCKET_SCM_HELPER', 'socket_scm_helper')
>   
>   luks_default_secret_object = 'secret,id=keysec0,data=' + \
> diff --git a/tests/qemu-iotests/testenv.py b/tests/qemu-iotests/testenv.py
> index e131ff42cb..39ae7ace33 100644
> --- a/tests/qemu-iotests/testenv.py
> +++ b/tests/qemu-iotests/testenv.py
> @@ -277,6 +277,7 @@ def print_env(self) -> None:
>   SOCK_DIR      -- {SOCK_DIR}
>   SOCKET_SCM_HELPER -- {SOCKET_SCM_HELPER}
>   GDB_QEMU      -- "{GDB_QEMU}"
> +VALGRIND_QEMU     -- "{VALGRIND_QEMU}"

I don’t think this should be enclosed in quotes (and neither should 
GDB_QEMU be).  AFAIU, only the *_PROG options are quoted to signify that 
they will be executed as a single program, whether they include spaces 
or not.  OTOH, GDB_QEMU is a list of options, so spaces act as 
separators, and VALGRIND_QEMU is just y or not set, so it’s impossible 
for spaces to be in there.

(Also, I think the “--” should be aligned to all the other “--”s.)

Bikeshedding, though, so:

Reviewed-by: Max Reitz <mreitz@redhat.com>

>   """
>   
>           args = collections.defaultdict(str, self.get_env())
> 

[PATCH v3 10/15] qemu_iotests: extent QMP socket timeout when using valgrind

[Emanuele Giuseppe Esposito]

As with gdbserver, valgrind delays the test execution, so
the default QMP socket timeout timeout too soon.

Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
---
 python/qemu/machine.py        | 2 +-
 tests/qemu-iotests/iotests.py | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/python/qemu/machine.py b/python/qemu/machine.py
index d6142271c2..dce96e1858 100644
--- a/python/qemu/machine.py
+++ b/python/qemu/machine.py
@@ -410,7 +410,7 @@ def _launch(self) -> None:
                                        shell=False,
                                        close_fds=False)
 
-        if 'gdbserver' in self._wrapper:
+        if 'gdbserver' in self._wrapper or 'valgrind' in self._wrapper:
             self._qmp_timer = None
         self._post_launch()
 
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index a2e8604674..94597433fa 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -489,7 +489,7 @@ def log(msg: Msg,
 
 class Timeout:
     def __init__(self, seconds, errmsg="Timeout"):
-        if qemu_gdb:
+        if qemu_gdb or qemu_valgrind:
             self.seconds = 3000
         else:
             self.seconds = seconds
@@ -700,7 +700,7 @@ def qmp_to_opts(self, obj):
         return ','.join(output_list)
 
     def get_qmp_events(self, wait: bool = False) -> List[QMPMessage]:
-        if qemu_gdb:
+        if qemu_gdb or qemu_valgrind:
             wait = 0.0
         return super().get_qmp_events(wait=wait)
 
-- 
2.30.2

Re: [PATCH v3 10/15] qemu_iotests: extent QMP socket timeout when using valgrind

[Max Reitz]

On 14.04.21 19:03, Emanuele Giuseppe Esposito wrote:
> As with gdbserver, valgrind delays the test execution, so
> the default QMP socket timeout timeout too soon.

I’m curious: The default timeouts should be long enough for slow 
systems, too, though (e.g. heavily-loaded CI systems).  I would expect 
that valgrind is used on developer systems where there is more leeway, 
so the timeouts should still work.

But in practice, thinking about that doesn’t matter.  If valgrind leads 
to a timeout being hit, that wouldn’t be nice.  OTOH, if you run 
valgrind to debug a test/qemu, you don’t particularly care about the 
timeouts anyway.

So in principle, this patch sounds good to me, it’s just that it’s based 
on patch 5, which I don’t fully agree with.

Max

> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> ---
>   python/qemu/machine.py        | 2 +-
>   tests/qemu-iotests/iotests.py | 4 ++--
>   2 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/python/qemu/machine.py b/python/qemu/machine.py
> index d6142271c2..dce96e1858 100644
> --- a/python/qemu/machine.py
> +++ b/python/qemu/machine.py
> @@ -410,7 +410,7 @@ def _launch(self) -> None:
>                                          shell=False,
>                                          close_fds=False)
>   
> -        if 'gdbserver' in self._wrapper:
> +        if 'gdbserver' in self._wrapper or 'valgrind' in self._wrapper:
>               self._qmp_timer = None
>           self._post_launch()
>   
> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
> index a2e8604674..94597433fa 100644
> --- a/tests/qemu-iotests/iotests.py
> +++ b/tests/qemu-iotests/iotests.py
> @@ -489,7 +489,7 @@ def log(msg: Msg,
>   
>   class Timeout:
>       def __init__(self, seconds, errmsg="Timeout"):
> -        if qemu_gdb:
> +        if qemu_gdb or qemu_valgrind:
>               self.seconds = 3000
>           else:
>               self.seconds = seconds
> @@ -700,7 +700,7 @@ def qmp_to_opts(self, obj):
>           return ','.join(output_list)
>   
>       def get_qmp_events(self, wait: bool = False) -> List[QMPMessage]:
> -        if qemu_gdb:
> +        if qemu_gdb or qemu_valgrind:
>               wait = 0.0
>           return super().get_qmp_events(wait=wait)
>   
> 

Re: [PATCH v3 10/15] qemu_iotests: extent QMP socket timeout when using valgrind

[Emanuele Giuseppe Esposito]

On 30/04/2021 15:02, Max Reitz wrote:
> On 14.04.21 19:03, Emanuele Giuseppe Esposito wrote:
>> As with gdbserver, valgrind delays the test execution, so
>> the default QMP socket timeout timeout too soon.
> 
> I’m curious: The default timeouts should be long enough for slow 
> systems, too, though (e.g. heavily-loaded CI systems).  I would expect 
> that valgrind is used on developer systems where there is more leeway, 
> so the timeouts should still work.

As said in patch 5, I will check again which timeout is essential to 
avoid and which not.

Emanuele
> 
> But in practice, thinking about that doesn’t matter.  If valgrind leads 
> to a timeout being hit, that wouldn’t be nice.  OTOH, if you run 
> valgrind to debug a test/qemu, you don’t particularly care about the 
> timeouts anyway.
> 
> So in principle, this patch sounds good to me, it’s just that it’s based 
> on patch 5, which I don’t fully agree with.
> 
> Max
> 
>> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
>> ---
>>   python/qemu/machine.py        | 2 +-
>>   tests/qemu-iotests/iotests.py | 4 ++--
>>   2 files changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/python/qemu/machine.py b/python/qemu/machine.py
>> index d6142271c2..dce96e1858 100644
>> --- a/python/qemu/machine.py
>> +++ b/python/qemu/machine.py
>> @@ -410,7 +410,7 @@ def _launch(self) -> None:
>>                                          shell=False,
>>                                          close_fds=False)
>> -        if 'gdbserver' in self._wrapper:
>> +        if 'gdbserver' in self._wrapper or 'valgrind' in self._wrapper:
>>               self._qmp_timer = None
>>           self._post_launch()
>> diff --git a/tests/qemu-iotests/iotests.py 
>> b/tests/qemu-iotests/iotests.py
>> index a2e8604674..94597433fa 100644
>> --- a/tests/qemu-iotests/iotests.py
>> +++ b/tests/qemu-iotests/iotests.py
>> @@ -489,7 +489,7 @@ def log(msg: Msg,
>>   class Timeout:
>>       def __init__(self, seconds, errmsg="Timeout"):
>> -        if qemu_gdb:
>> +        if qemu_gdb or qemu_valgrind:
>>               self.seconds = 3000
>>           else:
>>               self.seconds = seconds
>> @@ -700,7 +700,7 @@ def qmp_to_opts(self, obj):
>>           return ','.join(output_list)
>>       def get_qmp_events(self, wait: bool = False) -> List[QMPMessage]:
>> -        if qemu_gdb:
>> +        if qemu_gdb or qemu_valgrind:
>>               wait = 0.0
>>           return super().get_qmp_events(wait=wait)
>>
> 

Re: [PATCH v3 10/15] qemu_iotests: extent QMP socket timeout when using valgrind

[John Snow]

On 4/14/21 1:03 PM, Emanuele Giuseppe Esposito wrote:
> As with gdbserver, valgrind delays the test execution, so
> the default QMP socket timeout timeout too soon.
> 
> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> ---
>   python/qemu/machine.py        | 2 +-
>   tests/qemu-iotests/iotests.py | 4 ++--
>   2 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/python/qemu/machine.py b/python/qemu/machine.py
> index d6142271c2..dce96e1858 100644
> --- a/python/qemu/machine.py
> +++ b/python/qemu/machine.py
> @@ -410,7 +410,7 @@ def _launch(self) -> None:
>                                          shell=False,
>                                          close_fds=False)
>   
> -        if 'gdbserver' in self._wrapper:
> +        if 'gdbserver' in self._wrapper or 'valgrind' in self._wrapper:

This approaches me suggesting that we just change __init__ to accept a 
parameter that lets the caller decide what kind of timeout(s) they find 
acceptable. They know more about what they're trying to run than we do.

Certainly after launch occurs, the user is free to just grab the qmp 
object and tinker around with the timeouts, but that does not allow us 
to change the timeout(s) for accept itself.

D'oh.

(Spilled milk: It was probably a mistake to make the default launch 
behavior here have a timeout of 15 seconds. That logic likely belongs to 
the iotests implementation. The default here probably ought to indeed be 
"wait forever".)

In the here and now ... would it be acceptable to change the launch() 
method to add a timeout parameter? It's still a little awkward, because 
conceptually it's a timeout for just QMP and not for the actual duration 
of the entire launch process.

But, I guess, it's *closer* to the truth.

If you wanted to route it that way, I take back what I said about not 
wanting to pass around variables to event loop hooks.

If we defined the timeout as something that applies exclusively to the 
launching process, then it'd be appropriate to route that to the 
launch-related functions ... and subclasses would have to be adjusted to 
be made aware that they're expected to operate within those parameters, 
which is good.

Sorry for my waffling back and forth on this. Let me know what the 
actual requirements are if you figure out which timeouts you need / 
don't need and I'll give you some review priority.

If you attack this series again, can you please split out the python/* 
patches into its own little series and CC me?

--js

>               self._qmp_timer = None
>           self._post_launch()
>   
> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
> index a2e8604674..94597433fa 100644
> --- a/tests/qemu-iotests/iotests.py
> +++ b/tests/qemu-iotests/iotests.py
> @@ -489,7 +489,7 @@ def log(msg: Msg,
>   
>   class Timeout:
>       def __init__(self, seconds, errmsg="Timeout"):
> -        if qemu_gdb:
> +        if qemu_gdb or qemu_valgrind:
>               self.seconds = 3000
>           else:
>               self.seconds = seconds
> @@ -700,7 +700,7 @@ def qmp_to_opts(self, obj):
>           return ','.join(output_list)
>   
>       def get_qmp_events(self, wait: bool = False) -> List[QMPMessage]:
> -        if qemu_gdb:
> +        if qemu_gdb or qemu_valgrind:
>               wait = 0.0
>           return super().get_qmp_events(wait=wait)
>   
> 

Re: [PATCH v3 10/15] qemu_iotests: extent QMP socket timeout when using valgrind

[Emanuele Giuseppe Esposito]

On 13/05/2021 20:47, John Snow wrote:
> On 4/14/21 1:03 PM, Emanuele Giuseppe Esposito wrote:
>> As with gdbserver, valgrind delays the test execution, so
>> the default QMP socket timeout timeout too soon.
>>
>> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
>> ---
>>   python/qemu/machine.py        | 2 +-
>>   tests/qemu-iotests/iotests.py | 4 ++--
>>   2 files changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/python/qemu/machine.py b/python/qemu/machine.py
>> index d6142271c2..dce96e1858 100644
>> --- a/python/qemu/machine.py
>> +++ b/python/qemu/machine.py
>> @@ -410,7 +410,7 @@ def _launch(self) -> None:
>>                                          shell=False,
>>                                          close_fds=False)
>> -        if 'gdbserver' in self._wrapper:
>> +        if 'gdbserver' in self._wrapper or 'valgrind' in self._wrapper:
> 
> This approaches me suggesting that we just change __init__ to accept a 
> parameter that lets the caller decide what kind of timeout(s) they find 
> acceptable. They know more about what they're trying to run than we do.
> 
> Certainly after launch occurs, the user is free to just grab the qmp 
> object and tinker around with the timeouts, but that does not allow us 
> to change the timeout(s) for accept itself.
> 
> D'oh.
> 
> (Spilled milk: It was probably a mistake to make the default launch 
> behavior here have a timeout of 15 seconds. That logic likely belongs to 
> the iotests implementation. The default here probably ought to indeed be 
> "wait forever".)
> 
> In the here and now ... would it be acceptable to change the launch() 
> method to add a timeout parameter? It's still a little awkward, because 
> conceptually it's a timeout for just QMP and not for the actual duration 
> of the entire launch process.
> 
> But, I guess, it's *closer* to the truth.
> 
> If you wanted to route it that way, I take back what I said about not 
> wanting to pass around variables to event loop hooks.
> 
> If we defined the timeout as something that applies exclusively to the 
> launching process, then it'd be appropriate to route that to the 
> launch-related functions ... and subclasses would have to be adjusted to 
> be made aware that they're expected to operate within those parameters, 
> which is good.
> 
> Sorry for my waffling back and forth on this. Let me know what the 
> actual requirements are if you figure out which timeouts you need / 
> don't need and I'll give you some review priority.

Uhm.. I am getting a little bit confused on what to do too :)

So the current plan I have for _qmp_timer is:

- As Max suggested, move it in __init__ and check there for the wrapper 
contents. If we need to block forever (gdb, valgrind), we set it to 
None. Otherwise to 15 seconds. I think setting it always to None is not 
ideal, because if you are testing something that deadlocks (see my 
attempts to remove/add locks in QEMU multiqueue) and the socket is set 
to block forever, you don't know if the test is super slow or it just 
deadlocked.

Well, one can argue that in both cases this is not the expected 
behavior, but I think having an upper bound on each QMP command 
execution would be good.

- pass _qmp_timer directly to self._qmp.accept() in _post launch, 
leaving _launch() intact. I think this makes sense because as you also 
mentioned, changing _post_launch() into taking a parameter requires 
changing also all subclasses and pass values around.

Any opinion on this is very welcome.

Spoiler alert I haven't tested these changes yet, but I am positive that 
there shouldn't be any problem. (Last famous words)

Emanuele


> 
> If you attack this series again, can you please split out the python/* 
> patches into its own little series and CC me?
> 
> --js
> 
>>               self._qmp_timer = None
>>           self._post_launch()
>> diff --git a/tests/qemu-iotests/iotests.py 
>> b/tests/qemu-iotests/iotests.py
>> index a2e8604674..94597433fa 100644
>> --- a/tests/qemu-iotests/iotests.py
>> +++ b/tests/qemu-iotests/iotests.py
>> @@ -489,7 +489,7 @@ def log(msg: Msg,
>>   class Timeout:
>>       def __init__(self, seconds, errmsg="Timeout"):
>> -        if qemu_gdb:
>> +        if qemu_gdb or qemu_valgrind:
>>               self.seconds = 3000
>>           else:
>>               self.seconds = seconds
>> @@ -700,7 +700,7 @@ def qmp_to_opts(self, obj):
>>           return ','.join(output_list)
>>       def get_qmp_events(self, wait: bool = False) -> List[QMPMessage]:
>> -        if qemu_gdb:
>> +        if qemu_gdb or qemu_valgrind:
>>               wait = 0.0
>>           return super().get_qmp_events(wait=wait)
>>
> 

Re: [PATCH v3 10/15] qemu_iotests: extent QMP socket timeout when using valgrind

[John Snow]

On 5/14/21 4:16 AM, Emanuele Giuseppe Esposito wrote:
> 
> 
> On 13/05/2021 20:47, John Snow wrote:
>> On 4/14/21 1:03 PM, Emanuele Giuseppe Esposito wrote:
>>> As with gdbserver, valgrind delays the test execution, so
>>> the default QMP socket timeout timeout too soon.
>>>
>>> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
>>> ---
>>>   python/qemu/machine.py        | 2 +-
>>>   tests/qemu-iotests/iotests.py | 4 ++--
>>>   2 files changed, 3 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/python/qemu/machine.py b/python/qemu/machine.py
>>> index d6142271c2..dce96e1858 100644
>>> --- a/python/qemu/machine.py
>>> +++ b/python/qemu/machine.py
>>> @@ -410,7 +410,7 @@ def _launch(self) -> None:
>>>                                          shell=False,
>>>                                          close_fds=False)
>>> -        if 'gdbserver' in self._wrapper:
>>> +        if 'gdbserver' in self._wrapper or 'valgrind' in self._wrapper:
>>
>> This approaches me suggesting that we just change __init__ to accept a 
>> parameter that lets the caller decide what kind of timeout(s) they 
>> find acceptable. They know more about what they're trying to run than 
>> we do.
>>
>> Certainly after launch occurs, the user is free to just grab the qmp 
>> object and tinker around with the timeouts, but that does not allow us 
>> to change the timeout(s) for accept itself.
>>
>> D'oh.
>>
>> (Spilled milk: It was probably a mistake to make the default launch 
>> behavior here have a timeout of 15 seconds. That logic likely belongs 
>> to the iotests implementation. The default here probably ought to 
>> indeed be "wait forever".)
>>
>> In the here and now ... would it be acceptable to change the launch() 
>> method to add a timeout parameter? It's still a little awkward, 
>> because conceptually it's a timeout for just QMP and not for the 
>> actual duration of the entire launch process.
>>
>> But, I guess, it's *closer* to the truth.
>>
>> If you wanted to route it that way, I take back what I said about not 
>> wanting to pass around variables to event loop hooks.
>>
>> If we defined the timeout as something that applies exclusively to the 
>> launching process, then it'd be appropriate to route that to the 
>> launch-related functions ... and subclasses would have to be adjusted 
>> to be made aware that they're expected to operate within those 
>> parameters, which is good.
>>
>> Sorry for my waffling back and forth on this. Let me know what the 
>> actual requirements are if you figure out which timeouts you need / 
>> don't need and I'll give you some review priority.
> 
> Uhm.. I am getting a little bit confused on what to do too :)
> 

SORRY, I hit send too quickly and then change my mind. I've handed you a 
giant bag of my own confusion. Very unfair of me!

> So the current plan I have for _qmp_timer is:
> 
> - As Max suggested, move it in __init__ and check there for the wrapper 
> contents. If we need to block forever (gdb, valgrind), we set it to 
> None. Otherwise to 15 seconds. I think setting it always to None is not 
> ideal, because if you are testing something that deadlocks (see my 
> attempts to remove/add locks in QEMU multiqueue) and the socket is set 
> to block forever, you don't know if the test is super slow or it just 
> deadlocked.
> 

I agree with your concern on rational defaults, let's focus on that briefly:

Let's have QEMUMachine default to *no timeouts* moving forward, and have 
the timeouts be *opt-in*. This keeps the Machine class somewhat pure and 
free of opinions. The separation of mechanism and policy.

Next, instead of modifying hundreds of tests to opt-in to the timeout, 
let's modify the VM class in iotests.py to opt-in to that timeout, 
restoring the current "safe" behavior of iotests.

The above items can happen in a single commit, preserving behavior in 
the bisect.

Finally, we can add a non-private property that individual tests can 
re-override to opt BACK out of the default.

Something as simple as:

vm.qmp_timeout = None

would be just fine.

> Well, one can argue that in both cases this is not the expected 
> behavior, but I think having an upper bound on each QMP command 
> execution would be good.
> 
> - pass _qmp_timer directly to self._qmp.accept() in _post launch, 
> leaving _launch() intact. I think this makes sense because as you also 
> mentioned, changing _post_launch() into taking a parameter requires 
> changing also all subclasses and pass values around.
> 

Sounds OK. If we do change the defaults back to "No Timeout" in a way 
that allows an override by an opinionated class, we'll already have the 
public property, though, so a parameter might not be needed.

(Yes, this is the THIRD time I've changed my mind in 48 hours.)

> Any opinion on this is very welcome.
> 

Brave words!

My last thought here is that I still don't like the idea of QEMUMachine 
class changing its timeout behavior based on the introspection of 
wrapper args.

It feels much more like the case that a caller who is knowingly wrapping 
it with a program that delays its execution should change its parameters 
accordingly based on what the caller knows about what they're trying to 
accomplish.

Does that make the code too messy? I understand you probably want to 
ensure that adding a GDB wrapper is painless and simple, so it might not 
be great to always ask a caller to remember to set some timeout value to 
use it.

I figure that the right place to do this, though, is wherever the 
boilerplate code gets written that knows how to set up the right gdb 
args and so on, and not in machine.py. It sounds like iotests.py code to 
me, maybe in the VM class.

> Spoiler alert I haven't tested these changes yet, but I am positive that 
> there shouldn't be any problem. (Last famous words)
> 
> Emanuele
> 
> 
Clear as mud?

--js

Re: [PATCH v3 10/15] qemu_iotests: extent QMP socket timeout when using valgrind

[Emanuele Giuseppe Esposito]

>> So the current plan I have for _qmp_timer is:
>>
>> - As Max suggested, move it in __init__ and check there for the 
>> wrapper contents. If we need to block forever (gdb, valgrind), we set 
>> it to None. Otherwise to 15 seconds. I think setting it always to None 
>> is not ideal, because if you are testing something that deadlocks (see 
>> my attempts to remove/add locks in QEMU multiqueue) and the socket is 
>> set to block forever, you don't know if the test is super slow or it 
>> just deadlocked.
>>
> 
> I agree with your concern on rational defaults, let's focus on that 
> briefly:
> 
> Let's have QEMUMachine default to *no timeouts* moving forward, and have 
> the timeouts be *opt-in*. This keeps the Machine class somewhat pure and 
> free of opinions. The separation of mechanism and policy.
> 
> Next, instead of modifying hundreds of tests to opt-in to the timeout, 
> let's modify the VM class in iotests.py to opt-in to that timeout, 
> restoring the current "safe" behavior of iotests.
> 
> The above items can happen in a single commit, preserving behavior in 
> the bisect.
> 
> Finally, we can add a non-private property that individual tests can 
> re-override to opt BACK out of the default.
> 
> Something as simple as:
> 
> vm.qmp_timeout = None
> 
> would be just fine.
>

I applied these suggested changes, will send v4 and we'll see what comes 
out of it.

>> Well, one can argue that in both cases this is not the expected 
>> behavior, but I think having an upper bound on each QMP command 
>> execution would be good.
>>
>> - pass _qmp_timer directly to self._qmp.accept() in _post launch, 
>> leaving _launch() intact. I think this makes sense because as you also 
>> mentioned, changing _post_launch() into taking a parameter requires 
>> changing also all subclasses and pass values around.
>>
> 
> Sounds OK. If we do change the defaults back to "No Timeout" in a way 
> that allows an override by an opinionated class, we'll already have the 
> public property, though, so a parameter might not be needed.
> 
> (Yes, this is the THIRD time I've changed my mind in 48 hours.)
> 
>> Any opinion on this is very welcome.
>>
> 
> Brave words!
> 
> My last thought here is that I still don't like the idea of QEMUMachine 
> class changing its timeout behavior based on the introspection of 
> wrapper args.
> 
> It feels much more like the case that a caller who is knowingly wrapping 
> it with a program that delays its execution should change its parameters 
> accordingly based on what the caller knows about what they're trying to 
> accomplish.
> 
> Does that make the code too messy? I understand you probably want to 
> ensure that adding a GDB wrapper is painless and simple, so it might not 
> be great to always ask a caller to remember to set some timeout value to 
> use it.
> 
I am not sure I follow you here, where do you want to move this logic? 
Can you please elaborate more, I did not understand what you mean.

I understand that tweaking the timers in iotests.py with checks like

if not (qemu_gdb or qemu_valgrind):
	normal timer

may not be the most beautiful piece of code, but as you said it keeps 
things as simple as they can.

> I figure that the right place to do this, though, is wherever the 
> boilerplate code gets written that knows how to set up the right gdb 
> args and so on, and not in machine.py. It sounds like iotests.py code to 
> me, maybe in the VM class.

After the changes suggested on qmp_timeout, iotests.py already contains 
the only code to perform the setup right for gdb and valgrind, and 
machine.py is not touched (except for qmp_timeout). iotests.py will 
essentially contain a couple of ifs like the one above, changing the 
timer when gdb and valgring are *not* needed.

Emanuele

Re: [PATCH v3 10/15] qemu_iotests: extent QMP socket timeout when using valgrind

[John Snow]

On 5/18/21 9:58 AM, Emanuele Giuseppe Esposito wrote:
> 
>>> So the current plan I have for _qmp_timer is:
>>>
>>> - As Max suggested, move it in __init__ and check there for the 
>>> wrapper contents. If we need to block forever (gdb, valgrind), we set 
>>> it to None. Otherwise to 15 seconds. I think setting it always to 
>>> None is not ideal, because if you are testing something that 
>>> deadlocks (see my attempts to remove/add locks in QEMU multiqueue) 
>>> and the socket is set to block forever, you don't know if the test is 
>>> super slow or it just deadlocked.
>>>
>>
>> I agree with your concern on rational defaults, let's focus on that 
>> briefly:
>>
>> Let's have QEMUMachine default to *no timeouts* moving forward, and 
>> have the timeouts be *opt-in*. This keeps the Machine class somewhat 
>> pure and free of opinions. The separation of mechanism and policy.
>>
>> Next, instead of modifying hundreds of tests to opt-in to the timeout, 
>> let's modify the VM class in iotests.py to opt-in to that timeout, 
>> restoring the current "safe" behavior of iotests.
>>
>> The above items can happen in a single commit, preserving behavior in 
>> the bisect.
>>
>> Finally, we can add a non-private property that individual tests can 
>> re-override to opt BACK out of the default.
>>
>> Something as simple as:
>>
>> vm.qmp_timeout = None
>>
>> would be just fine.
>>
> 
> I applied these suggested changes, will send v4 and we'll see what comes 
> out of it.
> 
>>> Well, one can argue that in both cases this is not the expected 
>>> behavior, but I think having an upper bound on each QMP command 
>>> execution would be good.
>>>
>>> - pass _qmp_timer directly to self._qmp.accept() in _post launch, 
>>> leaving _launch() intact. I think this makes sense because as you 
>>> also mentioned, changing _post_launch() into taking a parameter 
>>> requires changing also all subclasses and pass values around.
>>>
>>
>> Sounds OK. If we do change the defaults back to "No Timeout" in a way 
>> that allows an override by an opinionated class, we'll already have 
>> the public property, though, so a parameter might not be needed.
>>
>> (Yes, this is the THIRD time I've changed my mind in 48 hours.)
>>
>>> Any opinion on this is very welcome.
>>>
>>
>> Brave words!
>>
>> My last thought here is that I still don't like the idea of 
>> QEMUMachine class changing its timeout behavior based on the 
>> introspection of wrapper args.
>>
>> It feels much more like the case that a caller who is knowingly 
>> wrapping it with a program that delays its execution should change its 
>> parameters accordingly based on what the caller knows about what 
>> they're trying to accomplish.
>>
>> Does that make the code too messy? I understand you probably want to 
>> ensure that adding a GDB wrapper is painless and simple, so it might 
>> not be great to always ask a caller to remember to set some timeout 
>> value to use it.
>>
> I am not sure I follow you here, where do you want to move this logic? 
> Can you please elaborate more, I did not understand what you mean.
> 
> I understand that tweaking the timers in iotests.py with checks like
> 
> if not (qemu_gdb or qemu_valgrind):
>      normal timer
> 
> may not be the most beautiful piece of code, but as you said it keeps 
> things as simple as they can.
> 

What I mean is that of the two patterns:

caller.py:
     vm = machine(..., wrapper_args=['gdb', ...])

machine.py:
     def __init__(...):
         if 'gdb' in wrapper_args:
             self.timer = None

vs this one:

caller.py:
     vm = machine(..., wrapper_args=['gdb', ...], timer=None)

machine.py:
     def __init__(...):
         ... # No introspection of wrapper_args


I prefer the second. I would assume it's possible to localize the logic 
that creates a GDB-wrapped machine alongside the knowledge that it needs 
the timeout turned off *outside* of the machine class.

I could be *very wrong* about that assumption though. The reason I 
prefer the second pattern is because it avoids having to deal with what 
happens when a caller specifies both a timeout and a gdb-wrapper. In the 
second case, the caller explicitly requested the timeout to be None, so 
anything that occurs afterwards is the fault of the caller, not machine.py.

To me, that's "simpler". (I could be wrong, I don't have a great overall 
view of your series, just the bits that I have seen that touch machine.py.)

--js

>> I figure that the right place to do this, though, is wherever the 
>> boilerplate code gets written that knows how to set up the right gdb 
>> args and so on, and not in machine.py. It sounds like iotests.py code 
>> to me, maybe in the VM class.
> 
> After the changes suggested on qmp_timeout, iotests.py already contains 
> the only code to perform the setup right for gdb and valgrind, and 
> machine.py is not touched (except for qmp_timeout). iotests.py will 
> essentially contain a couple of ifs like the one above, changing the 
> timer when gdb and valgring are *not* needed.
> 
> Emanuele
> 

Re: [PATCH v3 10/15] qemu_iotests: extent QMP socket timeout when using valgrind

[Emanuele Giuseppe Esposito]

On 18/05/2021 16:26, John Snow wrote:
> On 5/18/21 9:58 AM, Emanuele Giuseppe Esposito wrote:
>>
>>>> So the current plan I have for _qmp_timer is:
>>>>
>>>> - As Max suggested, move it in __init__ and check there for the 
>>>> wrapper contents. If we need to block forever (gdb, valgrind), we 
>>>> set it to None. Otherwise to 15 seconds. I think setting it always 
>>>> to None is not ideal, because if you are testing something that 
>>>> deadlocks (see my attempts to remove/add locks in QEMU multiqueue) 
>>>> and the socket is set to block forever, you don't know if the test 
>>>> is super slow or it just deadlocked.
>>>>
>>>
>>> I agree with your concern on rational defaults, let's focus on that 
>>> briefly:
>>>
>>> Let's have QEMUMachine default to *no timeouts* moving forward, and 
>>> have the timeouts be *opt-in*. This keeps the Machine class somewhat 
>>> pure and free of opinions. The separation of mechanism and policy.
>>>
>>> Next, instead of modifying hundreds of tests to opt-in to the 
>>> timeout, let's modify the VM class in iotests.py to opt-in to that 
>>> timeout, restoring the current "safe" behavior of iotests.
>>>
>>> The above items can happen in a single commit, preserving behavior in 
>>> the bisect.
>>>
>>> Finally, we can add a non-private property that individual tests can 
>>> re-override to opt BACK out of the default.
>>>
>>> Something as simple as:
>>>
>>> vm.qmp_timeout = None
>>>
>>> would be just fine.
>>>
>>
>> I applied these suggested changes, will send v4 and we'll see what 
>> comes out of it.
>>
>>>> Well, one can argue that in both cases this is not the expected 
>>>> behavior, but I think having an upper bound on each QMP command 
>>>> execution would be good.
>>>>
>>>> - pass _qmp_timer directly to self._qmp.accept() in _post launch, 
>>>> leaving _launch() intact. I think this makes sense because as you 
>>>> also mentioned, changing _post_launch() into taking a parameter 
>>>> requires changing also all subclasses and pass values around.
>>>>
>>>
>>> Sounds OK. If we do change the defaults back to "No Timeout" in a way 
>>> that allows an override by an opinionated class, we'll already have 
>>> the public property, though, so a parameter might not be needed.
>>>
>>> (Yes, this is the THIRD time I've changed my mind in 48 hours.)
>>>
>>>> Any opinion on this is very welcome.
>>>>
>>>
>>> Brave words!
>>>
>>> My last thought here is that I still don't like the idea of 
>>> QEMUMachine class changing its timeout behavior based on the 
>>> introspection of wrapper args.
>>>
>>> It feels much more like the case that a caller who is knowingly 
>>> wrapping it with a program that delays its execution should change 
>>> its parameters accordingly based on what the caller knows about what 
>>> they're trying to accomplish.
>>>
>>> Does that make the code too messy? I understand you probably want to 
>>> ensure that adding a GDB wrapper is painless and simple, so it might 
>>> not be great to always ask a caller to remember to set some timeout 
>>> value to use it.
>>>
>> I am not sure I follow you here, where do you want to move this logic? 
>> Can you please elaborate more, I did not understand what you mean.
>>
>> I understand that tweaking the timers in iotests.py with checks like
>>
>> if not (qemu_gdb or qemu_valgrind):
>>      normal timer
>>
>> may not be the most beautiful piece of code, but as you said it keeps 
>> things as simple as they can.
>>
> 
> What I mean is that of the two patterns:
> 
> caller.py:
>      vm = machine(..., wrapper_args=['gdb', ...])
> 
> machine.py:
>      def __init__(...):
>          if 'gdb' in wrapper_args:
>              self.timer = None
> 
> vs this one:
> 
> caller.py:
>      vm = machine(..., wrapper_args=['gdb', ...], timer=None)
> 
> machine.py:
>      def __init__(...):
>          ... # No introspection of wrapper_args
> 
> 
> I prefer the second. I would assume it's possible to localize the logic 
> that creates a GDB-wrapped machine alongside the knowledge that it needs 
> the timeout turned off *outside* of the machine class.
> 
> I could be *very wrong* about that assumption though. The reason I 
> prefer the second pattern is because it avoids having to deal with what 
> happens when a caller specifies both a timeout and a gdb-wrapper. In the 
> second case, the caller explicitly requested the timeout to be None, so 
> anything that occurs afterwards is the fault of the caller, not machine.py.
> 
> To me, that's "simpler". (I could be wrong, I don't have a great overall 
> view of your series, just the bits that I have seen that touch machine.py.)

I think this can be done almost effortless. With your suggested changes 
on qmp_timer, we can have:

machine.py
def __init__(self, ..., wrapper, timer: None)
	self._qmp_timer = timer

def _post_launch(self)
	self._qmp.accept(self._qmp_timer)

iotests.py
	timer = None if qemu_gdb or qemu_valgrind else 15.0
	wrapper = qemu_gdb or qemu_valgrind # did not know about this OR trick btw
	vm = machine(..., wrapper, timer)

Thank you,
Emanuele
> 
> --js
> 
>>> I figure that the right place to do this, though, is wherever the 
>>> boilerplate code gets written that knows how to set up the right gdb 
>>> args and so on, and not in machine.py. It sounds like iotests.py code 
>>> to me, maybe in the VM class.
>>
>> After the changes suggested on qmp_timeout, iotests.py already 
>> contains the only code to perform the setup right for gdb and 
>> valgrind, and machine.py is not touched (except for qmp_timeout). 
>> iotests.py will essentially contain a couple of ifs like the one 
>> above, changing the timer when gdb and valgring are *not* needed.
>>
>> Emanuele
>>
> 

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

[Emanuele Giuseppe Esposito]

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 <eesposit@redhat.com>
---
 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:
+
+        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)
-- 
2.30.2

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

[Max Reitz]

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 <eesposit@redhat.com>
> ---
>   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 v3 12/15] qemu_iotests: insert valgrind command line as wrapper for qemu binary

[Emanuele Giuseppe Esposito]

The priority will be given to gdb command line, meaning if the -gdb
parameter and -valgrind are given, gdb will be wrapped around
the qemu binary.

Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
---
 tests/qemu-iotests/iotests.py | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index aef67e3a86..f9832558a0 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -593,7 +593,8 @@ class VM(qtest.QEMUQtestMachine):
 
     def __init__(self, path_suffix=''):
         name = "qemu%s-%d" % (path_suffix, os.getpid())
-        super().__init__(qemu_prog, qemu_opts, wrapper=qemu_gdb,
+        wrapper = qemu_gdb if qemu_gdb else qemu_valgrind
+        super().__init__(qemu_prog, qemu_opts, wrapper=wrapper,
                          name=name,
                          test_dir=test_dir,
                          socket_scm_helper=socket_scm_helper,
-- 
2.30.2

Re: [PATCH v3 12/15] qemu_iotests: insert valgrind command line as wrapper for qemu binary

[Max Reitz]

On 14.04.21 19:03, Emanuele Giuseppe Esposito wrote:
> The priority will be given to gdb command line, meaning if the -gdb
> parameter and -valgrind are given, gdb will be wrapped around
> the qemu binary.
> 
> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> ---
>   tests/qemu-iotests/iotests.py | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)

Reviewed-by: Max Reitz <mreitz@redhat.com>

[PATCH v3 13/15] docs/devel/testing: add -valgrind option to the debug section of QEMU iotests

[Emanuele Giuseppe Esposito]

Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
---
 docs/devel/testing.rst | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/docs/devel/testing.rst b/docs/devel/testing.rst
index 2ee77a057b..62902cfd2d 100644
--- a/docs/devel/testing.rst
+++ b/docs/devel/testing.rst
@@ -236,6 +236,13 @@ given as options to the ``check`` script:
   by setting the ``$GDB_QEMU`` environmental variable.
   The final command line will be ``gdbserver $GDB_QEMU $QEMU ...``
 
+* ``-valgrind`` wraps a valgrind instance to QEMU. If it detects
+  warnings, it will print and save the log in
+  ``$TEST_DIR/<valgrind_pid>.valgrind``.
+  The final command line will be ``valgrind --log-file=$TEST_DIR/
+  <valgrind_pid>.valgrind --error-exitcode=99 $QEMU ...``
+  Note: if used together with ``-gdb``, this command will be ignored.
+
 * ``-d`` (debug) just increases the logging verbosity, showing
   for example the QMP commands and answers.
 
-- 
2.30.2

Re: [PATCH v3 13/15] docs/devel/testing: add -valgrind option to the debug section of QEMU iotests

[Max Reitz]

On 14.04.21 19:03, Emanuele Giuseppe Esposito wrote:
> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> ---
>   docs/devel/testing.rst | 7 +++++++
>   1 file changed, 7 insertions(+)
> 
> diff --git a/docs/devel/testing.rst b/docs/devel/testing.rst
> index 2ee77a057b..62902cfd2d 100644
> --- a/docs/devel/testing.rst
> +++ b/docs/devel/testing.rst
> @@ -236,6 +236,13 @@ given as options to the ``check`` script:
>     by setting the ``$GDB_QEMU`` environmental variable.
>     The final command line will be ``gdbserver $GDB_QEMU $QEMU ...``
>   
> +* ``-valgrind`` wraps a valgrind instance to QEMU. If it detects

Not a native speaker, but I think it should be “attaches ... to QEMU”, 
“wraps QEMU in a valgrind instances”, or “wraps ... around QEMU".

Apart from that:

Reviewed-by: Max Reitz <mreitz@redhat.com>

> +  warnings, it will print and save the log in
> +  ``$TEST_DIR/<valgrind_pid>.valgrind``.
> +  The final command line will be ``valgrind --log-file=$TEST_DIR/
> +  <valgrind_pid>.valgrind --error-exitcode=99 $QEMU ...``
> +  Note: if used together with ``-gdb``, this command will be ignored.
> +
>   * ``-d`` (debug) just increases the logging verbosity, showing
>     for example the QMP commands and answers.
>   
> 

[PATCH v3 14/15] qemu_iotests: add option to show qemu binary logs on stdout

[Emanuele Giuseppe Esposito]

Using the flag -p, allow the qemu binary to print to stdout.
This helps especially when doing print-debugging.

Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
---
 tests/qemu-iotests/check      | 3 ++-
 tests/qemu-iotests/iotests.py | 9 +++++++++
 tests/qemu-iotests/testenv.py | 9 +++++++--
 3 files changed, 18 insertions(+), 3 deletions(-)

diff --git a/tests/qemu-iotests/check b/tests/qemu-iotests/check
index 489178d9a4..84483922eb 100755
--- a/tests/qemu-iotests/check
+++ b/tests/qemu-iotests/check
@@ -33,6 +33,7 @@ def make_argparser() -> argparse.ArgumentParser:
                    help='pretty print output for make check')
 
     p.add_argument('-d', dest='debug', action='store_true', help='debug')
+    p.add_argument('-p', dest='print', action='store_true', help='shows qemu binary prints to stdout')
     p.add_argument('-gdb', action='store_true',
                    help="start gdbserver with $GDB_QEMU options. \
                          Default is localhost:12345")
@@ -117,7 +118,7 @@ if __name__ == '__main__':
                   aiomode=args.aiomode, cachemode=args.cachemode,
                   imgopts=args.imgopts, misalign=args.misalign,
                   debug=args.debug, valgrind=args.valgrind,
-                  gdb=args.gdb)
+                  gdb=args.gdb, qprint=args.print)
 
     testfinder = TestFinder(test_dir=env.source_iotests)
 
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index f9832558a0..52ff7332f8 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -79,6 +79,8 @@
 if os.environ.get('GDB_QEMU'):
     qemu_gdb = ['gdbserver'] + os.environ.get('GDB_QEMU').strip().split(' ')
 
+qemu_print = os.environ.get('PRINT_QEMU', False)
+
 imgfmt = os.environ.get('IMGFMT', 'raw')
 imgproto = os.environ.get('IMGPROTO', 'file')
 output_dir = os.environ.get('OUTPUT_DIR', '.')
@@ -621,6 +623,13 @@ def _post_shutdown(self) -> None:
         super()._post_shutdown()
         self.subprocess_check_valgrind(qemu_valgrind)
 
+    def _pre_launch(self) -> None:
+        super()._pre_launch()
+        if qemu_print and self._qemu_log_file != None:
+            # set QEMU binary output to stdout
+            self._qemu_log_file.close()
+            self._qemu_log_file = None
+
     def add_object(self, opts):
         self._args.append('-object')
         self._args.append(opts)
diff --git a/tests/qemu-iotests/testenv.py b/tests/qemu-iotests/testenv.py
index 39ae7ace33..6ae099114e 100644
--- a/tests/qemu-iotests/testenv.py
+++ b/tests/qemu-iotests/testenv.py
@@ -73,7 +73,7 @@ class TestEnv(ContextManager['TestEnv']):
                      'AIOMODE', 'CACHEMODE', 'VALGRIND_QEMU',
                      'CACHEMODE_IS_DEFAULT', 'IMGFMT_GENERIC', 'IMGOPTSSYNTAX',
                      'IMGKEYSECRET', 'QEMU_DEFAULT_MACHINE', 'MALLOC_PERTURB_',
-                     'GDB_QEMU']
+                     'GDB_QEMU', 'PRINT_QEMU']
 
     def get_env(self) -> Dict[str, str]:
         env = {}
@@ -165,7 +165,8 @@ def __init__(self, imgfmt: str, imgproto: str, aiomode: str,
                  misalign: bool = False,
                  debug: bool = False,
                  valgrind: bool = False,
-                 gdb: bool = False) -> None:
+                 gdb: bool = False,
+                 qprint: bool = False) -> None:
         self.imgfmt = imgfmt
         self.imgproto = imgproto
         self.aiomode = aiomode
@@ -173,6 +174,9 @@ def __init__(self, imgfmt: str, imgproto: str, aiomode: str,
         self.misalign = misalign
         self.debug = debug
 
+        if qprint:
+            self.print_qemu = 'y'
+
         if gdb:
             self.gdb_qemu = os.environ.get('GDB_QEMU', 'localhost:12345')
         elif 'GDB_QEMU' in os.environ:
@@ -278,6 +282,7 @@ def print_env(self) -> None:
 SOCKET_SCM_HELPER -- {SOCKET_SCM_HELPER}
 GDB_QEMU      -- "{GDB_QEMU}"
 VALGRIND_QEMU     -- "{VALGRIND_QEMU}"
+PRINT_QEMU    --  "{PRINT_QEMU}"
 """
 
         args = collections.defaultdict(str, self.get_env())
-- 
2.30.2

Re: [PATCH v3 14/15] qemu_iotests: add option to show qemu binary logs on stdout

[Max Reitz]

On 14.04.21 19:03, Emanuele Giuseppe Esposito wrote:
> Using the flag -p, allow the qemu binary to print to stdout.
> This helps especially when doing print-debugging.

I think this shouldn’t refer to prints but to qemu’s stdout/stderr in 
general, i.e....

> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> ---
>   tests/qemu-iotests/check      | 3 ++-
>   tests/qemu-iotests/iotests.py | 9 +++++++++
>   tests/qemu-iotests/testenv.py | 9 +++++++--
>   3 files changed, 18 insertions(+), 3 deletions(-)
> 
> diff --git a/tests/qemu-iotests/check b/tests/qemu-iotests/check
> index 489178d9a4..84483922eb 100755
> --- a/tests/qemu-iotests/check
> +++ b/tests/qemu-iotests/check
> @@ -33,6 +33,7 @@ def make_argparser() -> argparse.ArgumentParser:
>                      help='pretty print output for make check')
>   
>       p.add_argument('-d', dest='debug', action='store_true', help='debug')
> +    p.add_argument('-p', dest='print', action='store_true', help='shows qemu binary prints to stdout')

I’d prefer for the description to be “redirects qemu's stdout and stderr 
to the test output”.

Also, this line is too long.

>       p.add_argument('-gdb', action='store_true',
>                      help="start gdbserver with $GDB_QEMU options. \
>                            Default is localhost:12345")
> @@ -117,7 +118,7 @@ if __name__ == '__main__':
>                     aiomode=args.aiomode, cachemode=args.cachemode,
>                     imgopts=args.imgopts, misalign=args.misalign,
>                     debug=args.debug, valgrind=args.valgrind,
> -                  gdb=args.gdb)
> +                  gdb=args.gdb, qprint=args.print)
>   
>       testfinder = TestFinder(test_dir=env.source_iotests)
>   
> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
> index f9832558a0..52ff7332f8 100644
> --- a/tests/qemu-iotests/iotests.py
> +++ b/tests/qemu-iotests/iotests.py
> @@ -79,6 +79,8 @@
>   if os.environ.get('GDB_QEMU'):
>       qemu_gdb = ['gdbserver'] + os.environ.get('GDB_QEMU').strip().split(' ')
>   
> +qemu_print = os.environ.get('PRINT_QEMU', False)
> +
>   imgfmt = os.environ.get('IMGFMT', 'raw')
>   imgproto = os.environ.get('IMGPROTO', 'file')
>   output_dir = os.environ.get('OUTPUT_DIR', '.')
> @@ -621,6 +623,13 @@ def _post_shutdown(self) -> None:
>           super()._post_shutdown()
>           self.subprocess_check_valgrind(qemu_valgrind)
>   
> +    def _pre_launch(self) -> None:
> +        super()._pre_launch()
> +        if qemu_print and self._qemu_log_file != None:

I think "is not None" is mostly used in Python.  (I’m saying this in 
this weird way because I’m not the one to ask what’s mostly used in 
Python...)

(That also silences mypy, which otherwise complains and then fails 297.)

> +            # set QEMU binary output to stdout
> +            self._qemu_log_file.close()
> +            self._qemu_log_file = None

I don’t know enough Python to know how this works.  I suppose this does 
access the super class’s member?  (I could also imagine it creates a new 
member, just for this child class, but that then effectively overwrites 
the super class’s member.)

Considering _qemu_log_file is apparently meant to be a private attribute 
(from the leading underscore), I think it would be better to have a 
function provided by the super class for this.

> +
>       def add_object(self, opts):
>           self._args.append('-object')
>           self._args.append(opts)
> diff --git a/tests/qemu-iotests/testenv.py b/tests/qemu-iotests/testenv.py
> index 39ae7ace33..6ae099114e 100644
> --- a/tests/qemu-iotests/testenv.py
> +++ b/tests/qemu-iotests/testenv.py
> @@ -73,7 +73,7 @@ class TestEnv(ContextManager['TestEnv']):
>                        'AIOMODE', 'CACHEMODE', 'VALGRIND_QEMU',
>                        'CACHEMODE_IS_DEFAULT', 'IMGFMT_GENERIC', 'IMGOPTSSYNTAX',
>                        'IMGKEYSECRET', 'QEMU_DEFAULT_MACHINE', 'MALLOC_PERTURB_',
> -                     'GDB_QEMU']
> +                     'GDB_QEMU', 'PRINT_QEMU']
>   
>       def get_env(self) -> Dict[str, str]:
>           env = {}
> @@ -165,7 +165,8 @@ def __init__(self, imgfmt: str, imgproto: str, aiomode: str,
>                    misalign: bool = False,
>                    debug: bool = False,
>                    valgrind: bool = False,
> -                 gdb: bool = False) -> None:
> +                 gdb: bool = False,
> +                 qprint: bool = False) -> None:
>           self.imgfmt = imgfmt
>           self.imgproto = imgproto
>           self.aiomode = aiomode
> @@ -173,6 +174,9 @@ def __init__(self, imgfmt: str, imgproto: str, aiomode: str,
>           self.misalign = misalign
>           self.debug = debug
>   
> +        if qprint:
> +            self.print_qemu = 'y'
> +
>           if gdb:
>               self.gdb_qemu = os.environ.get('GDB_QEMU', 'localhost:12345')
>           elif 'GDB_QEMU' in os.environ:
> @@ -278,6 +282,7 @@ def print_env(self) -> None:
>   SOCKET_SCM_HELPER -- {SOCKET_SCM_HELPER}
>   GDB_QEMU      -- "{GDB_QEMU}"
>   VALGRIND_QEMU     -- "{VALGRIND_QEMU}"
> +PRINT_QEMU    --  "{PRINT_QEMU}"

Again, I personally don’t like the quotes.

(I think “PRINT_QEMU -- y” and “PRINT_QEMU --” look better.)

Another thing: Here in this place, the name of the environment variable 
is visible.  “PRINT_QEMU” doesn’t really have meaning; now, if it 
weren’t visible, I wouldn’t care, but here it is visible.  Perhaps 
“PRINT_QEMU_OUTPUT”, or "SHOW_QEMU_OUTPUT" would mean more?

Well, it’s all bike-shedding, so with “is not None” and the add_argument 
line broken so it doesn’t exceed 79 characters:

Reviewed-by: Max Reitz <mreitz@redhat.com>

>   """
>   
>           args = collections.defaultdict(str, self.get_env())
> 

Re: [PATCH v3 14/15] qemu_iotests: add option to show qemu binary logs on stdout

[Emanuele Giuseppe Esposito]

On 30/04/2021 15:50, Max Reitz wrote:
> On 14.04.21 19:03, Emanuele Giuseppe Esposito wrote:
>> Using the flag -p, allow the qemu binary to print to stdout.
>> This helps especially when doing print-debugging.
> 
> I think this shouldn’t refer to prints but to qemu’s stdout/stderr in 
> general, i.e....
> 
>> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
>> ---
>>   tests/qemu-iotests/check      | 3 ++-
>>   tests/qemu-iotests/iotests.py | 9 +++++++++
>>   tests/qemu-iotests/testenv.py | 9 +++++++--
>>   3 files changed, 18 insertions(+), 3 deletions(-)
>>
>> diff --git a/tests/qemu-iotests/check b/tests/qemu-iotests/check
>> index 489178d9a4..84483922eb 100755
>> --- a/tests/qemu-iotests/check
>> +++ b/tests/qemu-iotests/check
>> @@ -33,6 +33,7 @@ def make_argparser() -> argparse.ArgumentParser:
>>                      help='pretty print output for make check')
>>       p.add_argument('-d', dest='debug', action='store_true', 
>> help='debug')
>> +    p.add_argument('-p', dest='print', action='store_true', 
>> help='shows qemu binary prints to stdout')
> 
> I’d prefer for the description to be “redirects qemu's stdout and stderr 
> to the test output”.
> 
> Also, this line is too long.
> 
>>       p.add_argument('-gdb', action='store_true',
>>                      help="start gdbserver with $GDB_QEMU options. \
>>                            Default is localhost:12345")
>> @@ -117,7 +118,7 @@ if __name__ == '__main__':
>>                     aiomode=args.aiomode, cachemode=args.cachemode,
>>                     imgopts=args.imgopts, misalign=args.misalign,
>>                     debug=args.debug, valgrind=args.valgrind,
>> -                  gdb=args.gdb)
>> +                  gdb=args.gdb, qprint=args.print)
>>       testfinder = TestFinder(test_dir=env.source_iotests)
>> diff --git a/tests/qemu-iotests/iotests.py 
>> b/tests/qemu-iotests/iotests.py
>> index f9832558a0..52ff7332f8 100644
>> --- a/tests/qemu-iotests/iotests.py
>> +++ b/tests/qemu-iotests/iotests.py
>> @@ -79,6 +79,8 @@
>>   if os.environ.get('GDB_QEMU'):
>>       qemu_gdb = ['gdbserver'] + 
>> os.environ.get('GDB_QEMU').strip().split(' ')
>> +qemu_print = os.environ.get('PRINT_QEMU', False)
>> +
>>   imgfmt = os.environ.get('IMGFMT', 'raw')
>>   imgproto = os.environ.get('IMGPROTO', 'file')
>>   output_dir = os.environ.get('OUTPUT_DIR', '.')
>> @@ -621,6 +623,13 @@ def _post_shutdown(self) -> None:
>>           super()._post_shutdown()
>>           self.subprocess_check_valgrind(qemu_valgrind)
>> +    def _pre_launch(self) -> None:
>> +        super()._pre_launch()
>> +        if qemu_print and self._qemu_log_file != None:
> 
> I think "is not None" is mostly used in Python.  (I’m saying this in 
> this weird way because I’m not the one to ask what’s mostly used in 
> Python...)
> 
> (That also silences mypy, which otherwise complains and then fails 297.)
> 
>> +            # set QEMU binary output to stdout
>> +            self._qemu_log_file.close()
>> +            self._qemu_log_file = None
> 
> I don’t know enough Python to know how this works.  I suppose this does 
> access the super class’s member?  (I could also imagine it creates a new 
> member, just for this child class, but that then effectively overwrites 
> the super class’s member.)
> 
> Considering _qemu_log_file is apparently meant to be a private attribute 
> (from the leading underscore), I think it would be better to have a 
> function provided by the super class for this.

It should access the superclass member, and it's the same that 
_post_shutdown does. I can make a function for that.

Regarding all other feedback in all patches that I did not answer: agree 
on all of them, will adjust everything in v4.

Thank you,
Emanuele

> 
>> +
>>       def add_object(self, opts):
>>           self._args.append('-object')
>>           self._args.append(opts)
>> diff --git a/tests/qemu-iotests/testenv.py 
>> b/tests/qemu-iotests/testenv.py
>> index 39ae7ace33..6ae099114e 100644
>> --- a/tests/qemu-iotests/testenv.py
>> +++ b/tests/qemu-iotests/testenv.py
>> @@ -73,7 +73,7 @@ class TestEnv(ContextManager['TestEnv']):
>>                        'AIOMODE', 'CACHEMODE', 'VALGRIND_QEMU',
>>                        'CACHEMODE_IS_DEFAULT', 'IMGFMT_GENERIC', 
>> 'IMGOPTSSYNTAX',
>>                        'IMGKEYSECRET', 'QEMU_DEFAULT_MACHINE', 
>> 'MALLOC_PERTURB_',
>> -                     'GDB_QEMU']
>> +                     'GDB_QEMU', 'PRINT_QEMU']
>>       def get_env(self) -> Dict[str, str]:
>>           env = {}
>> @@ -165,7 +165,8 @@ def __init__(self, imgfmt: str, imgproto: str, 
>> aiomode: str,
>>                    misalign: bool = False,
>>                    debug: bool = False,
>>                    valgrind: bool = False,
>> -                 gdb: bool = False) -> None:
>> +                 gdb: bool = False,
>> +                 qprint: bool = False) -> None:
>>           self.imgfmt = imgfmt
>>           self.imgproto = imgproto
>>           self.aiomode = aiomode
>> @@ -173,6 +174,9 @@ def __init__(self, imgfmt: str, imgproto: str, 
>> aiomode: str,
>>           self.misalign = misalign
>>           self.debug = debug
>> +        if qprint:
>> +            self.print_qemu = 'y'
>> +
>>           if gdb:
>>               self.gdb_qemu = os.environ.get('GDB_QEMU', 
>> 'localhost:12345')
>>           elif 'GDB_QEMU' in os.environ:
>> @@ -278,6 +282,7 @@ def print_env(self) -> None:
>>   SOCKET_SCM_HELPER -- {SOCKET_SCM_HELPER}
>>   GDB_QEMU      -- "{GDB_QEMU}"
>>   VALGRIND_QEMU     -- "{VALGRIND_QEMU}"
>> +PRINT_QEMU    --  "{PRINT_QEMU}"
> 
> Again, I personally don’t like the quotes.
> 
> (I think “PRINT_QEMU -- y” and “PRINT_QEMU --” look better.)
> 
> Another thing: Here in this place, the name of the environment variable 
> is visible.  “PRINT_QEMU” doesn’t really have meaning; now, if it 
> weren’t visible, I wouldn’t care, but here it is visible.  Perhaps 
> “PRINT_QEMU_OUTPUT”, or "SHOW_QEMU_OUTPUT" would mean more?
> 
> Well, it’s all bike-shedding, so with “is not None” and the add_argument 
> line broken so it doesn’t exceed 79 characters:
> 
> Reviewed-by: Max Reitz <mreitz@redhat.com>
> 
>>   """
>>           args = collections.defaultdict(str, self.get_env())
>>
> 

Re: [PATCH v3 14/15] qemu_iotests: add option to show qemu binary logs on stdout

[Max Reitz]

On 30.04.21 23:04, Emanuele Giuseppe Esposito wrote:
> 
> 
> On 30/04/2021 15:50, Max Reitz wrote:
>> On 14.04.21 19:03, Emanuele Giuseppe Esposito wrote:
>>> Using the flag -p, allow the qemu binary to print to stdout.
>>> This helps especially when doing print-debugging.
>>
>> I think this shouldn’t refer to prints but to qemu’s stdout/stderr in 
>> general, i.e....
>>
>>> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
>>> ---
>>>   tests/qemu-iotests/check      | 3 ++-
>>>   tests/qemu-iotests/iotests.py | 9 +++++++++
>>>   tests/qemu-iotests/testenv.py | 9 +++++++--
>>>   3 files changed, 18 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/tests/qemu-iotests/check b/tests/qemu-iotests/check
>>> index 489178d9a4..84483922eb 100755
>>> --- a/tests/qemu-iotests/check
>>> +++ b/tests/qemu-iotests/check
>>> @@ -33,6 +33,7 @@ def make_argparser() -> argparse.ArgumentParser:
>>>                      help='pretty print output for make check')
>>>       p.add_argument('-d', dest='debug', action='store_true', 
>>> help='debug')
>>> +    p.add_argument('-p', dest='print', action='store_true', 
>>> help='shows qemu binary prints to stdout')
>>
>> I’d prefer for the description to be “redirects qemu's stdout and 
>> stderr to the test output”.
>>
>> Also, this line is too long.
>>
>>>       p.add_argument('-gdb', action='store_true',
>>>                      help="start gdbserver with $GDB_QEMU options. \
>>>                            Default is localhost:12345")
>>> @@ -117,7 +118,7 @@ if __name__ == '__main__':
>>>                     aiomode=args.aiomode, cachemode=args.cachemode,
>>>                     imgopts=args.imgopts, misalign=args.misalign,
>>>                     debug=args.debug, valgrind=args.valgrind,
>>> -                  gdb=args.gdb)
>>> +                  gdb=args.gdb, qprint=args.print)
>>>       testfinder = TestFinder(test_dir=env.source_iotests)
>>> diff --git a/tests/qemu-iotests/iotests.py 
>>> b/tests/qemu-iotests/iotests.py
>>> index f9832558a0..52ff7332f8 100644
>>> --- a/tests/qemu-iotests/iotests.py
>>> +++ b/tests/qemu-iotests/iotests.py
>>> @@ -79,6 +79,8 @@
>>>   if os.environ.get('GDB_QEMU'):
>>>       qemu_gdb = ['gdbserver'] + 
>>> os.environ.get('GDB_QEMU').strip().split(' ')
>>> +qemu_print = os.environ.get('PRINT_QEMU', False)
>>> +
>>>   imgfmt = os.environ.get('IMGFMT', 'raw')
>>>   imgproto = os.environ.get('IMGPROTO', 'file')
>>>   output_dir = os.environ.get('OUTPUT_DIR', '.')
>>> @@ -621,6 +623,13 @@ def _post_shutdown(self) -> None:
>>>           super()._post_shutdown()
>>>           self.subprocess_check_valgrind(qemu_valgrind)
>>> +    def _pre_launch(self) -> None:
>>> +        super()._pre_launch()
>>> +        if qemu_print and self._qemu_log_file != None:
>>
>> I think "is not None" is mostly used in Python.  (I’m saying this in 
>> this weird way because I’m not the one to ask what’s mostly used in 
>> Python...)
>>
>> (That also silences mypy, which otherwise complains and then fails 297.)
>>
>>> +            # set QEMU binary output to stdout
>>> +            self._qemu_log_file.close()
>>> +            self._qemu_log_file = None
>>
>> I don’t know enough Python to know how this works.  I suppose this 
>> does access the super class’s member?  (I could also imagine it 
>> creates a new member, just for this child class, but that then 
>> effectively overwrites the super class’s member.)
>>
>> Considering _qemu_log_file is apparently meant to be a private 
>> attribute (from the leading underscore), I think it would be better to 
>> have a function provided by the super class for this.
> 
> It should access the superclass member, and it's the same that 
> _post_shutdown does. I can make a function for that.

Understood.

> Regarding all other feedback in all patches that I did not answer: agree 
> on all of them, will adjust everything in v4.

Thanks, looking forward to it! :)

Max

[PATCH v3 15/15] docs/devel/testing: add -p option to the debug section of QEMU iotests

[Emanuele Giuseppe Esposito]

Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
---
 docs/devel/testing.rst | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/docs/devel/testing.rst b/docs/devel/testing.rst
index 62902cfd2d..0c18fc4571 100644
--- a/docs/devel/testing.rst
+++ b/docs/devel/testing.rst
@@ -246,6 +246,10 @@ given as options to the ``check`` script:
 * ``-d`` (debug) just increases the logging verbosity, showing
   for example the QMP commands and answers.
 
+* ``-p`` (print) allows QEMU binary stdout to be shown in the
+  test console, instead of saving it into a log file in
+  ``$TEST_DIR/qemu-machine-<random_string>``.
+
 Test case groups
 ----------------
 
-- 
2.30.2

Re: [PATCH v3 15/15] docs/devel/testing: add -p option to the debug section of QEMU iotests

[Max Reitz]

On 14.04.21 19:03, Emanuele Giuseppe Esposito wrote:
> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> ---
>   docs/devel/testing.rst | 4 ++++
>   1 file changed, 4 insertions(+)
> 
> diff --git a/docs/devel/testing.rst b/docs/devel/testing.rst
> index 62902cfd2d..0c18fc4571 100644
> --- a/docs/devel/testing.rst
> +++ b/docs/devel/testing.rst
> @@ -246,6 +246,10 @@ given as options to the ``check`` script:
>   * ``-d`` (debug) just increases the logging verbosity, showing
>     for example the QMP commands and answers.
>   
> +* ``-p`` (print) allows QEMU binary stdout to be shown in the

stderr, too.

> +  test console, instead of saving it into a log file in
> +  ``$TEST_DIR/qemu-machine-<random_string>``.
> +

It doesn’t allow this, though, it forces it.  That means that tests that 
use the log will fail (e.g. 245)[1].  That is, I’d drop the “allows” and 
just state “redirect QEMU’s stdout and stderr to the test output, 
instead of...”.

[1] I realize that all tests will technically fail with -p, because the 
reference output differs, but 245 will not just fail because of that 
difference, but because two of its cases actually really fail.
No need to make a note of this, though.  It’s just that “allows” sounds 
a bit like qemu could choose to put some info into the test output and 
some into the log, when really it’s all or nothing (-p or not -p).

Max

>   Test case groups
>   ----------------
>   
>