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

[PATCH v4 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,5,10 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>
---
v4:
* Rename environment variable from GDB_QEMU to GDB_OPTIONS
* This time test 297 (pylint) passes [Max]
* Refactor the qmp_timer field in machine.py, and add a new 
  parameter in machine.py and subclasses constructor [John]
* Add additional check in patch 4 to cover the case where
  GDB_OPTIONS is empty

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        | 30 +++++++++++++++++++
 python/qemu/machine.py        |  7 +++--
 python/qemu/qtest.py          |  9 ++++--
 tests/qemu-iotests/check      | 15 +++++++---
 tests/qemu-iotests/common.rc  |  8 ++++-
 tests/qemu-iotests/iotests.py | 56 +++++++++++++++++++++++++++++++----
 tests/qemu-iotests/testenv.py | 25 ++++++++++++++--
 7 files changed, 132 insertions(+), 18 deletions(-)

-- 
2.30.2

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

[Emanuele Giuseppe Esposito]

Alsp add a new _qmp_timer field to the QEMUMachine class.

Let's change the default socket timeout to None, so that if
a subclass needs to add a timer, it can be done by modifying
this private field.

At the same time, restore the timer to be 15 seconds in iotests.py, to
give an upper bound to qemu-iotests execution.

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

diff --git a/python/qemu/machine.py b/python/qemu/machine.py
index 6e44bda337..df32de4377 100644
--- a/python/qemu/machine.py
+++ b/python/qemu/machine.py
@@ -89,7 +89,8 @@ def __init__(self,
                  socket_scm_helper: Optional[str] = None,
                  sock_dir: Optional[str] = None,
                  drain_console: bool = False,
-                 console_log: Optional[str] = None):
+                 console_log: Optional[str] = None,
+                 qmp_timer: Optional[float] = None):
         '''
         Initialize a QEMUMachine
 
@@ -103,6 +104,7 @@ def __init__(self,
         @param sock_dir: where to create socket (overrides test_dir for sock)
         @param drain_console: (optional) True to drain console socket to buffer
         @param console_log: (optional) path to console log file
+        @param qmp_timer: (optional) default QMP socket timeout
         @note: Qemu process is not started until launch() is used.
         '''
         # Direct user configuration
@@ -110,6 +112,7 @@ def __init__(self,
         self._binary = binary
         self._args = list(args)
         self._wrapper = wrapper
+        self._qmp_timer = qmp_timer
 
         self._name = name or "qemu-%d" % os.getpid()
         self._test_dir = test_dir
@@ -323,7 +326,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:
         """
diff --git a/python/qemu/qtest.py b/python/qemu/qtest.py
index 39a0cf62fe..afea210d9d 100644
--- a/python/qemu/qtest.py
+++ b/python/qemu/qtest.py
@@ -114,14 +114,15 @@ def __init__(self,
                  name: Optional[str] = None,
                  test_dir: str = "/var/tmp",
                  socket_scm_helper: Optional[str] = None,
-                 sock_dir: Optional[str] = None):
+                 sock_dir: Optional[str] = None,
+                 qmp_timer: Optional[float] = None):
         if name is None:
             name = "qemu-%d" % os.getpid()
         if sock_dir is None:
             sock_dir = test_dir
         super().__init__(binary, args, name=name, test_dir=test_dir,
                          socket_scm_helper=socket_scm_helper,
-                         sock_dir=sock_dir)
+                         sock_dir=sock_dir, qmp_timer=qmp_timer)
         self._qtest: Optional[QEMUQtestProtocol] = None
         self._qtest_path = os.path.join(sock_dir, name + "-qtest.sock")
 
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index ec3c69daf1..5d78de0f0b 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -571,10 +571,11 @@ class VM(qtest.QEMUQtestMachine):
 
     def __init__(self, path_suffix=''):
         name = "qemu%s-%d" % (path_suffix, os.getpid())
+        timer = 15.0
         super().__init__(qemu_prog, qemu_opts, name=name,
                          test_dir=test_dir,
                          socket_scm_helper=socket_scm_helper,
-                         sock_dir=sock_dir)
+                         sock_dir=sock_dir, qmp_timer=timer)
         self._num_drives = 0
 
     def add_object(self, opts):
-- 
2.30.2

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

[Vladimir Sementsov-Ogievskiy]

20.05.2021 10:52, Emanuele Giuseppe Esposito wrote:
> Alsp add a new _qmp_timer field to the QEMUMachine class.
> 
> Let's change the default socket timeout to None, so that if
> a subclass needs to add a timer, it can be done by modifying
> this private field.
> 
> At the same time, restore the timer to be 15 seconds in iotests.py, to
> give an upper bound to qemu-iotests execution.

Hmm. Not to qemu-iotests execution, but only to connection to qmp monitor, isn't it?

> 
> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> ---
>   python/qemu/machine.py        | 7 +++++--
>   python/qemu/qtest.py          | 5 +++--
>   tests/qemu-iotests/iotests.py | 3 ++-
>   3 files changed, 10 insertions(+), 5 deletions(-)
> 
> diff --git a/python/qemu/machine.py b/python/qemu/machine.py
> index 6e44bda337..df32de4377 100644
> --- a/python/qemu/machine.py
> +++ b/python/qemu/machine.py
> @@ -89,7 +89,8 @@ def __init__(self,
>                    socket_scm_helper: Optional[str] = None,
>                    sock_dir: Optional[str] = None,
>                    drain_console: bool = False,
> -                 console_log: Optional[str] = None):
> +                 console_log: Optional[str] = None,
> +                 qmp_timer: Optional[float] = None):
>           '''
>           Initialize a QEMUMachine
>   
> @@ -103,6 +104,7 @@ def __init__(self,
>           @param sock_dir: where to create socket (overrides test_dir for sock)
>           @param drain_console: (optional) True to drain console socket to buffer
>           @param console_log: (optional) path to console log file
> +        @param qmp_timer: (optional) default QMP socket timeout
>           @note: Qemu process is not started until launch() is used.
>           '''
>           # Direct user configuration
> @@ -110,6 +112,7 @@ def __init__(self,
>           self._binary = binary
>           self._args = list(args)
>           self._wrapper = wrapper
> +        self._qmp_timer = qmp_timer
>   
>           self._name = name or "qemu-%d" % os.getpid()
>           self._test_dir = test_dir
> @@ -323,7 +326,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:
>           """
> diff --git a/python/qemu/qtest.py b/python/qemu/qtest.py
> index 39a0cf62fe..afea210d9d 100644
> --- a/python/qemu/qtest.py
> +++ b/python/qemu/qtest.py
> @@ -114,14 +114,15 @@ def __init__(self,
>                    name: Optional[str] = None,
>                    test_dir: str = "/var/tmp",
>                    socket_scm_helper: Optional[str] = None,
> -                 sock_dir: Optional[str] = None):
> +                 sock_dir: Optional[str] = None,
> +                 qmp_timer: Optional[float] = None):
>           if name is None:
>               name = "qemu-%d" % os.getpid()
>           if sock_dir is None:
>               sock_dir = test_dir
>           super().__init__(binary, args, name=name, test_dir=test_dir,
>                            socket_scm_helper=socket_scm_helper,
> -                         sock_dir=sock_dir)
> +                         sock_dir=sock_dir, qmp_timer=qmp_timer)
>           self._qtest: Optional[QEMUQtestProtocol] = None
>           self._qtest_path = os.path.join(sock_dir, name + "-qtest.sock")
>   
> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
> index ec3c69daf1..5d78de0f0b 100644
> --- a/tests/qemu-iotests/iotests.py
> +++ b/tests/qemu-iotests/iotests.py
> @@ -571,10 +571,11 @@ class VM(qtest.QEMUQtestMachine):
>   
>       def __init__(self, path_suffix=''):
>           name = "qemu%s-%d" % (path_suffix, os.getpid())
> +        timer = 15.0
>           super().__init__(qemu_prog, qemu_opts, name=name,
>                            test_dir=test_dir,
>                            socket_scm_helper=socket_scm_helper,
> -                         sock_dir=sock_dir)
> +                         sock_dir=sock_dir, qmp_timer=timer)
>           self._num_drives = 0
>   
>       def add_object(self, opts):
> 

I'd call it qmp_accept_timeout to be precise. Anyway:

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>


-- 
Best regards,
Vladimir

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

[John Snow]

On 5/20/21 3:52 AM, Emanuele Giuseppe Esposito wrote:
> Alsp add a new _qmp_timer field to the QEMUMachine class.
> 
> Let's change the default socket timeout to None, so that if
> a subclass needs to add a timer, it can be done by modifying
> this private field.
> 
> At the same time, restore the timer to be 15 seconds in iotests.py, to
> give an upper bound to qemu-iotests execution.
> 
> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>

Hi Emanuele: I'm sorry, but with the recent Python PR this no longer 
applies to origin/master -- the python files got shuffled around a bit 
when I added the new CI tests.

May I please ask you to rebase? You don't have to re-spin just yet, just 
pointing me to the rebase would help me out.

(Also: if you push to gitlab, you can take advantage of the new Python 
CI tests!)

Apologies,
--js

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

[Emanuele Giuseppe Esposito]

On 03/06/2021 01:23, John Snow wrote:
> On 5/20/21 3:52 AM, Emanuele Giuseppe Esposito wrote:
>> Alsp add a new _qmp_timer field to the QEMUMachine class.
>>
>> Let's change the default socket timeout to None, so that if
>> a subclass needs to add a timer, it can be done by modifying
>> this private field.
>>
>> At the same time, restore the timer to be 15 seconds in iotests.py, to
>> give an upper bound to qemu-iotests execution.
>>
>> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> 
> Hi Emanuele: I'm sorry, but with the recent Python PR this no longer 
> applies to origin/master -- the python files got shuffled around a bit 
> when I added the new CI tests.
> 
> May I please ask you to rebase? You don't have to re-spin just yet, just 
> pointing me to the rebase would help me out.

Hi John, no problem. I rebased here:
https://gitlab.com/eesposit/qemu/-/commits/qemu_iotests_io

Let me know if I need to do anything else.
I will re-spin later today.

Thank you,
Emanuele

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

[John Snow]

On 6/3/21 4:06 AM, Emanuele Giuseppe Esposito wrote:
> 
> 
> On 03/06/2021 01:23, John Snow wrote:
>> On 5/20/21 3:52 AM, Emanuele Giuseppe Esposito wrote:
>>> Alsp add a new _qmp_timer field to the QEMUMachine class.
>>>
>>> Let's change the default socket timeout to None, so that if
>>> a subclass needs to add a timer, it can be done by modifying
>>> this private field.
>>>
>>> At the same time, restore the timer to be 15 seconds in iotests.py, to
>>> give an upper bound to qemu-iotests execution.
>>>
>>> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
>>
>> Hi Emanuele: I'm sorry, but with the recent Python PR this no longer 
>> applies to origin/master -- the python files got shuffled around a bit 
>> when I added the new CI tests.
>>
>> May I please ask you to rebase? You don't have to re-spin just yet, 
>> just pointing me to the rebase would help me out.
> 
> Hi John, no problem. I rebased here:
> https://gitlab.com/eesposit/qemu/-/commits/qemu_iotests_io
> 
> Let me know if I need to do anything else.
> I will re-spin later today.
> 
> Thank you,
> Emanuele
> 

Hi Emanuele:

https://gitlab.com/jsnow/qemu/-/commits/review

I added in a pylint ignore for you and these patches are clean now.

--js

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

[Emanuele Giuseppe Esposito]

On 03/06/2021 21:02, John Snow wrote:
> On 6/3/21 4:06 AM, Emanuele Giuseppe Esposito wrote:
>>
>>
>> On 03/06/2021 01:23, John Snow wrote:
>>> On 5/20/21 3:52 AM, Emanuele Giuseppe Esposito wrote:
>>>> Alsp add a new _qmp_timer field to the QEMUMachine class.
>>>>
>>>> Let's change the default socket timeout to None, so that if
>>>> a subclass needs to add a timer, it can be done by modifying
>>>> this private field.
>>>>
>>>> At the same time, restore the timer to be 15 seconds in iotests.py, to
>>>> give an upper bound to qemu-iotests execution.
>>>>
>>>> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
>>>
>>> Hi Emanuele: I'm sorry, but with the recent Python PR this no longer 
>>> applies to origin/master -- the python files got shuffled around a 
>>> bit when I added the new CI tests.
>>>
>>> May I please ask you to rebase? You don't have to re-spin just yet, 
>>> just pointing me to the rebase would help me out.
>>
>> Hi John, no problem. I rebased here:
>> https://gitlab.com/eesposit/qemu/-/commits/qemu_iotests_io
>>
>> Let me know if I need to do anything else.
>> I will re-spin later today.
>>
>> Thank you,
>> Emanuele
>>
> 
> Hi Emanuele:
> 
> https://gitlab.com/jsnow/qemu/-/commits/review
> 
> I added in a pylint ignore for you and these patches are clean now.

Thank you! I will add it to my series.

Emanuele

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

[Emanuele Giuseppe Esposito]

Reviewed-by: John Snow <jsnow@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
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 afea210d9d..e6a8fb5984 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,
@@ -120,7 +121,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, qmp_timer=qmp_timer)
         self._qtest: Optional[QEMUQtestProtocol] = None
-- 
2.30.2

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

[Vladimir Sementsov-Ogievskiy]

in subject: s/QEMUQtestmachine/QEMUQtestMachine

20.05.2021 10:52, Emanuele Giuseppe Esposito wrote:
> Reviewed-by: John Snow <jsnow@redhat.com>
> Reviewed-by: Max Reitz <mreitz@redhat.com>
> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>

Usually, r-b lines are placed below s-o-b line.

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.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 afea210d9d..e6a8fb5984 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,
> @@ -120,7 +121,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, qmp_timer=qmp_timer)
>           self._qtest: Optional[QEMUQtestProtocol] = None
> 


-- 
Best regards,
Vladimir

[PATCH v4 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 1da4c4e4c4..8144e316a4 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
+-----------------------
+The following options to the ``check`` script can be useful when debugging
+a failing test:
+
+* ``-d`` (debug) just increases the logging verbosity, showing
+  for example the QMP commands and answers.
+
 Test case groups
 ----------------
 
-- 
2.30.2

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

[Vladimir Sementsov-Ogievskiy]

20.05.2021 10:52, 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>

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

-- 
Best regards,
Vladimir

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

[Emanuele Giuseppe Esposito]

Define -gdb flag and GDB_OPTIONS environment variable
to python tests to attach a gdbserver to each qemu instance.
This patch only adds and parses this flag, it does not yet add
the implementation for it.

if -gdb is not provided but $GDB_OPTIONS is set, ignore the
environment variable.

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

diff --git a/tests/qemu-iotests/check b/tests/qemu-iotests/check
index d1c87ceaf1..b9820fdaaf 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_OPTIONS options \
+                        ('localhost:12345' if $GDB_OPTIONS is empty)")
     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 5d78de0f0b..d667fde6f8 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -75,6 +75,11 @@
 qemu_prog = os.environ.get('QEMU_PROG', 'qemu')
 qemu_opts = os.environ.get('QEMU_OPTIONS', '').strip().split(' ')
 
+gdb_qemu_env = os.environ.get('GDB_OPTIONS')
+qemu_gdb = []
+if gdb_qemu_env:
+    qemu_gdb = ['gdbserver'] + gdb_qemu_env.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 6d27712617..49ddd586ef 100644
--- a/tests/qemu-iotests/testenv.py
+++ b/tests/qemu-iotests/testenv.py
@@ -27,6 +27,7 @@
 import glob
 from typing import Dict, Any, Optional, ContextManager
 
+DEF_GDB_OPTIONS = 'localhost:12345'
 
 def isxfile(path: str) -> bool:
     return os.path.isfile(path) and os.access(path, os.X_OK)
@@ -72,7 +73,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_OPTIONS']
 
     def get_env(self) -> Dict[str, str]:
         env = {}
@@ -163,7 +165,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 +174,14 @@ def __init__(self, imgfmt: str, imgproto: str, aiomode: str,
         self.misalign = misalign
         self.debug = debug
 
+        if gdb:
+            self.gdb_options = os.environ.get('GDB_OPTIONS', DEF_GDB_OPTIONS)
+            if not self.gdb_options:
+                # cover the case 'export GDB_OPTIONS='
+                self.gdb_options = DEF_GDB_OPTIONS
+        elif 'GDB_OPTIONS' in os.environ:
+            del os.environ['GDB_OPTIONS']
+
         if valgrind:
             self.valgrind_qemu = 'y'
 
@@ -269,7 +280,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_OPTIONS   -- {GDB_OPTIONS}
+"""
 
         args = collections.defaultdict(str, self.get_env())
 
-- 
2.30.2

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

[Vladimir Sementsov-Ogievskiy]

20.05.2021 10:52, Emanuele Giuseppe Esposito wrote:
> Define -gdb flag and GDB_OPTIONS environment variable

Let's use --option notation for new long options

> to python tests to attach a gdbserver to each qemu instance.
> This patch only adds and parses this flag, it does not yet add
> the implementation for it.
> 
> if -gdb is not provided but $GDB_OPTIONS is set, ignore the
> environment variable.
> 
> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> ---
>   tests/qemu-iotests/check      |  6 +++++-
>   tests/qemu-iotests/iotests.py |  5 +++++
>   tests/qemu-iotests/testenv.py | 19 ++++++++++++++++---
>   3 files changed, 26 insertions(+), 4 deletions(-)
> 
> diff --git a/tests/qemu-iotests/check b/tests/qemu-iotests/check
> index d1c87ceaf1..b9820fdaaf 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_OPTIONS options \
> +                        ('localhost:12345' if $GDB_OPTIONS is empty)")

Hmm.. Why not just make --gdb a string option, that defaults to GDB_OPTIONS? This way it will more similar with other options.

>       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 5d78de0f0b..d667fde6f8 100644
> --- a/tests/qemu-iotests/iotests.py
> +++ b/tests/qemu-iotests/iotests.py
> @@ -75,6 +75,11 @@
>   qemu_prog = os.environ.get('QEMU_PROG', 'qemu')
>   qemu_opts = os.environ.get('QEMU_OPTIONS', '').strip().split(' ')
>   
> +gdb_qemu_env = os.environ.get('GDB_OPTIONS')
> +qemu_gdb = []
> +if gdb_qemu_env:
> +    qemu_gdb = ['gdbserver'] + gdb_qemu_env.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 6d27712617..49ddd586ef 100644
> --- a/tests/qemu-iotests/testenv.py
> +++ b/tests/qemu-iotests/testenv.py
> @@ -27,6 +27,7 @@
>   import glob
>   from typing import Dict, Any, Optional, ContextManager
>   
> +DEF_GDB_OPTIONS = 'localhost:12345'
>   
>   def isxfile(path: str) -> bool:
>       return os.path.isfile(path) and os.access(path, os.X_OK)
> @@ -72,7 +73,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_OPTIONS']
>   
>       def get_env(self) -> Dict[str, str]:
>           env = {}
> @@ -163,7 +165,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 +174,14 @@ def __init__(self, imgfmt: str, imgproto: str, aiomode: str,
>           self.misalign = misalign
>           self.debug = debug
>   
> +        if gdb:
> +            self.gdb_options = os.environ.get('GDB_OPTIONS', DEF_GDB_OPTIONS)
> +            if not self.gdb_options:
> +                # cover the case 'export GDB_OPTIONS='
> +                self.gdb_options = DEF_GDB_OPTIONS
> +        elif 'GDB_OPTIONS' in os.environ:
> +            del os.environ['GDB_OPTIONS']
> +
>           if valgrind:
>               self.valgrind_qemu = 'y'
>   
> @@ -269,7 +280,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_OPTIONS   -- {GDB_OPTIONS}
> +"""
>   
>           args = collections.defaultdict(str, self.get_env())
>   
> 


-- 
Best regards,
Vladimir

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

[Paolo Bonzini]

On 26/05/21 13:24, Vladimir Sementsov-Ogievskiy wrote:
> 
>> Define -gdb flag and GDB_OPTIONS environment variable
> 
> Let's use --option notation for new long options

Why make a mix of two styles? -- suggests that single-character options 
like -d and -v can be combined, is that the case?

>> if -gdb is not provided but $GDB_OPTIONS is set, ignore the
>> environment variable.
>>
>> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
>> ---
>>   tests/qemu-iotests/check      |  6 +++++-
>>   tests/qemu-iotests/iotests.py |  5 +++++
>>   tests/qemu-iotests/testenv.py | 19 ++++++++++++++++---
>>   3 files changed, 26 insertions(+), 4 deletions(-)
>>
>> diff --git a/tests/qemu-iotests/check b/tests/qemu-iotests/check
>> index d1c87ceaf1..b9820fdaaf 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_OPTIONS options \
>> +                        ('localhost:12345' if $GDB_OPTIONS is empty)")
> 
> Hmm.. Why not just make --gdb a string option, that defaults to 
> GDB_OPTIONS? This way it will more similar with other options.

I think then something like "./check -gdb 030" would not work, right?

Paolo

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

[Vladimir Sementsov-Ogievskiy]

26.05.2021 15:48, Paolo Bonzini wrote:
> On 26/05/21 13:24, Vladimir Sementsov-Ogievskiy wrote:
>>
>>> Define -gdb flag and GDB_OPTIONS environment variable
>>
>> Let's use --option notation for new long options
> 
> Why make a mix of two styles? -- suggests that single-character options like -d and -v can be combined, is that the case?

Yes.. I think think that --options (with -o short options) is more usual and modern style.

We already have both --options and -options.. So, my idea when I was rewriting ./check was that better to move to --options. I can send patch to change all existing -options of check to be --options for full consistency. It would be some pain for developers..

> 
>>> if -gdb is not provided but $GDB_OPTIONS is set, ignore the
>>> environment variable.
>>>
>>> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
>>> ---
>>>   tests/qemu-iotests/check      |  6 +++++-
>>>   tests/qemu-iotests/iotests.py |  5 +++++
>>>   tests/qemu-iotests/testenv.py | 19 ++++++++++++++++---
>>>   3 files changed, 26 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/tests/qemu-iotests/check b/tests/qemu-iotests/check
>>> index d1c87ceaf1..b9820fdaaf 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_OPTIONS options \
>>> +                        ('localhost:12345' if $GDB_OPTIONS is empty)")
>>
>> Hmm.. Why not just make --gdb a string option, that defaults to GDB_OPTIONS? This way it will more similar with other options.
> 
> I think then something like "./check -gdb 030" would not work, right?
> 

Hmm, yes, that's not very convenient. OK then, let's keep bool.


-- 
Best regards,
Vladimir

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

[Emanuele Giuseppe Esposito]

On 26/05/2021 15:25, Vladimir Sementsov-Ogievskiy wrote:
> 26.05.2021 15:48, Paolo Bonzini wrote:
>> On 26/05/21 13:24, Vladimir Sementsov-Ogievskiy wrote:
>>>
>>>> Define -gdb flag and GDB_OPTIONS environment variable
>>>
>>> Let's use --option notation for new long options
>>
>> Why make a mix of two styles? -- suggests that single-character 
>> options like -d and -v can be combined, is that the case?
> 
> Yes.. I think think that --options (with -o short options) is more usual 
> and modern style.
> 
> We already have both --options and -options.. So, my idea when I was 
> rewriting ./check was that better to move to --options. I can send patch 
> to change all existing -options of check to be --options for full 
> consistency. It would be some pain for developers..

I am following the current convention. I put gdb on the same 
level/category of valgrind, and since the current option is -valgrind, I 
would like to stick to that. If you want to send a patch changing all 
-options in --options, feel free to do it in a separate series that 
changes also -gdb :)

Thank you,
Emanuele
> 
>>
>>>> if -gdb is not provided but $GDB_OPTIONS is set, ignore the
>>>> environment variable.
>>>>
>>>> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
>>>> ---
>>>>   tests/qemu-iotests/check      |  6 +++++-
>>>>   tests/qemu-iotests/iotests.py |  5 +++++
>>>>   tests/qemu-iotests/testenv.py | 19 ++++++++++++++++---
>>>>   3 files changed, 26 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/tests/qemu-iotests/check b/tests/qemu-iotests/check
>>>> index d1c87ceaf1..b9820fdaaf 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_OPTIONS options \
>>>> +                        ('localhost:12345' if $GDB_OPTIONS is empty)")
>>>
>>> Hmm.. Why not just make --gdb a string option, that defaults to 
>>> GDB_OPTIONS? This way it will more similar with other options.
>>
>> I think then something like "./check -gdb 030" would not work, right?
>>
> 
> Hmm, yes, that's not very convenient. OK then, let's keep bool.
> 
> 

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

[Vladimir Sementsov-Ogievskiy]

20.05.2021 10:52, Emanuele Giuseppe Esposito wrote:
> Define -gdb flag and GDB_OPTIONS environment variable
> to python tests to attach a gdbserver to each qemu instance.
> This patch only adds and parses this flag, it does not yet add
> the implementation for it.
> 
> if -gdb is not provided but $GDB_OPTIONS is set, ignore the
> environment variable.
> 
> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> ---
>   tests/qemu-iotests/check      |  6 +++++-
>   tests/qemu-iotests/iotests.py |  5 +++++
>   tests/qemu-iotests/testenv.py | 19 ++++++++++++++++---
>   3 files changed, 26 insertions(+), 4 deletions(-)
> 
> diff --git a/tests/qemu-iotests/check b/tests/qemu-iotests/check
> index d1c87ceaf1..b9820fdaaf 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_OPTIONS options \
> +                        ('localhost:12345' if $GDB_OPTIONS is empty)")
>       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 5d78de0f0b..d667fde6f8 100644
> --- a/tests/qemu-iotests/iotests.py
> +++ b/tests/qemu-iotests/iotests.py
> @@ -75,6 +75,11 @@
>   qemu_prog = os.environ.get('QEMU_PROG', 'qemu')
>   qemu_opts = os.environ.get('QEMU_OPTIONS', '').strip().split(' ')
>   
> +gdb_qemu_env = os.environ.get('GDB_OPTIONS')

should we specify a default? otherwise get() should raise an exception when GDB_OPTIONS is not set..

or you need os.getenv, which will return None if variable is absent.

> +qemu_gdb = []
> +if gdb_qemu_env:
> +    qemu_gdb = ['gdbserver'] + gdb_qemu_env.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 6d27712617..49ddd586ef 100644
> --- a/tests/qemu-iotests/testenv.py
> +++ b/tests/qemu-iotests/testenv.py
> @@ -27,6 +27,7 @@
>   import glob
>   from typing import Dict, Any, Optional, ContextManager
>   
> +DEF_GDB_OPTIONS = 'localhost:12345'
>   
>   def isxfile(path: str) -> bool:
>       return os.path.isfile(path) and os.access(path, os.X_OK)
> @@ -72,7 +73,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_OPTIONS']
>   
>       def get_env(self) -> Dict[str, str]:
>           env = {}
> @@ -163,7 +165,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 +174,14 @@ def __init__(self, imgfmt: str, imgproto: str, aiomode: str,
>           self.misalign = misalign
>           self.debug = debug
>   
> +        if gdb:
> +            self.gdb_options = os.environ.get('GDB_OPTIONS', DEF_GDB_OPTIONS)

everywhere in the file os.getenv is used, let's be consistent on it.

> +            if not self.gdb_options:
> +                # cover the case 'export GDB_OPTIONS='
> +                self.gdb_options = DEF_GDB_OPTIONS

Hmm, a bit strange to handle 'export X=' only for this new variable, we don't have such logic for other variables.. I'm not against still, if you need it.

> +        elif 'GDB_OPTIONS' in os.environ:
> +            del os.environ['GDB_OPTIONS']

Don't need to remove variable from envirton, it has no effect. The task of TestEnv class is to prepare environment in its attributes, listed in env_variables. Then prepared attributes are available through get_env(). So if you don't want to provide GDB_OPTIONS, it's enough to not initialize self.gdb_options. So, just drop "elif:" branch.

> +
>           if valgrind:
>               self.valgrind_qemu = 'y'
>   
> @@ -269,7 +280,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_OPTIONS   -- {GDB_OPTIONS}

Not sure we need to see this additional line every time.. Maybe, show it only when gdb is set?

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


-- 
Best regards,
Vladimir

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

[Emanuele Giuseppe Esposito]

On 26/05/2021 21:15, Vladimir Sementsov-Ogievskiy wrote:
> 20.05.2021 10:52, Emanuele Giuseppe Esposito wrote:
>> Define -gdb flag and GDB_OPTIONS environment variable
>> to python tests to attach a gdbserver to each qemu instance.
>> This patch only adds and parses this flag, it does not yet add
>> the implementation for it.
>>
>> if -gdb is not provided but $GDB_OPTIONS is set, ignore the
>> environment variable.
>>
>> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
>> ---
>>   tests/qemu-iotests/check      |  6 +++++-
>>   tests/qemu-iotests/iotests.py |  5 +++++
>>   tests/qemu-iotests/testenv.py | 19 ++++++++++++++++---
>>   3 files changed, 26 insertions(+), 4 deletions(-)
>>
>> diff --git a/tests/qemu-iotests/check b/tests/qemu-iotests/check
>> index d1c87ceaf1..b9820fdaaf 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_OPTIONS options \
>> +                        ('localhost:12345' if $GDB_OPTIONS is empty)")
>>       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 5d78de0f0b..d667fde6f8 100644
>> --- a/tests/qemu-iotests/iotests.py
>> +++ b/tests/qemu-iotests/iotests.py
>> @@ -75,6 +75,11 @@
>>   qemu_prog = os.environ.get('QEMU_PROG', 'qemu')
>>   qemu_opts = os.environ.get('QEMU_OPTIONS', '').strip().split(' ')
>> +gdb_qemu_env = os.environ.get('GDB_OPTIONS')
> 
> should we specify a default? otherwise get() should raise an exception 
> when GDB_OPTIONS is not set..
> 
> or you need os.getenv, which will return None if variable is absent.

No, os.environ.get does not raise any exception. I tested it myself, plus:
https://stackoverflow.com/questions/16924471/difference-between-os-getenv-and-os-environ-get

> 
>> +qemu_gdb = []
>> +if gdb_qemu_env:
>> +    qemu_gdb = ['gdbserver'] + gdb_qemu_env.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 6d27712617..49ddd586ef 100644
>> --- a/tests/qemu-iotests/testenv.py
>> +++ b/tests/qemu-iotests/testenv.py
>> @@ -27,6 +27,7 @@
>>   import glob
>>   from typing import Dict, Any, Optional, ContextManager
>> +DEF_GDB_OPTIONS = 'localhost:12345'
>>   def isxfile(path: str) -> bool:
>>       return os.path.isfile(path) and os.access(path, os.X_OK)
>> @@ -72,7 +73,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_OPTIONS']
>>       def get_env(self) -> Dict[str, str]:
>>           env = {}
>> @@ -163,7 +165,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 +174,14 @@ def __init__(self, imgfmt: str, imgproto: str, 
>> aiomode: str,
>>           self.misalign = misalign
>>           self.debug = debug
>> +        if gdb:
>> +            self.gdb_options = os.environ.get('GDB_OPTIONS', 
>> DEF_GDB_OPTIONS)
> 
> everywhere in the file os.getenv is used, let's be consistent on it.
> 
>> +            if not self.gdb_options:
>> +                # cover the case 'export GDB_OPTIONS='
>> +                self.gdb_options = DEF_GDB_OPTIONS
> 
> Hmm, a bit strange to handle 'export X=' only for this new variable, we 
> don't have such logic for other variables.. I'm not against still, if 
> you need it.
> 
>> +        elif 'GDB_OPTIONS' in os.environ:
>> +            del os.environ['GDB_OPTIONS']
> 
> Don't need to remove variable from envirton, it has no effect. The task 
> of TestEnv class is to prepare environment in its attributes, listed in 
> env_variables. Then prepared attributes are available through get_env(). 
> So if you don't want to provide GDB_OPTIONS, it's enough to not 
> initialize self.gdb_options. So, just drop "elif:" branch.

You forget that there are bash tests :)
Think about the following case:

# export GDB_OPTIONS="localhost:1236"

# ./check -qcow2 007 # a script test

The output will contain:
GDB_OPTIONS   --
VALGRIND_QEMU --
PRINT_QEMU_OUTPUT --

BUT in common.rc we will have:
	GDB=""
         if [ ! -z ${GDB_OPTIONS} ]; then # <---- still set!
             GDB="gdbserver ${GDB_OPTIONS}"
         fi

so gsdbserver will be set anyways, and the test will block waiting for a 
gdb connection.

Therefore we need that elif.

> 
>> +
>>           if valgrind:
>>               self.valgrind_qemu = 'y'
>> @@ -269,7 +280,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_OPTIONS   -- {GDB_OPTIONS}
> 
> Not sure we need to see this additional line every time.. Maybe, show it 
> only when gdb is set?

I think it can be helpful to remind the users what is not set and what 
is available to them (yes, one can also do ./check --help or read the 
docs but this is something even the laziest cannot unsee).

Thank you,
Emanuele
> 
>> +"""
>>           args = collections.defaultdict(str, self.get_env())
>>
> 
> 

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

[Vladimir Sementsov-Ogievskiy]

27.05.2021 14:06, Emanuele Giuseppe Esposito wrote:
> 
> 
> On 26/05/2021 21:15, Vladimir Sementsov-Ogievskiy wrote:
>> 20.05.2021 10:52, Emanuele Giuseppe Esposito wrote:
>>> Define -gdb flag and GDB_OPTIONS environment variable
>>> to python tests to attach a gdbserver to each qemu instance.
>>> This patch only adds and parses this flag, it does not yet add
>>> the implementation for it.
>>>
>>> if -gdb is not provided but $GDB_OPTIONS is set, ignore the
>>> environment variable.
>>>
>>> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
>>> ---
>>>   tests/qemu-iotests/check      |  6 +++++-
>>>   tests/qemu-iotests/iotests.py |  5 +++++
>>>   tests/qemu-iotests/testenv.py | 19 ++++++++++++++++---
>>>   3 files changed, 26 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/tests/qemu-iotests/check b/tests/qemu-iotests/check
>>> index d1c87ceaf1..b9820fdaaf 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_OPTIONS options \
>>> +                        ('localhost:12345' if $GDB_OPTIONS is empty)")
>>>       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 5d78de0f0b..d667fde6f8 100644
>>> --- a/tests/qemu-iotests/iotests.py
>>> +++ b/tests/qemu-iotests/iotests.py
>>> @@ -75,6 +75,11 @@
>>>   qemu_prog = os.environ.get('QEMU_PROG', 'qemu')
>>>   qemu_opts = os.environ.get('QEMU_OPTIONS', '').strip().split(' ')
>>> +gdb_qemu_env = os.environ.get('GDB_OPTIONS')
>>
>> should we specify a default? otherwise get() should raise an exception when GDB_OPTIONS is not set..
>>
>> or you need os.getenv, which will return None if variable is absent.
> 
> No, os.environ.get does not raise any exception. I tested it myself, plus:
> https://stackoverflow.com/questions/16924471/difference-between-os-getenv-and-os-environ-get

Ah, I'm wrong than. OK.

> 
>>
>>> +qemu_gdb = []
>>> +if gdb_qemu_env:
>>> +    qemu_gdb = ['gdbserver'] + gdb_qemu_env.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 6d27712617..49ddd586ef 100644
>>> --- a/tests/qemu-iotests/testenv.py
>>> +++ b/tests/qemu-iotests/testenv.py
>>> @@ -27,6 +27,7 @@
>>>   import glob
>>>   from typing import Dict, Any, Optional, ContextManager
>>> +DEF_GDB_OPTIONS = 'localhost:12345'
>>>   def isxfile(path: str) -> bool:
>>>       return os.path.isfile(path) and os.access(path, os.X_OK)
>>> @@ -72,7 +73,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_OPTIONS']
>>>       def get_env(self) -> Dict[str, str]:
>>>           env = {}
>>> @@ -163,7 +165,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 +174,14 @@ def __init__(self, imgfmt: str, imgproto: str, aiomode: str,
>>>           self.misalign = misalign
>>>           self.debug = debug
>>> +        if gdb:
>>> +            self.gdb_options = os.environ.get('GDB_OPTIONS', DEF_GDB_OPTIONS)
>>
>> everywhere in the file os.getenv is used, let's be consistent on it.
>>
>>> +            if not self.gdb_options:
>>> +                # cover the case 'export GDB_OPTIONS='
>>> +                self.gdb_options = DEF_GDB_OPTIONS
>>
>> Hmm, a bit strange to handle 'export X=' only for this new variable, we don't have such logic for other variables.. I'm not against still, if you need it.
>>
>>> +        elif 'GDB_OPTIONS' in os.environ:
>>> +            del os.environ['GDB_OPTIONS']
>>
>> Don't need to remove variable from envirton, it has no effect. The task of TestEnv class is to prepare environment in its attributes, listed in env_variables. Then prepared attributes are available through get_env(). So if you don't want to provide GDB_OPTIONS, it's enough to not initialize self.gdb_options. So, just drop "elif:" branch.
> 
> You forget that there are bash tests :)

It shouldn't differ, environment is prepared in the same way for bash and python tests

> Think about the following case:
> 
> # export GDB_OPTIONS="localhost:1236"
> 
> # ./check -qcow2 007 # a script test
> 
> The output will contain:
> GDB_OPTIONS   --
> VALGRIND_QEMU --
> PRINT_QEMU_OUTPUT --
> 
> BUT in common.rc we will have:
>      GDB=""
>          if [ ! -z ${GDB_OPTIONS} ]; then # <---- still set!

Ah, I thought  that we work through testenv.get_env.. But we have testenv.prepare_subprocess, which propagates the original environment too..

the bit I don't like in this all is inconsistency in interfaces of check and tests:

New interface of check:

with -gdb option use GDB_OPTIONS or default value to start gdbserver
without -gdb option ignore GDB_OPTIONS

New interface of tests:

with GDB_OPTIONS run gdbserver
without GDB_OPTIONS don't run gdbserver

So, GDB_OPTIONS is different thing for tests and for check script.


I'd go another way:

Add GDB option (boolean false/true, default false)
Add GDB_OPTIONS (string, default localhost:1236)

Add -gdb argument to check, which is an alternative way to set GDB variable.

This way environment-variables interface is similar for tests and ./check, and we don't need to drop a variable from the environ, and new variable behave similar to existing variables, doesn't introduce new logic.

But that all don't worth arguing. I'm OK with patch as is.

>              GDB="gdbserver ${GDB_OPTIONS}"
>          fi
> 
> so gsdbserver will be set anyways, and the test will block waiting for a gdb connection.
> 
> Therefore we need that elif.
> 
>>
>>> +
>>>           if valgrind:
>>>               self.valgrind_qemu = 'y'
>>> @@ -269,7 +280,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_OPTIONS   -- {GDB_OPTIONS}
>>
>> Not sure we need to see this additional line every time.. Maybe, show it only when gdb is set?
> 
> I think it can be helpful to remind the users what is not set and what is available to them (yes, one can also do ./check --help or read the docs but this is something even the laziest cannot unsee).
> 
> Thank you,
> Emanuele
>>
>>> +"""
>>>           args = collections.defaultdict(str, self.get_env())
>>>
>>
>>
> 


-- 
Best regards,
Vladimir

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

[Vladimir Sementsov-Ogievskiy]

20.05.2021 10:52, Emanuele Giuseppe Esposito wrote:
> Define -gdb flag and GDB_OPTIONS environment variable
> to python tests to attach a gdbserver to each qemu instance.
> This patch only adds and parses this flag, it does not yet add
> the implementation for it.
> 
> if -gdb is not provided but $GDB_OPTIONS is set, ignore the
> environment variable.
> 
> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> ---
>   tests/qemu-iotests/check      |  6 +++++-
>   tests/qemu-iotests/iotests.py |  5 +++++
>   tests/qemu-iotests/testenv.py | 19 ++++++++++++++++---
>   3 files changed, 26 insertions(+), 4 deletions(-)
> 
> diff --git a/tests/qemu-iotests/check b/tests/qemu-iotests/check
> index d1c87ceaf1..b9820fdaaf 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_OPTIONS options \
> +                        ('localhost:12345' if $GDB_OPTIONS is empty)")
>       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 5d78de0f0b..d667fde6f8 100644
> --- a/tests/qemu-iotests/iotests.py
> +++ b/tests/qemu-iotests/iotests.py
> @@ -75,6 +75,11 @@
>   qemu_prog = os.environ.get('QEMU_PROG', 'qemu')
>   qemu_opts = os.environ.get('QEMU_OPTIONS', '').strip().split(' ')
>   
> +gdb_qemu_env = os.environ.get('GDB_OPTIONS')
> +qemu_gdb = []
> +if gdb_qemu_env:
> +    qemu_gdb = ['gdbserver'] + gdb_qemu_env.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 6d27712617..49ddd586ef 100644
> --- a/tests/qemu-iotests/testenv.py
> +++ b/tests/qemu-iotests/testenv.py
> @@ -27,6 +27,7 @@
>   import glob
>   from typing import Dict, Any, Optional, ContextManager
>   
> +DEF_GDB_OPTIONS = 'localhost:12345'
>   
>   def isxfile(path: str) -> bool:
>       return os.path.isfile(path) and os.access(path, os.X_OK)
> @@ -72,7 +73,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_OPTIONS']
>   
>       def get_env(self) -> Dict[str, str]:
>           env = {}
> @@ -163,7 +165,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 +174,14 @@ def __init__(self, imgfmt: str, imgproto: str, aiomode: str,
>           self.misalign = misalign
>           self.debug = debug
>   
> +        if gdb:
> +            self.gdb_options = os.environ.get('GDB_OPTIONS', DEF_GDB_OPTIONS)
> +            if not self.gdb_options:
> +                # cover the case 'export GDB_OPTIONS='
> +                self.gdb_options = DEF_GDB_OPTIONS
> +        elif 'GDB_OPTIONS' in os.environ:

please add a comment:

  # to not propagate it in prepare_subprocess()

> +            del os.environ['GDB_OPTIONS']
> +
>           if valgrind:
>               self.valgrind_qemu = 'y'
>   
> @@ -269,7 +280,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_OPTIONS   -- {GDB_OPTIONS}
> +"""
>   
>           args = collections.defaultdict(str, self.get_env())
>   
> 

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

-- 
Best regards,
Vladimir

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

[Emanuele Giuseppe Esposito]

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

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

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index d667fde6f8..cf1ca60376 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -478,11 +478,13 @@ def __init__(self, seconds, errmsg="Timeout"):
         self.seconds = seconds
         self.errmsg = errmsg
     def __enter__(self):
-        signal.signal(signal.SIGALRM, self.timeout)
-        signal.setitimer(signal.ITIMER_REAL, self.seconds)
+        if not qemu_gdb:
+            signal.signal(signal.SIGALRM, self.timeout)
+            signal.setitimer(signal.ITIMER_REAL, self.seconds)
         return self
     def __exit__(self, exc_type, value, traceback):
-        signal.setitimer(signal.ITIMER_REAL, 0)
+        if not qemu_gdb:
+            signal.setitimer(signal.ITIMER_REAL, 0)
         return False
     def timeout(self, signum, frame):
         raise Exception(self.errmsg)
@@ -576,7 +578,7 @@ class VM(qtest.QEMUQtestMachine):
 
     def __init__(self, path_suffix=''):
         name = "qemu%s-%d" % (path_suffix, os.getpid())
-        timer = 15.0
+        timer = 15.0 if not qemu_gdb else None
         super().__init__(qemu_prog, qemu_opts, name=name,
                          test_dir=test_dir,
                          socket_scm_helper=socket_scm_helper,
-- 
2.30.2

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

[Vladimir Sementsov-Ogievskiy]

20.05.2021 10:52, Emanuele Giuseppe Esposito wrote:
> Attaching gdbserver implies that the qmp socket
> should wait indefinitely for an answer from QEMU.
> 
> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> ---
>   tests/qemu-iotests/iotests.py | 10 ++++++----
>   1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
> index d667fde6f8..cf1ca60376 100644
> --- a/tests/qemu-iotests/iotests.py
> +++ b/tests/qemu-iotests/iotests.py
> @@ -478,11 +478,13 @@ def __init__(self, seconds, errmsg="Timeout"):
>           self.seconds = seconds
>           self.errmsg = errmsg
>       def __enter__(self):
> -        signal.signal(signal.SIGALRM, self.timeout)
> -        signal.setitimer(signal.ITIMER_REAL, self.seconds)
> +        if not qemu_gdb:
> +            signal.signal(signal.SIGALRM, self.timeout)
> +            signal.setitimer(signal.ITIMER_REAL, self.seconds)
>           return self
>       def __exit__(self, exc_type, value, traceback):
> -        signal.setitimer(signal.ITIMER_REAL, 0)
> +        if not qemu_gdb:
> +            signal.setitimer(signal.ITIMER_REAL, 0)
>           return False
>       def timeout(self, signum, frame):
>           raise Exception(self.errmsg)

So, you just make the class do nothing.. I'd prefer something like this:

@contextmanager
def NoTimeout:
    yield

if qemu_gdb:
   Timeout = NoTimeout


anyway:

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>


> @@ -576,7 +578,7 @@ class VM(qtest.QEMUQtestMachine):
>   
>       def __init__(self, path_suffix=''):
>           name = "qemu%s-%d" % (path_suffix, os.getpid())
> -        timer = 15.0
> +        timer = 15.0 if not qemu_gdb else None
>           super().__init__(qemu_prog, qemu_opts, name=name,
>                            test_dir=test_dir,
>                            socket_scm_helper=socket_scm_helper,
> 


Still, it's not simple to avoid any kind of timeouts. The most annoying would be timeouts in event_wait() / events_wait()

-- 
Best regards,
Vladimir

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

[Emanuele Giuseppe Esposito]

> 
> So, you just make the class do nothing.. I'd prefer something like this:
> 
> @contextmanager
> def NoTimeout:
>     yield
> 
> if qemu_gdb:
>    Timeout = NoTimeout

I am not sure I understand what you want to do here.
I have a basic understanding of @contextmanager, but can you explain 
more what you want to do?

Alternatively, I send the patch as-is, and then you can send this change.

Thank you,
Emanuele

> 
> 
> anyway:
> 
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> 

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

[Vladimir Sementsov-Ogievskiy]

03.06.2021 14:03, Emanuele Giuseppe Esposito wrote:
> 
>>
>> So, you just make the class do nothing.. I'd prefer something like this:
>>
>> @contextmanager
>> def NoTimeout:
>>     yield
>>
>> if qemu_gdb:
>>    Timeout = NoTimeout
> 
> I am not sure I understand what you want to do here.
> I have a basic understanding of @contextmanager, but can you explain more what you want to do?

With qemu_gdb you make the class do absolutely nothing. So, it's just provides an interface of context manager, but does nothing.

>> @contextmanager
>> def NoTimeout:
>>     yield

is the same thing: it's context manager, so you can do

with NoTimeout():
   ....

but that context manager does absolutely nothing.


@contextmanager gives you a simple way to create a context manager. You define a function which has yield point. So, everything before yield is  __enter__, everything after yield is __exit__. And you can return a value by yield, it will be a return value of __enter__.


So, what I meant: keep Timeout class as is and just add code that I suggested after it.


> 
> Alternatively, I send the patch as-is, and then you can send this change.

OK for me.

> 
> Thank you,
> Emanuele
> 
>>
>>
>> anyway:
>>
>> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>
> 


-- 
Best regards,
Vladimir

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

[Emanuele Giuseppe Esposito]

On 03/06/2021 14:25, Vladimir Sementsov-Ogievskiy wrote:
> 03.06.2021 14:03, Emanuele Giuseppe Esposito wrote:
>>
>>>
>>> So, you just make the class do nothing.. I'd prefer something like this:
>>>
>>> @contextmanager
>>> def NoTimeout:
>>>     yield
>>>
>>> if qemu_gdb:
>>>    Timeout = NoTimeout
>>
>> I am not sure I understand what you want to do here.
>> I have a basic understanding of @contextmanager, but can you explain 
>> more what you want to do?
> 
> With qemu_gdb you make the class do absolutely nothing. So, it's just 
> provides an interface of context manager, but does nothing.
> 
>>> @contextmanager
>>> def NoTimeout:
>>>     yield
> 
> is the same thing: it's context manager, so you can do
> 
> with NoTimeout():
>    ....
> 
> but that context manager does absolutely nothing.
> 
> 
> @contextmanager gives you a simple way to create a context manager. You 
> define a function which has yield point. So, everything before yield is  
> __enter__, everything after yield is __exit__. And you can return a 
> value by yield, it will be a return value of __enter__.
> 
> 
> So, what I meant: keep Timeout class as is and just add code that I 
> suggested after it.

Oh okay. I didn't know Timeout = NoTimeout was possible (and I didn't 
know where to put it anyways). Looks cleaner than the normal ifs, will 
apply this change and send v5.

Thank you,
Emanuele

[PATCH v4 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 cf1ca60376..c9628e6828 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -579,7 +579,8 @@ class VM(qtest.QEMUQtestMachine):
     def __init__(self, path_suffix=''):
         name = "qemu%s-%d" % (path_suffix, os.getpid())
         timer = 15.0 if not qemu_gdb else None
-        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, qmp_timer=timer)
-- 
2.30.2

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

[Vladimir Sementsov-Ogievskiy]

20.05.2021 10:52, Emanuele Giuseppe Esposito wrote:
> Signed-off-by: Emanuele Giuseppe Esposito<eesposit@redhat.com>

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

-- 
Best regards,
Vladimir

[PATCH v4 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 7f49c9716d..f1d5395ff2 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=""
+        if [ ! -z ${GDB_OPTIONS} ]; then
+            GDB="gdbserver ${GDB_OPTIONS}"
+        fi
+
         VALGRIND_QEMU="${VALGRIND_QEMU_VM}" _qemu_proc_exec "${VALGRIND_LOGFILE}" \
-            "$QEMU_PROG" $QEMU_OPTIONS "$@"
+            $GDB "$QEMU_PROG" $QEMU_OPTIONS "$@"
     )
     RETVAL=$?
     _qemu_proc_valgrind_log "${VALGRIND_LOGFILE}" $RETVAL
-- 
2.30.2

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

[Vladimir Sementsov-Ogievskiy]

20.05.2021 10:52, 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>

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

-- 
Best regards,
Vladimir

[PATCH v4 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 | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/docs/devel/testing.rst b/docs/devel/testing.rst
index 8144e316a4..a746cab745 100644
--- a/docs/devel/testing.rst
+++ b/docs/devel/testing.rst
@@ -229,6 +229,17 @@ Debugging a test case
 The following options to the ``check`` script can be useful when debugging
 a failing test:
 
+* ``-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_OPTIONS``
+  environment variable.  By default (if ``$GDB_OPTIONS`` is empty), it listens on
+  ``localhost:12345``.
+  It is possible to connect to it for example with
+  ``gdb -iex "target remote $addr"``, where ``$addr`` is the address
+  ``gdbserver`` listens on.
+  If the ``-gdb`` option is not used, ``$GDB_OPTIONS`` is ignored,
+  regardless on whether it is set or not.
+
 * ``-d`` (debug) just increases the logging verbosity, showing
   for example the QMP commands and answers.
 
-- 
2.30.2

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

[Vladimir Sementsov-Ogievskiy]

20.05.2021 10:52, Emanuele Giuseppe Esposito wrote:
> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

> ---
>   docs/devel/testing.rst | 11 +++++++++++
>   1 file changed, 11 insertions(+)
> 
> diff --git a/docs/devel/testing.rst b/docs/devel/testing.rst
> index 8144e316a4..a746cab745 100644
> --- a/docs/devel/testing.rst
> +++ b/docs/devel/testing.rst
> @@ -229,6 +229,17 @@ Debugging a test case
>   The following options to the ``check`` script can be useful when debugging
>   a failing test:
>   
> +* ``-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_OPTIONS``
> +  environment variable.  By default (if ``$GDB_OPTIONS`` is empty), it listens on
> +  ``localhost:12345``.
> +  It is possible to connect to it for example with
> +  ``gdb -iex "target remote $addr"``, where ``$addr`` is the address
> +  ``gdbserver`` listens on.
> +  If the ``-gdb`` option is not used, ``$GDB_OPTIONS`` is ignored,
> +  regardless on whether it is set or not.
> +
>   * ``-d`` (debug) just increases the logging verbosity, showing
>     for example the QMP commands and answers.
>   
> 

Didn't you think of an interface as simple as just wrap qemu binary by "gdb --args" and redirect stdin and stdout directly to the terminal where test is running? Wouldn't it more convenient? So, you just start ./check -gdb <test>, and you are in gdb. Or you need exactly gdb server, to attach from another machine?

Keeping this possibility in mind, it's probably better to call you option "-gdbserver"... But we can rename later if needed, finally, it's only a test framework.


-- 
Best regards,
Vladimir

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

[Emanuele Giuseppe Esposito]

On 28/05/2021 19:16, Vladimir Sementsov-Ogievskiy wrote:
> 20.05.2021 10:52, Emanuele Giuseppe Esposito wrote:
>> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> 
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> 
>> ---
>>   docs/devel/testing.rst | 11 +++++++++++
>>   1 file changed, 11 insertions(+)
>>
>> diff --git a/docs/devel/testing.rst b/docs/devel/testing.rst
>> index 8144e316a4..a746cab745 100644
>> --- a/docs/devel/testing.rst
>> +++ b/docs/devel/testing.rst
>> @@ -229,6 +229,17 @@ Debugging a test case
>>   The following options to the ``check`` script can be useful when 
>> debugging
>>   a failing test:
>> +* ``-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_OPTIONS``
>> +  environment variable.  By default (if ``$GDB_OPTIONS`` is empty), 
>> it listens on
>> +  ``localhost:12345``.
>> +  It is possible to connect to it for example with
>> +  ``gdb -iex "target remote $addr"``, where ``$addr`` is the address
>> +  ``gdbserver`` listens on.
>> +  If the ``-gdb`` option is not used, ``$GDB_OPTIONS`` is ignored,
>> +  regardless on whether it is set or not.
>> +
>>   * ``-d`` (debug) just increases the logging verbosity, showing
>>     for example the QMP commands and answers.
>>
> 
> Didn't you think of an interface as simple as just wrap qemu binary by 
> "gdb --args" and redirect stdin and stdout directly to the terminal 
> where test is running? Wouldn't it more convenient? So, you just start 
> ./check -gdb <test>, and you are in gdb. Or you need exactly gdb server, 
> to attach from another machine?
> 
> Keeping this possibility in mind, it's probably better to call you 
> option "-gdbserver"... But we can rename later if needed, finally, it's 
> only a test framework.
> 
>
See 
https://patchew.org/QEMU/20210414170352.29927-1-eesposit@redhat.com/20210414170352.29927-5-eesposit@redhat.com/ 

(penultimate email), there was a similar question:

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

At this point I should put this somewhere, either in commit message or 
in the actual documentation...

I don't know about the name. "gdb" should also be short for "gdbserver" 
in a way.

Thank you,
Emanuele

[PATCH v4 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.

Reviewed-by: Max Reitz <mreitz@redhat.com>
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 b9820fdaaf..2101cedfe3 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_OPTIONS options \
                         ('localhost:12345' if $GDB_OPTIONS is empty)")
+    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 c9628e6828..41462a80fc 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -97,6 +97,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.Popen 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 49ddd586ef..319d29cb0c 100644
--- a/tests/qemu-iotests/testenv.py
+++ b/tests/qemu-iotests/testenv.py
@@ -282,6 +282,7 @@ def print_env(self) -> None:
 SOCK_DIR      -- {SOCK_DIR}
 SOCKET_SCM_HELPER -- {SOCKET_SCM_HELPER}
 GDB_OPTIONS   -- {GDB_OPTIONS}
+VALGRIND_QEMU -- {VALGRIND_QEMU}
 """
 
         args = collections.defaultdict(str, self.get_env())
-- 
2.30.2

Re: [PATCH v4 09/15] qemu-iotests: extend the check script to support valgrind for python tests

[Vladimir Sementsov-Ogievskiy]

20.05.2021 10:52, 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.
> 
> Reviewed-by: Max Reitz <mreitz@redhat.com>
> 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 b9820fdaaf..2101cedfe3 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_OPTIONS options \
>                           ('localhost:12345' if $GDB_OPTIONS is empty)")
> +    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 c9628e6828..41462a80fc 100644
> --- a/tests/qemu-iotests/iotests.py
> +++ b/tests/qemu-iotests/iotests.py
> @@ -97,6 +97,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()

a bit strange that you need to strip test_dir here.. Why?

> +    # %p allows to put the valgrind process PID, since
> +    # we don't know it a priori (subprocess.Popen 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 49ddd586ef..319d29cb0c 100644
> --- a/tests/qemu-iotests/testenv.py
> +++ b/tests/qemu-iotests/testenv.py
> @@ -282,6 +282,7 @@ def print_env(self) -> None:
>   SOCK_DIR      -- {SOCK_DIR}
>   SOCKET_SCM_HELPER -- {SOCKET_SCM_HELPER}
>   GDB_OPTIONS   -- {GDB_OPTIONS}
> +VALGRIND_QEMU -- {VALGRIND_QEMU}
>   """
>   
>           args = collections.defaultdict(str, self.get_env())
> 

commit subject sais "support valgrind", but actually we only add a variable. valgrind is not actually used yet.. I'd reflect it in commit subject somehow

-- 
Best regards,
Vladimir

Re: [PATCH v4 09/15] qemu-iotests: extend the check script to support valgrind for python tests

[Emanuele Giuseppe Esposito]

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

> a bit strange that you need to strip test_dir here.. Why?
Yep, it's unnecessary


>> +VALGRIND_QEMU -- {VALGRIND_QEMU}
>>   """
>>           args = collections.defaultdict(str, self.get_env())
>>
> 
> commit subject sais "support valgrind", but actually we only add a 
> variable. valgrind is not actually used yet.. I'd reflect it in commit 
> subject somehow
> 
will replace with "prepare supporting"

Emanuele

[PATCH v4 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>
---
 tests/qemu-iotests/iotests.py | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 41462a80fc..5d75094ba6 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -489,12 +489,12 @@ def __init__(self, seconds, errmsg="Timeout"):
         self.seconds = seconds
         self.errmsg = errmsg
     def __enter__(self):
-        if not qemu_gdb:
+        if not (qemu_gdb or qemu_valgrind):
             signal.signal(signal.SIGALRM, self.timeout)
             signal.setitimer(signal.ITIMER_REAL, self.seconds)
         return self
     def __exit__(self, exc_type, value, traceback):
-        if not qemu_gdb:
+        if not (qemu_gdb or qemu_valgrind):
             signal.setitimer(signal.ITIMER_REAL, 0)
         return False
     def timeout(self, signum, frame):
@@ -589,7 +589,7 @@ class VM(qtest.QEMUQtestMachine):
 
     def __init__(self, path_suffix=''):
         name = "qemu%s-%d" % (path_suffix, os.getpid())
-        timer = 15.0 if not qemu_gdb else None
+        timer = 15.0 if not (qemu_gdb or qemu_valgrind) else None
         super().__init__(qemu_prog, qemu_opts, wrapper=qemu_gdb,
                          name=name,
                          test_dir=test_dir,
-- 
2.30.2

Re: [PATCH v4 10/15] qemu-iotests: extent QMP socket timeout when using valgrind

[Vladimir Sementsov-Ogievskiy]

20.05.2021 10:52, Emanuele Giuseppe Esposito wrote:
> As with gdbserver, valgrind delays the test execution, so
> the default QMP socket timeout timeout too soon.

First, "Timeout" class is a generic class for timeouts, not relying to sockets. So,  commit message lacks information about that we modify generic context-provider class and why we do it.

> 
> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> ---
>   tests/qemu-iotests/iotests.py | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
> index 41462a80fc..5d75094ba6 100644
> --- a/tests/qemu-iotests/iotests.py
> +++ b/tests/qemu-iotests/iotests.py
> @@ -489,12 +489,12 @@ def __init__(self, seconds, errmsg="Timeout"):
>           self.seconds = seconds
>           self.errmsg = errmsg
>       def __enter__(self):
> -        if not qemu_gdb:
> +        if not (qemu_gdb or qemu_valgrind):
>               signal.signal(signal.SIGALRM, self.timeout)
>               signal.setitimer(signal.ITIMER_REAL, self.seconds)
>           return self
>       def __exit__(self, exc_type, value, traceback):
> -        if not qemu_gdb:
> +        if not (qemu_gdb or qemu_valgrind):

If you follow my suggestion on 05, you'll have to modify only one line instead of two.

>               signal.setitimer(signal.ITIMER_REAL, 0)
>           return False
>       def timeout(self, signum, frame):
> @@ -589,7 +589,7 @@ class VM(qtest.QEMUQtestMachine):
>   
>       def __init__(self, path_suffix=''):
>           name = "qemu%s-%d" % (path_suffix, os.getpid())
> -        timer = 15.0 if not qemu_gdb else None
> +        timer = 15.0 if not (qemu_gdb or qemu_valgrind) else None
>           super().__init__(qemu_prog, qemu_opts, wrapper=qemu_gdb,
>                            name=name,
>                            test_dir=test_dir,
> 

still it should work as intended:
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>


-- 
Best regards,
Vladimir

[PATCH v4 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 | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 5d75094ba6..a06284acad 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -597,6 +597,22 @@ def __init__(self, path_suffix=''):
                          sock_dir=sock_dir, qmp_timer=timer)
         self._num_drives = 0
 
+    def subprocess_check_valgrind(self, valgrind: List[str]) -> None:
+        if not valgrind or not self._popen:
+            return
+
+        valgrind_filename =  f"{test_dir}/{self._popen.pid}.valgrind"
+
+        if self.exitcode() == 99:
+            with open(valgrind_filename) as f:
+                print(f.read())
+        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 v4 11/15] qemu-iotests: allow valgrind to read/delete the generated log file

[Vladimir Sementsov-Ogievskiy]

20.05.2021 10:52, 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 | 16 ++++++++++++++++
>   1 file changed, 16 insertions(+)
> 
> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
> index 5d75094ba6..a06284acad 100644
> --- a/tests/qemu-iotests/iotests.py
> +++ b/tests/qemu-iotests/iotests.py
> @@ -597,6 +597,22 @@ def __init__(self, path_suffix=''):
>                            sock_dir=sock_dir, qmp_timer=timer)
>           self._num_drives = 0
>   
> +    def subprocess_check_valgrind(self, valgrind: List[str]) -> None:

For me just "check_valgrind" would be more intuitive.

I think, you also can use qemu_valgrind global variable directly.. What is the reason for passing it as parameter?

> +        if not valgrind or not self._popen:
> +            return
> +
> +        valgrind_filename =  f"{test_dir}/{self._popen.pid}.valgrind"

should we use os.path.join instead? I don't know.. And don't care, as I don't use windows anyway. Still os.path.join is used everywhere in the file except for has_working_luks() function.. So, you are going add another exception.

> +
> +        if self.exitcode() == 99:
> +            with open(valgrind_filename) as f:
> +                print(f.read())
> +        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)
> 

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

-- 
Best regards,
Vladimir

[PATCH v4 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 a06284acad..75f1e1711c 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -590,7 +590,8 @@ class VM(qtest.QEMUQtestMachine):
     def __init__(self, path_suffix=''):
         name = "qemu%s-%d" % (path_suffix, os.getpid())
         timer = 15.0 if not (qemu_gdb or qemu_valgrind) else None
-        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 v4 12/15] qemu-iotests: insert valgrind command line as wrapper for qemu binary

[Vladimir Sementsov-Ogievskiy]

20.05.2021 10:52, 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.

I'd prefer just return an error immediately if user specify both -gdb and -valgrind

> 
> 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 a06284acad..75f1e1711c 100644
> --- a/tests/qemu-iotests/iotests.py
> +++ b/tests/qemu-iotests/iotests.py
> @@ -590,7 +590,8 @@ class VM(qtest.QEMUQtestMachine):
>       def __init__(self, path_suffix=''):
>           name = "qemu%s-%d" % (path_suffix, os.getpid())
>           timer = 15.0 if not (qemu_gdb or qemu_valgrind) else None
> -        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,
> 


Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

-- 
Best regards,
Vladimir

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

[Emanuele Giuseppe Esposito]

Reviewed-by: Max Reitz <mreitz@redhat.com>
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 a746cab745..d743e88746 100644
--- a/docs/devel/testing.rst
+++ b/docs/devel/testing.rst
@@ -240,6 +240,13 @@ a failing test:
   If the ``-gdb`` option is not used, ``$GDB_OPTIONS`` is ignored,
   regardless on whether it is set or not.
 
+* ``-valgrind`` attaches 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 v4 13/15] docs/devel/testing: add -valgrind option to the debug section of QEMU iotests

[Vladimir Sementsov-Ogievskiy]

20.05.2021 10:52, Emanuele Giuseppe Esposito wrote:
> Reviewed-by: Max Reitz<mreitz@redhat.com>
> Signed-off-by: Emanuele Giuseppe Esposito<eesposit@redhat.com>


Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

-- 
Best regards,
Vladimir

[PATCH v4 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.

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

diff --git a/tests/qemu-iotests/check b/tests/qemu-iotests/check
index 2101cedfe3..51b90681ab 100755
--- a/tests/qemu-iotests/check
+++ b/tests/qemu-iotests/check
@@ -33,6 +33,8 @@ 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='redirects qemu\'s stdout and stderr to the test output')
     p.add_argument('-gdb', action='store_true',
                    help="start gdbserver with $GDB_OPTIONS options \
                         ('localhost:12345' if $GDB_OPTIONS is empty)")
@@ -117,7 +119,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 75f1e1711c..53a3916a91 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -80,6 +80,8 @@
 if gdb_qemu_env:
     qemu_gdb = ['gdbserver'] + gdb_qemu_env.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', '.')
@@ -614,6 +616,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 is not 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 319d29cb0c..b79ce22fe9 100644
--- a/tests/qemu-iotests/testenv.py
+++ b/tests/qemu-iotests/testenv.py
@@ -74,7 +74,7 @@ class TestEnv(ContextManager['TestEnv']):
                      'AIOMODE', 'CACHEMODE', 'VALGRIND_QEMU',
                      'CACHEMODE_IS_DEFAULT', 'IMGFMT_GENERIC', 'IMGOPTSSYNTAX',
                      'IMGKEYSECRET', 'QEMU_DEFAULT_MACHINE', 'MALLOC_PERTURB_',
-                     'GDB_OPTIONS']
+                     'GDB_OPTIONS', 'PRINT_QEMU']
 
     def get_env(self) -> Dict[str, str]:
         env = {}
@@ -166,7 +166,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
@@ -174,6 +175,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_options = os.environ.get('GDB_OPTIONS', DEF_GDB_OPTIONS)
             if not self.gdb_options:
@@ -283,6 +287,7 @@ def print_env(self) -> None:
 SOCKET_SCM_HELPER -- {SOCKET_SCM_HELPER}
 GDB_OPTIONS   -- {GDB_OPTIONS}
 VALGRIND_QEMU -- {VALGRIND_QEMU}
+PRINT_QEMU_OUTPUT -- {PRINT_QEMU}
 """
 
         args = collections.defaultdict(str, self.get_env())
-- 
2.30.2

Re: [PATCH v4 14/15] qemu-iotests: add option to show qemu binary logs on stdout

[Vladimir Sementsov-Ogievskiy]

20.05.2021 10:52, Emanuele Giuseppe Esposito wrote:
> Using the flag -p, allow the qemu binary to print to stdout.
> 
> Reviewed-by: Max Reitz <mreitz@redhat.com>
> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> ---
>   tests/qemu-iotests/check      | 4 +++-
>   tests/qemu-iotests/iotests.py | 9 +++++++++
>   tests/qemu-iotests/testenv.py | 9 +++++++--
>   3 files changed, 19 insertions(+), 3 deletions(-)
> 
> diff --git a/tests/qemu-iotests/check b/tests/qemu-iotests/check
> index 2101cedfe3..51b90681ab 100755
> --- a/tests/qemu-iotests/check
> +++ b/tests/qemu-iotests/check
> @@ -33,6 +33,8 @@ 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='redirects qemu\'s stdout and stderr to the test output')
>       p.add_argument('-gdb', action='store_true',
>                      help="start gdbserver with $GDB_OPTIONS options \
>                           ('localhost:12345' if $GDB_OPTIONS is empty)")
> @@ -117,7 +119,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 75f1e1711c..53a3916a91 100644
> --- a/tests/qemu-iotests/iotests.py
> +++ b/tests/qemu-iotests/iotests.py
> @@ -80,6 +80,8 @@
>   if gdb_qemu_env:
>       qemu_gdb = ['gdbserver'] + gdb_qemu_env.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', '.')
> @@ -614,6 +616,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 is not None:
> +            # set QEMU binary output to stdout
> +            self._qemu_log_file.close()
> +            self._qemu_log_file = None
> +

So, many use of _private members actually show that proper way of doing this is adding an option to __init__ instead

Interesting will pylint complain on using _private members outside of the home class?

>       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 319d29cb0c..b79ce22fe9 100644
> --- a/tests/qemu-iotests/testenv.py
> +++ b/tests/qemu-iotests/testenv.py
> @@ -74,7 +74,7 @@ class TestEnv(ContextManager['TestEnv']):
>                        'AIOMODE', 'CACHEMODE', 'VALGRIND_QEMU',
>                        'CACHEMODE_IS_DEFAULT', 'IMGFMT_GENERIC', 'IMGOPTSSYNTAX',
>                        'IMGKEYSECRET', 'QEMU_DEFAULT_MACHINE', 'MALLOC_PERTURB_',
> -                     'GDB_OPTIONS']
> +                     'GDB_OPTIONS', 'PRINT_QEMU']
>   
>       def get_env(self) -> Dict[str, str]:
>           env = {}
> @@ -166,7 +166,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
> @@ -174,6 +175,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_options = os.environ.get('GDB_OPTIONS', DEF_GDB_OPTIONS)
>               if not self.gdb_options:
> @@ -283,6 +287,7 @@ def print_env(self) -> None:
>   SOCKET_SCM_HELPER -- {SOCKET_SCM_HELPER}
>   GDB_OPTIONS   -- {GDB_OPTIONS}
>   VALGRIND_QEMU -- {VALGRIND_QEMU}
> +PRINT_QEMU_OUTPUT -- {PRINT_QEMU}
>   """
>   
>           args = collections.defaultdict(str, self.get_env())
> 

5 places we need to modify to add a new option. That's not very good :( (not about your patch).

And I still doubt, that we want to add any new variable to print_env. If we do, than it's simpler to print all variables here, than, one place less to modify by hand.


Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

-- 
Best regards,
Vladimir

Re: [PATCH v4 14/15] qemu-iotests: add option to show qemu binary logs on stdout

[Emanuele Giuseppe Esposito]

>> +
>>   imgfmt = os.environ.get('IMGFMT', 'raw')
>>   imgproto = os.environ.get('IMGPROTO', 'file')
>>   output_dir = os.environ.get('OUTPUT_DIR', '.')
>> @@ -614,6 +616,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 is not None:
>> +            # set QEMU binary output to stdout
>> +            self._qemu_log_file.close()
>> +            self._qemu_log_file = None
>> +
> 
> So, many use of _private members actually show that proper way of doing 
> this is adding an option to __init__ instead

And then add yet another bool variable in __init__ just to mark when use 
the log file or not? At this point, if we really don't want this here we 
can just create a public function in machine.py and call that...
This can also be shared with machine.py's _post_shutdown().

> 
> Interesting will pylint complain on using _private members outside of 
> the home class?

No, test 297 tests it and prints no warning or error.


Thank you,
Emanuele

[PATCH v4 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 d743e88746..1192d6489e 100644
--- a/docs/devel/testing.rst
+++ b/docs/devel/testing.rst
@@ -250,6 +250,10 @@ a failing test:
 * ``-d`` (debug) just increases the logging verbosity, showing
   for example the QMP commands and answers.
 
+* ``-p`` (print) redirect QEMU’s stdout and stderr to the test output,
+  instead of saving it into a log file in
+  ``$TEST_DIR/qemu-machine-<random_string>``.
+
 Test case groups
 ----------------
 
-- 
2.30.2

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

[Vladimir Sementsov-Ogievskiy]

20.05.2021 10:52, Emanuele Giuseppe Esposito wrote:
> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>

I'd merge it to previous patch. anyway:

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

> ---
>   docs/devel/testing.rst | 4 ++++
>   1 file changed, 4 insertions(+)
> 
> diff --git a/docs/devel/testing.rst b/docs/devel/testing.rst
> index d743e88746..1192d6489e 100644
> --- a/docs/devel/testing.rst
> +++ b/docs/devel/testing.rst
> @@ -250,6 +250,10 @@ a failing test:
>   * ``-d`` (debug) just increases the logging verbosity, showing
>     for example the QMP commands and answers.
>   
> +* ``-p`` (print) redirect QEMU’s stdout and stderr to the test output,
> +  instead of saving it into a log file in
> +  ``$TEST_DIR/qemu-machine-<random_string>``.
> +
>   Test case groups
>   ----------------
>   
> 


-- 
Best regards,
Vladimir

Re: [PATCH v4 00/15] qemu_iotests: improve debugging options

[Vladimir Sementsov-Ogievskiy]

20.05.2021 10:52, Emanuele Giuseppe Esposito wrote:
> 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,

Could you describe how to use it?

I often have to debug qemu iotests with gdb, and can say the following:

1. Test runs different qemu binaries (not only qemu, but also qemu-nbd, qemu-img, qemu-io)

2. Test can run qemu binaries several times, or even in parallel.

So, with a new option, how will I choice, which qemu (or qemu-nbd, etc) process I want to be attached to gdb?

Currently, I do the following: I recompile with  "sleep(15);" at the place where I want to attach, and start the test. During the sleep, I find the needed process in "ps aux" output and start "gdb -p PID".. Sometimes there may be troubles if the place where I inserted the sleep is triggered on another code path.

So, with new option, can my workflow be improved or not?

> 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,5,10 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>
> ---
> v4:
> * Rename environment variable from GDB_QEMU to GDB_OPTIONS
> * This time test 297 (pylint) passes [Max]
> * Refactor the qmp_timer field in machine.py, and add a new
>    parameter in machine.py and subclasses constructor [John]
> * Add additional check in patch 4 to cover the case where
>    GDB_OPTIONS is empty
> 
> 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        | 30 +++++++++++++++++++
>   python/qemu/machine.py        |  7 +++--
>   python/qemu/qtest.py          |  9 ++++--
>   tests/qemu-iotests/check      | 15 +++++++---
>   tests/qemu-iotests/common.rc  |  8 ++++-
>   tests/qemu-iotests/iotests.py | 56 +++++++++++++++++++++++++++++++----
>   tests/qemu-iotests/testenv.py | 25 ++++++++++++++--
>   7 files changed, 132 insertions(+), 18 deletions(-)
> 


-- 
Best regards,
Vladimir

Re: [PATCH v4 00/15] qemu_iotests: improve debugging options

[Emanuele Giuseppe Esposito]

On 26/05/2021 13:32, Vladimir Sementsov-Ogievskiy wrote:
> 20.05.2021 10:52, Emanuele Giuseppe Esposito wrote:
>> 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,
> 
> Could you describe how to use it?
> 
> I often have to debug qemu iotests with gdb, and can say the following:
> 
> 1. Test runs different qemu binaries (not only qemu, but also qemu-nbd, 
> qemu-img, qemu-io)
> 
> 2. Test can run qemu binaries several times, or even in parallel.
> 
> So, with a new option, how will I choice, which qemu (or qemu-nbd, etc) 
> process I want to be attached to gdb?
> 
> Currently, I do the following: I recompile with  "sleep(15);" at the 
> place where I want to attach, and start the test. During the sleep, I 
> find the needed process in "ps aux" output and start "gdb -p PID".. 
> Sometimes there may be troubles if the place where I inserted the sleep 
> is triggered on another code path.
> 
> So, with new option, can my workflow be improved or not?

Unfortunately not, unless you want to debug a qemu_system binary.
The usage should be clear in the doc patches of this series (please 
check them too if you haven't done already), but the idea is to just 
wrap gdb around every qemu binary usage.

If you apply these patches together with Paolo's "qemu-iotests: quality 
of life improvements", you will get something much better, where you can 
choose which test function to run under gdb.

If I recall correctly, you should be able to do something like:
check -gdb -- ../../../tests/qemu-iotests/055 TestCompressedToQcow2

Still, if the test function has multiple qemu binary invocations or in 
parallel, you gdb will be applied to all of them. I don't think 
test/debug granularity can be reduced any further, at least not so easily.

Thank you,
Emanuele

> 
>> 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,5,10 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>
>> ---
>> v4:
>> * Rename environment variable from GDB_QEMU to GDB_OPTIONS
>> * This time test 297 (pylint) passes [Max]
>> * Refactor the qmp_timer field in machine.py, and add a new
>>    parameter in machine.py and subclasses constructor [John]
>> * Add additional check in patch 4 to cover the case where
>>    GDB_OPTIONS is empty
>>
>> 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        | 30 +++++++++++++++++++
>>   python/qemu/machine.py        |  7 +++--
>>   python/qemu/qtest.py          |  9 ++++--
>>   tests/qemu-iotests/check      | 15 +++++++---
>>   tests/qemu-iotests/common.rc  |  8 ++++-
>>   tests/qemu-iotests/iotests.py | 56 +++++++++++++++++++++++++++++++----
>>   tests/qemu-iotests/testenv.py | 25 ++++++++++++++--
>>   7 files changed, 132 insertions(+), 18 deletions(-)
>>
> 
>