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