Re: [PATCH v4 04/15] qemu-iotests: add option to attach gdbserver

[Emanuele Giuseppe Esposito <eesposit@redhat.com>]

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.
> 
>