tests/check-block.sh | 2 +- tests/qemu-iotests/check | 8 ++++---- tests/qemu-iotests/common.rc | 4 ++-- 3 files changed, 7 insertions(+), 7 deletions(-)
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
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
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
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
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
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
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
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
© 2016 - 2024 Red Hat, Inc.