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
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
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.
--
Best regards,
Vladimir
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())
>
>
--
Best regards,
Vladimir
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())
>>>
>>
>>
>
--
Best regards,
Vladimir
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
© 2016 - 2026 Red Hat, Inc.