Re: [PATCH v5 10/16] qemu-iotests: extend the check script to prepare supporting valgrind for python tests

[Vladimir Sementsov-Ogievskiy]

05.06.2021 20:38, Emanuele Giuseppe Esposito wrote:
> On 05/06/2021 15:28, Vladimir Sementsov-Ogievskiy wrote:
> > 04.06.2021 12:17, 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 valgrind
> > > PID to assign to the log file name, use the "%p" flag in valgrind
> > > log file name that automatically puts the process PID at runtime.
> > >
> > > Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> > > Reviewed-by: Max Reitz <mreitz@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 1dba4218c0..e6aa110715 100755
> > > --- a/tests/qemu-iotests/check
> > > +++ b/tests/qemu-iotests/check
> > > @@ -39,6 +39,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'],
> > > @@ -88,9 +92,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 c547e8c07b..3fa1bd0ab5 100644
> > > --- a/tests/qemu-iotests/iotests.py
> > > +++ b/tests/qemu-iotests/iotests.py
> > > @@ -96,6 +96,17 @@
> > >       sys.stderr.write('Please run this test via the "check" script\n')
> > >       sys.exit(os.EX_USAGE)
> > > +qemu_valgrind = []
> > > +if os.environ.get('VALGRIND_QEMU') == "y" and \
> > > +    os.environ.get('NO_VALGRIND') != "y":
> >
> > Hmm, interesting, why do you need additional NO_VALGRIND variable
>
> To maintain consistency with the bash tests, where we have:
>
> # Valgrind bug #409141 https://bugs.kde.org/show_bug.cgi?id=409141
> # Until valgrind 3.16+ is ubiquitous, we must work around a hang in
> # valgrind when issuing sigkill. Disable valgrind for this invocation.
> _NO_VALGRIND()
> {
>      NO_VALGRIND="y" "$@"
> }

A, hm, I see it in bash tests. So, it's intended to not set by user but by 
test.. But I doubt that python test will want to set environment variable to 
disable valgrind. Most probably they will want to set some 
valgrind_supported=False near supported_fmts=['qcow2']. So, we'll need 
valgrind_supported argument for execute_setup_common. But no reason to 
implement it until we don't need.

> > > +    valgrind_logfile = "--log-file=" + test_dir
> > > +    # %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 8501c6caf5..8bf154376f 100644
> > > --- a/tests/qemu-iotests/testenv.py
> > > +++ b/tests/qemu-iotests/testenv.py
> > > @@ -298,6 +298,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())
> >
> >
> >
> > Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>


--
Best regards,
Vladimir