[PATCH] iotests/check: move general long options to double dash

[PATCH] iotests/check: move general long options to double dash

[Vladimir Sementsov-Ogievskiy]

So, the change:

  -makecheck -> --makecheck
  -valgrind  -> --valgrind
  -nocache   -> --nocache
  -misalign  -> --misalign

Motivation:

1. Several long options are already have double dash:
  --dry-run, --color, --groups, --exclude-groups, --start-from

2. -misalign is used to add --misalign option (two dashes) to qemu-io

3. qemu-io and qemu-nbd has --nocache option (two dashes)

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 tests/check-block.sh         | 2 +-
 tests/qemu-iotests/check     | 8 ++++----
 tests/qemu-iotests/common.rc | 4 ++--
 3 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/tests/check-block.sh b/tests/check-block.sh
index f86cb863de..90619454d3 100755
--- a/tests/check-block.sh
+++ b/tests/check-block.sh
@@ -77,7 +77,7 @@ export PYTHONUTF8=1
 
 ret=0
 for fmt in $format_list ; do
-    ${PYTHON} ./check -makecheck -$fmt $group || ret=1
+    ${PYTHON} ./check --makecheck -$fmt $group || ret=1
 done
 
 exit $ret
diff --git a/tests/qemu-iotests/check b/tests/qemu-iotests/check
index 2dd529eb75..3f3962dd75 100755
--- a/tests/qemu-iotests/check
+++ b/tests/qemu-iotests/check
@@ -32,11 +32,11 @@ def make_argparser() -> argparse.ArgumentParser:
 
     p.add_argument('-n', '--dry-run', action='store_true',
                    help='show me, do not run tests')
-    p.add_argument('-makecheck', action='store_true',
+    p.add_argument('--makecheck', action='store_true',
                    help='pretty print output for make check')
 
     p.add_argument('-d', dest='debug', action='store_true', help='debug')
-    p.add_argument('-misalign', action='store_true',
+    p.add_argument('--misalign', action='store_true',
                    help='misalign memory allocations')
     p.add_argument('--color', choices=['on', 'off', 'auto'],
                    default='auto', help="use terminal colors. The default "
@@ -46,7 +46,7 @@ def make_argparser() -> argparse.ArgumentParser:
     mg = g_env.add_mutually_exclusive_group()
     # We don't set default for cachemode, as we need to distinguish default
     # from user input later.
-    mg.add_argument('-nocache', dest='cachemode', action='store_const',
+    mg.add_argument('--nocache', dest='cachemode', action='store_const',
                     const='none', help='set cache mode "none" (O_DIRECT), '
                     'sets CACHEMODE environment variable')
     mg.add_argument('-c', dest='cachemode',
@@ -85,7 +85,7 @@ def make_argparser() -> argparse.ArgumentParser:
     g_bash.add_argument('-o', dest='imgopts',
                         help='options to pass to qemu-img create/convert, '
                         'sets IMGOPTS environment variable')
-    g_bash.add_argument('-valgrind', action='store_true',
+    g_bash.add_argument('--valgrind', action='store_true',
                         help='use valgrind, sets VALGRIND_QEMU environment '
                         'variable')
 
diff --git a/tests/qemu-iotests/common.rc b/tests/qemu-iotests/common.rc
index cbbf6d7c7f..e2f81cd41b 100644
--- a/tests/qemu-iotests/common.rc
+++ b/tests/qemu-iotests/common.rc
@@ -124,7 +124,7 @@ fi
 
 # Set the variables to the empty string to turn Valgrind off
 # for specific processes, e.g.
-# $ VALGRIND_QEMU_IO= ./check -qcow2 -valgrind 015
+# $ VALGRIND_QEMU_IO= ./check -qcow2 --valgrind 015
 
 : ${VALGRIND_QEMU_VM=$VALGRIND_QEMU}
 : ${VALGRIND_QEMU_IMG=$VALGRIND_QEMU}
@@ -134,7 +134,7 @@ fi
 
 # The Valgrind own parameters may be set with
 # its environment variable VALGRIND_OPTS, e.g.
-# $ VALGRIND_OPTS="--leak-check=yes" ./check -qcow2 -valgrind 015
+# $ VALGRIND_OPTS="--leak-check=yes" ./check -qcow2 --valgrind 015
 
 _qemu_proc_exec()
 {
-- 
2.29.2

Re: [PATCH] iotests/check: move general long options to double dash

[Emanuele Giuseppe Esposito]

On 26/05/2021 20:16, Vladimir Sementsov-Ogievskiy wrote:
> So, the change:
> 
>    -makecheck -> --makecheck
>    -valgrind  -> --valgrind
>    -nocache   -> --nocache
>    -misalign  -> --misalign
> 
> Motivation:
> 
> 1. Several long options are already have double dash:
>    --dry-run, --color, --groups, --exclude-groups, --start-from
> 
> 2. -misalign is used to add --misalign option (two dashes) to qemu-io
> 
> 3. qemu-io and qemu-nbd has --nocache option (two dashes)
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>   tests/check-block.sh         | 2 +-
>   tests/qemu-iotests/check     | 8 ++++----
>   tests/qemu-iotests/common.rc | 4 ++--
>   3 files changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/tests/check-block.sh b/tests/check-block.sh
> index f86cb863de..90619454d3 100755
> --- a/tests/check-block.sh
> +++ b/tests/check-block.sh
> @@ -77,7 +77,7 @@ export PYTHONUTF8=1
>   
>   ret=0
>   for fmt in $format_list ; do
> -    ${PYTHON} ./check -makecheck -$fmt $group || ret=1
> +    ${PYTHON} ./check --makecheck -$fmt $group || ret=1
>   done
>   
>   exit $ret
> diff --git a/tests/qemu-iotests/check b/tests/qemu-iotests/check
> index 2dd529eb75..3f3962dd75 100755
> --- a/tests/qemu-iotests/check
> +++ b/tests/qemu-iotests/check
> @@ -32,11 +32,11 @@ def make_argparser() -> argparse.ArgumentParser:
>   
>       p.add_argument('-n', '--dry-run', action='store_true',
>                      help='show me, do not run tests')
> -    p.add_argument('-makecheck', action='store_true',
> +    p.add_argument('--makecheck', action='store_true',
>                      help='pretty print output for make check')
>   
>       p.add_argument('-d', dest='debug', action='store_true', help='debug')
> -    p.add_argument('-misalign', action='store_true',
> +    p.add_argument('--misalign', action='store_true',
>                      help='misalign memory allocations')
>       p.add_argument('--color', choices=['on', 'off', 'auto'],
>                      default='auto', help="use terminal colors. The default "
> @@ -46,7 +46,7 @@ def make_argparser() -> argparse.ArgumentParser:
>       mg = g_env.add_mutually_exclusive_group()
>       # We don't set default for cachemode, as we need to distinguish default
>       # from user input later.
> -    mg.add_argument('-nocache', dest='cachemode', action='store_const',
> +    mg.add_argument('--nocache', dest='cachemode', action='store_const',
>                       const='none', help='set cache mode "none" (O_DIRECT), '
>                       'sets CACHEMODE environment variable')
>       mg.add_argument('-c', dest='cachemode',
> @@ -85,7 +85,7 @@ def make_argparser() -> argparse.ArgumentParser:
>       g_bash.add_argument('-o', dest='imgopts',
>                           help='options to pass to qemu-img create/convert, '
>                           'sets IMGOPTS environment variable')
> -    g_bash.add_argument('-valgrind', action='store_true',
> +    g_bash.add_argument('--valgrind', action='store_true',
>                           help='use valgrind, sets VALGRIND_QEMU environment '
>                           'variable')
>   
> diff --git a/tests/qemu-iotests/common.rc b/tests/qemu-iotests/common.rc
> index cbbf6d7c7f..e2f81cd41b 100644
> --- a/tests/qemu-iotests/common.rc
> +++ b/tests/qemu-iotests/common.rc
> @@ -124,7 +124,7 @@ fi
>   
>   # Set the variables to the empty string to turn Valgrind off
>   # for specific processes, e.g.
> -# $ VALGRIND_QEMU_IO= ./check -qcow2 -valgrind 015
> +# $ VALGRIND_QEMU_IO= ./check -qcow2 --valgrind 015
>   
>   : ${VALGRIND_QEMU_VM=$VALGRIND_QEMU}
>   : ${VALGRIND_QEMU_IMG=$VALGRIND_QEMU}
> @@ -134,7 +134,7 @@ fi
>   
>   # The Valgrind own parameters may be set with
>   # its environment variable VALGRIND_OPTS, e.g.
> -# $ VALGRIND_OPTS="--leak-check=yes" ./check -qcow2 -valgrind 015
> +# $ VALGRIND_OPTS="--leak-check=yes" ./check -qcow2 --valgrind 015

Ok I see why the short options do not make sense to have double dash, 
but if we want full consistency why fmt is left with one dash? Like 
"-qcow2", should we also change that? (that change might be more complex 
to do)

Thank you,
Emanuele

Re: [PATCH] iotests/check: move general long options to double dash

[Vladimir Sementsov-Ogievskiy]

03.06.2021 13:21, Emanuele Giuseppe Esposito wrote:
> 
> 
> On 26/05/2021 20:16, Vladimir Sementsov-Ogievskiy wrote:
>> So, the change:
>>
>>    -makecheck -> --makecheck
>>    -valgrind  -> --valgrind
>>    -nocache   -> --nocache
>>    -misalign  -> --misalign
>>
>> Motivation:
>>
>> 1. Several long options are already have double dash:
>>    --dry-run, --color, --groups, --exclude-groups, --start-from
>>
>> 2. -misalign is used to add --misalign option (two dashes) to qemu-io
>>
>> 3. qemu-io and qemu-nbd has --nocache option (two dashes)
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>   tests/check-block.sh         | 2 +-
>>   tests/qemu-iotests/check     | 8 ++++----
>>   tests/qemu-iotests/common.rc | 4 ++--
>>   3 files changed, 7 insertions(+), 7 deletions(-)
>>
>> diff --git a/tests/check-block.sh b/tests/check-block.sh
>> index f86cb863de..90619454d3 100755
>> --- a/tests/check-block.sh
>> +++ b/tests/check-block.sh
>> @@ -77,7 +77,7 @@ export PYTHONUTF8=1
>>   ret=0
>>   for fmt in $format_list ; do
>> -    ${PYTHON} ./check -makecheck -$fmt $group || ret=1
>> +    ${PYTHON} ./check --makecheck -$fmt $group || ret=1
>>   done
>>   exit $ret
>> diff --git a/tests/qemu-iotests/check b/tests/qemu-iotests/check
>> index 2dd529eb75..3f3962dd75 100755
>> --- a/tests/qemu-iotests/check
>> +++ b/tests/qemu-iotests/check
>> @@ -32,11 +32,11 @@ def make_argparser() -> argparse.ArgumentParser:
>>       p.add_argument('-n', '--dry-run', action='store_true',
>>                      help='show me, do not run tests')
>> -    p.add_argument('-makecheck', action='store_true',
>> +    p.add_argument('--makecheck', action='store_true',
>>                      help='pretty print output for make check')
>>       p.add_argument('-d', dest='debug', action='store_true', help='debug')
>> -    p.add_argument('-misalign', action='store_true',
>> +    p.add_argument('--misalign', action='store_true',
>>                      help='misalign memory allocations')
>>       p.add_argument('--color', choices=['on', 'off', 'auto'],
>>                      default='auto', help="use terminal colors. The default "
>> @@ -46,7 +46,7 @@ def make_argparser() -> argparse.ArgumentParser:
>>       mg = g_env.add_mutually_exclusive_group()
>>       # We don't set default for cachemode, as we need to distinguish default
>>       # from user input later.
>> -    mg.add_argument('-nocache', dest='cachemode', action='store_const',
>> +    mg.add_argument('--nocache', dest='cachemode', action='store_const',
>>                       const='none', help='set cache mode "none" (O_DIRECT), '
>>                       'sets CACHEMODE environment variable')
>>       mg.add_argument('-c', dest='cachemode',
>> @@ -85,7 +85,7 @@ def make_argparser() -> argparse.ArgumentParser:
>>       g_bash.add_argument('-o', dest='imgopts',
>>                           help='options to pass to qemu-img create/convert, '
>>                           'sets IMGOPTS environment variable')
>> -    g_bash.add_argument('-valgrind', action='store_true',
>> +    g_bash.add_argument('--valgrind', action='store_true',
>>                           help='use valgrind, sets VALGRIND_QEMU environment '
>>                           'variable')
>> diff --git a/tests/qemu-iotests/common.rc b/tests/qemu-iotests/common.rc
>> index cbbf6d7c7f..e2f81cd41b 100644
>> --- a/tests/qemu-iotests/common.rc
>> +++ b/tests/qemu-iotests/common.rc
>> @@ -124,7 +124,7 @@ fi
>>   # Set the variables to the empty string to turn Valgrind off
>>   # for specific processes, e.g.
>> -# $ VALGRIND_QEMU_IO= ./check -qcow2 -valgrind 015
>> +# $ VALGRIND_QEMU_IO= ./check -qcow2 --valgrind 015
>>   : ${VALGRIND_QEMU_VM=$VALGRIND_QEMU}
>>   : ${VALGRIND_QEMU_IMG=$VALGRIND_QEMU}
>> @@ -134,7 +134,7 @@ fi
>>   # The Valgrind own parameters may be set with
>>   # its environment variable VALGRIND_OPTS, e.g.
>> -# $ VALGRIND_OPTS="--leak-check=yes" ./check -qcow2 -valgrind 015
>> +# $ VALGRIND_OPTS="--leak-check=yes" ./check -qcow2 --valgrind 015
> 
> Ok I see why the short options do not make sense to have double dash, but if we want full consistency why fmt is left with one dash? Like "-qcow2", should we also change that? (that change might be more complex to do)
> 

I was afraid that changing format and protocol options would be more painful, as they are used often. And decided that we still can get more consistency, keeping other options similar..

-- 
Best regards,
Vladimir

Re: [PATCH] iotests/check: move general long options to double dash

[Paolo Bonzini]

On 26/05/21 20:16, Vladimir Sementsov-Ogievskiy wrote:
> So, the change:
> 
>    -makecheck -> --makecheck
>    -valgrind  -> --valgrind
>    -nocache   -> --nocache
>    -misalign  -> --misalign
> 
> Motivation:
> 
> 1. Several long options are already have double dash:
>    --dry-run, --color, --groups, --exclude-groups, --start-from
> 
> 2. -misalign is used to add --misalign option (two dashes) to qemu-io
> 
> 3. qemu-io and qemu-nbd has --nocache option (two dashes)

Just like for QEMU, let me reiterate that this is generally not an 
improvement.

Double-dash options give extra information to the user that short 
(single-dash) options can be combined, while this is not the case for 
iotests/check.

Paolo

Re: [PATCH] iotests/check: move general long options to double dash

[Vladimir Sementsov-Ogievskiy]

03.06.2021 14:38, Paolo Bonzini wrote:
> On 26/05/21 20:16, Vladimir Sementsov-Ogievskiy wrote:
>> So, the change:
>>
>>    -makecheck -> --makecheck
>>    -valgrind  -> --valgrind
>>    -nocache   -> --nocache
>>    -misalign  -> --misalign
>>
>> Motivation:
>>
>> 1. Several long options are already have double dash:
>>    --dry-run, --color, --groups, --exclude-groups, --start-from
>>
>> 2. -misalign is used to add --misalign option (two dashes) to qemu-io
>>
>> 3. qemu-io and qemu-nbd has --nocache option (two dashes)
> 
> Just like for QEMU, let me reiterate that this is generally not an improvement.
> 
> Double-dash options give extra information to the user that short (single-dash) options can be combined, while this is not the case for iotests/check.

You can combine short options for check script, as argparse supports it.

We don't have many short options yet.. But something like

./check -ng auto

makes sense and works..


-- 
Best regards,
Vladimir

Re: [PATCH] iotests/check: move general long options to double dash

[Paolo Bonzini]

On 03/06/21 14:19, Vladimir Sementsov-Ogievskiy wrote:
>>
>> Double-dash options give extra information to the user that short 
>> (single-dash) options can be combined, while this is not the case for 
>> iotests/check.
> 
> You can combine short options for check script, as argparse supports it.
> 
> We don't have many short options yet.. But something like
> 
> ./check -ng auto
> 
> makes sense and works..

Oh, I missed that.  Then it's okay, thanks!

paolo

Re: [PATCH] iotests/check: move general long options to double dash

[Vladimir Sementsov-Ogievskiy]

04.06.2021 10:19, Paolo Bonzini wrote:
> On 03/06/21 14:19, Vladimir Sementsov-Ogievskiy wrote:
>>>
>>> Double-dash options give extra information to the user that short (single-dash) options can be combined, while this is not the case for iotests/check.
>>
>> You can combine short options for check script, as argparse supports it.
>>
>> We don't have many short options yet.. But something like
>>
>> ./check -ng auto
>>
>> makes sense and works..
> 
> Oh, I missed that.  Then it's okay, thanks!
> 

Actually, I understand that my arguing against -gdb was time wasting nit-picking, sorry for that :\

When I've rewritten check into python, I decided that "I like double-dash options, they are modern and more usual and look nice", and used double dash for new options.. Nobody complained, so now we have inconsistent options, thanks to me :( Probably, I should have added new options with one dash. That's all not really significant, as check script is only a testing tool.. Still, inconsistency never helps.

Anyway now we have what we have.

So, there are some ways to improve the situation:

1. Take this patch and do nothing more.

Pros: as said in commit message, more consistency with qemu-io and qemu-nbd.
Cons: we still have -qcow2, -nbd, -raw, etc format and protocol options

2. Take this patch and also convert protocol and format options

Pros: everything is consistent and use two dashes, so we can safely use combining short options syntax
Cons: more pain for developers to write --qcow2 instead of -qcow2 every time. What actually stopped me of posting that patch (converting protocol and format options), I imagined the heavy extra load on all block-layer developers right pinky to push '-' one time more :))

3. Do nothing at all
Cons: all these inconsistent options

4. Convert new options to one dash
Pros: less pain of converting, as new options should be rarely used (unlike -qcow2 or -nbd),  and we have consistent set of options
Const: inconsistency with qemu-io and qemu-nbd
        ambiguity in using combined short options (note also, that starting from Python 3.8 combining short options can't be disabled)*
        

So, I'm OK with either way and can make patches. But I don't want to be the only person who makes a decision. So, let's wait for opinions, and if nobody really interested, go the default way [3].



* looking at https://docs.python.org/3/library/argparse.html I see:

Changed in version 3.5: allow_abbrev parameter was added.

Changed in version 3.8: In previous versions, allow_abbrev also disabled grouping of short flags such as -vv to mean -v -v.

-- 
Best regards,
Vladimir

Re: [PATCH] iotests/check: move general long options to double dash

[Eric Blake]

On Fri, Jun 04, 2021 at 11:25:16AM +0300, Vladimir Sementsov-Ogievskiy wrote:
> So, there are some ways to improve the situation:

My personal preference (although I'm fine with any of your listed
options, if others speak up in favor of a different one):

> 2. Take this patch and also convert protocol and format options
> 
> Pros: everything is consistent and use two dashes, so we can safely use combining short options syntax
> Cons: more pain for developers to write --qcow2 instead of -qcow2 every time. What actually stopped me of posting that patch (converting protocol and format options), I imagined the heavy extra load on all block-layer developers right pinky to push '-' one time more :))

I don't mind typing an extra - for './check --qcow2'.  I agree it will
cause some temporary learning curve when I type the short way and it
fails, but as long as the error message is good, I don't see a problem
in changing the interface since this is a developer-only tool.

> So, I'm OK with either way and can make patches. But I don't want to be the only person who makes a decision. So, let's wait for opinions, and if nobody really interested, go the default way [3].

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org