| Message ID | 20210520075236.44723-5-eesposit@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | qemu_iotests: improve debugging options | expand |
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()) > >
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
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.
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. > >
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()) > >
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()) >> > >
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()) >>> >> >> >
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>
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())
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(-)