|
From: | Vladimir Sementsov-Ogievskiy |
Subject: | Re: [PATCH v4 04/15] qemu-iotests: add option to attach gdbserver |
Date: | Thu, 27 May 2021 15:17:31 +0300 |
User-agent: | Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.10.2 |
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_OPTIONSHmm, 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
[Prev in Thread] | Current Thread | [Next in Thread] |