| Message ID | 20180426161958.2872-4-rkagan@virtuozzo.com |
|---|---|
| State | New |
| Headers | show |
| Series | iotests: don't choke on disabled drivers | expand |
On 2018-04-26 18:19, Roman Kagan wrote: > Add helper functions to query the block drivers actually supported by > QEMU using "-drive format=?". This allows to skip certain tests that > require drivers not built in or whitelisted in QEMU. > > Signed-off-by: Roman Kagan <rkagan@virtuozzo.com> > --- > tests/qemu-iotests/common.rc | 19 +++++++++++++++++++ > tests/qemu-iotests/iotests.py | 30 +++++++++++++++++++++++++++--- > 2 files changed, 46 insertions(+), 3 deletions(-) [...] > diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py > index e2abf0cb53..698ef2b2c0 100644 > --- a/tests/qemu-iotests/iotests.py > +++ b/tests/qemu-iotests/iotests.py [...] > @@ -550,13 +561,26 @@ def verify_cache_mode(supported_cache_modes=[]): > if supported_cache_modes and (cachemode not in supported_cache_modes): > notrun('not suitable for this cache mode: %s' % cachemode) > > +rw_formats = None > + > +def supports_format(format_name): > + format_message = qemu_pipe('-drive', 'format=?') > + global rw_formats > + if rw_formats is None: > + rw_formats = format_message.splitlines()[0].split(':')[1].split() Isn't it sufficient to call qemu_pipe() only if rw_formats is None? The rest looks good. Max > + return format_name in rw_formats > + > +def require_formats(*formats): > + for fmt in formats: > + if not supports_format(fmt): > + notrun('%s does not support format %s' % (qemu_prog, fmt)) > + > def supports_quorum(): > - return 'quorum' in qemu_img_pipe('--help') > + return supports_format('quorum') > > def verify_quorum(): > '''Skip test suite if quorum support is not available''' > - if not supports_quorum(): > - notrun('quorum support missing') > + require_formats('quorum') > > def main(supported_fmts=[], supported_oses=['linux'], supported_cache_modes=[], > unsupported_fmts=[]): >
On Wed, May 30, 2018 at 02:17:55PM +0200, Max Reitz wrote: > On 2018-04-26 18:19, Roman Kagan wrote: > > @@ -550,13 +561,26 @@ def verify_cache_mode(supported_cache_modes=[]): > > if supported_cache_modes and (cachemode not in supported_cache_modes): > > notrun('not suitable for this cache mode: %s' % cachemode) > > > > +rw_formats = None > > + > > +def supports_format(format_name): > > + format_message = qemu_pipe('-drive', 'format=?') > > + global rw_formats > > + if rw_formats is None: > > + rw_formats = format_message.splitlines()[0].split(':')[1].split() > > Isn't it sufficient to call qemu_pipe() only if rw_formats is None? Sure it is, and this was exactly my intention with this global, but I managed to forget moving the most relevant part under 'if' :$ Thanks for catching, Roman.
Roman Kagan <rkagan@virtuozzo.com> writes: > Add helper functions to query the block drivers actually supported by > QEMU using "-drive format=?". This allows to skip certain tests that > require drivers not built in or whitelisted in QEMU. > > Signed-off-by: Roman Kagan <rkagan@virtuozzo.com> > --- > tests/qemu-iotests/common.rc | 19 +++++++++++++++++++ > tests/qemu-iotests/iotests.py | 30 +++++++++++++++++++++++++++--- > 2 files changed, 46 insertions(+), 3 deletions(-) > > diff --git a/tests/qemu-iotests/common.rc b/tests/qemu-iotests/common.rc > index 9a65a11026..fe5a4d1cfd 100644 > --- a/tests/qemu-iotests/common.rc > +++ b/tests/qemu-iotests/common.rc > @@ -493,5 +493,24 @@ _require_command() > [ -x "$c" ] || _notrun "$1 utility required, skipped this test" > } > > +# this test requires support for specific formats > +# > +_require_format() > +{ > + supported_formats=$($QEMU_PROG $QEMU_OPTIONS -drive format=\? 2>&1 | \ Use of '?' to get help is deprecated. Please use 'format=help', and update your commit message accordingly. > + head -1 | cut -d : -f 2) > + for f; do > + found=false > + for sf in $supported_formats; do > + if [ "$f" = "$sf" ]; then > + found=true > + break > + fi > + done > + > + $found || _notrun "$QEMU_PROG doesn't support format $f" > + done > +} > + > # make sure this script returns success > true > diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py > index e2abf0cb53..698ef2b2c0 100644 > --- a/tests/qemu-iotests/iotests.py > +++ b/tests/qemu-iotests/iotests.py > @@ -119,6 +119,17 @@ def qemu_io(*args): > sys.stderr.write('qemu-io received signal %i: %s\n' % (-subp.returncode, ' '.join(args))) > return output > > +def qemu_pipe(*args): > + '''Run qemu with an option to print something and exit (e.g. a help option), > + and return its output''' > + args = [qemu_prog] + qemu_opts + list(args) > + subp = subprocess.Popen(args, stdout=subprocess.PIPE, > + stderr=subprocess.STDOUT) > + output = subp.communicate()[0] > + if subp.returncode < 0: > + sys.stderr.write('qemu received signal %i: %s\n' % (-subp.returncode, ' '.join(args))) > + return output > + > > class QemuIoInteractive: > def __init__(self, *args): > @@ -550,13 +561,26 @@ def verify_cache_mode(supported_cache_modes=[]): > if supported_cache_modes and (cachemode not in supported_cache_modes): > notrun('not suitable for this cache mode: %s' % cachemode) > > +rw_formats = None > + > +def supports_format(format_name): > + format_message = qemu_pipe('-drive', 'format=?') Likewise. > + global rw_formats > + if rw_formats is None: > + rw_formats = format_message.splitlines()[0].split(':')[1].split() > + return format_name in rw_formats > + > +def require_formats(*formats): > + for fmt in formats: > + if not supports_format(fmt): > + notrun('%s does not support format %s' % (qemu_prog, fmt)) > + > def supports_quorum(): > - return 'quorum' in qemu_img_pipe('--help') > + return supports_format('quorum') > > def verify_quorum(): > '''Skip test suite if quorum support is not available''' > - if not supports_quorum(): > - notrun('quorum support missing') > + require_formats('quorum') > > def main(supported_fmts=[], supported_oses=['linux'], supported_cache_modes=[], > unsupported_fmts=[]):
On 04.06.2018 09:18, Markus Armbruster wrote: > Roman Kagan <rkagan@virtuozzo.com> writes: > >> Add helper functions to query the block drivers actually supported by >> QEMU using "-drive format=?". This allows to skip certain tests that >> require drivers not built in or whitelisted in QEMU. >> >> Signed-off-by: Roman Kagan <rkagan@virtuozzo.com> >> --- >> tests/qemu-iotests/common.rc | 19 +++++++++++++++++++ >> tests/qemu-iotests/iotests.py | 30 +++++++++++++++++++++++++++--- >> 2 files changed, 46 insertions(+), 3 deletions(-) >> >> diff --git a/tests/qemu-iotests/common.rc b/tests/qemu-iotests/common.rc >> index 9a65a11026..fe5a4d1cfd 100644 >> --- a/tests/qemu-iotests/common.rc >> +++ b/tests/qemu-iotests/common.rc >> @@ -493,5 +493,24 @@ _require_command() >> [ -x "$c" ] || _notrun "$1 utility required, skipped this test" >> } >> >> +# this test requires support for specific formats >> +# >> +_require_format() >> +{ >> + supported_formats=$($QEMU_PROG $QEMU_OPTIONS -drive format=\? 2>&1 | \ > > Use of '?' to get help is deprecated. Please use 'format=help', and > update your commit message accordingly. Is it? Where did we document that? Thomas
On 06/04/2018 05:34 AM, Thomas Huth wrote: > On 04.06.2018 09:18, Markus Armbruster wrote: >> Roman Kagan <rkagan@virtuozzo.com> writes: >> >>> Add helper functions to query the block drivers actually supported by >>> QEMU using "-drive format=?". This allows to skip certain tests that >>> require drivers not built in or whitelisted in QEMU. >>> >>> + supported_formats=$($QEMU_PROG $QEMU_OPTIONS -drive format=\? 2>&1 | \ >> >> Use of '?' to get help is deprecated. Please use 'format=help', and >> update your commit message accordingly. > > Is it? Where did we document that? Hmm, we haven't documented it yet, but it's been that way since commit c8057f95, in Aug 2012, when we added 'help' as the preferred synonym, and have tried to avoid mention of '?' in new documentation (as it requires additional shell quoting). I'm guessing we'll probably see a patch from you to start an official deprecation window?
On 05.06.2018 00:40, Eric Blake wrote: > On 06/04/2018 05:34 AM, Thomas Huth wrote: >> On 04.06.2018 09:18, Markus Armbruster wrote: >>> Roman Kagan <rkagan@virtuozzo.com> writes: >>> >>>> Add helper functions to query the block drivers actually supported by >>>> QEMU using "-drive format=?". This allows to skip certain tests that >>>> require drivers not built in or whitelisted in QEMU. >>>> > >>>> + supported_formats=$($QEMU_PROG $QEMU_OPTIONS -drive format=\? >>>> 2>&1 | \ >>> >>> Use of '?' to get help is deprecated. Please use 'format=help', and >>> update your commit message accordingly. >> >> Is it? Where did we document that? > > Hmm, we haven't documented it yet, but it's been that way since commit > c8057f95, in Aug 2012, when we added 'help' as the preferred synonym, > and have tried to avoid mention of '?' in new documentation (as it > requires additional shell quoting). I'm guessing we'll probably see a > patch from you to start an official deprecation window? I'm using '?' regularly on my own, so don't expect a patch from my side here ;-) Anyway, we still use the question mark in our documentation, e.g.: options is a comma separated list of format specific options in a name=value format. Use -o ? for an overview of the options supported by the used format or see the format descriptions below for details. or: -b, --blacklist=list Comma-separated list of RPCs to disable (no spaces, ‘?’ to list available RPCs) So calling it deprecated sounds wrong to me. Thomas
Thomas Huth <thuth@redhat.com> writes: > On 05.06.2018 00:40, Eric Blake wrote: >> On 06/04/2018 05:34 AM, Thomas Huth wrote: >>> On 04.06.2018 09:18, Markus Armbruster wrote: >>>> Roman Kagan <rkagan@virtuozzo.com> writes: >>>> >>>>> Add helper functions to query the block drivers actually supported by >>>>> QEMU using "-drive format=?". This allows to skip certain tests that >>>>> require drivers not built in or whitelisted in QEMU. >>>>> >> >>>>> + supported_formats=$($QEMU_PROG $QEMU_OPTIONS -drive format=\? >>>>> 2>&1 | \ >>>> >>>> Use of '?' to get help is deprecated. Please use 'format=help', and >>>> update your commit message accordingly. >>> >>> Is it? Where did we document that? >> >> Hmm, we haven't documented it yet, but it's been that way since commit >> c8057f95, in Aug 2012, when we added 'help' as the preferred synonym, >> and have tried to avoid mention of '?' in new documentation (as it >> requires additional shell quoting). I'm guessing we'll probably see a >> patch from you to start an official deprecation window? > > I'm using '?' regularly on my own, so don't expect a patch from > my side here ;-) > Anyway, we still use the question mark in our documentation, e.g.: > > options > > is a comma separated list of format specific options in a name=value format. > Use -o ? for an overview of the options supported by the used format or see > the format descriptions below for details. > > or: > > -b, --blacklist=list > > Comma-separated list of RPCs to disable (no spaces, ‘?’ to list available RPCs) These are bugs, and we need to fix them. > So calling it deprecated sounds wrong to me. Our intent to deprecate it was pretty clear in commit c8057f95: /** * is_help_option: * @s: string to test * * Check whether @s is one of the standard strings which indicate * that the user is asking for a list of the valid values for a * command option like -cpu or -M. The current accepted strings --> * are 'help' and '?'. '?' is deprecated (it is a shell wildcard * which makes it annoying to use in a reliable way) but provided * for backwards compatibility. * * Returns: true if @s is a request for a list. */ static inline bool is_help_option(const char *s) Predates today's more formal deprecation policy. Simpler times. There's no real need to kill off '?', unless it gets in the way of steering people towards 'help'. We should steer them toward 'help', because '?' is a trap for insufficiently sophisticated users of shell[*]. Every use of '?' in our source tree works against that goal, and thus works towards a removal of '?'. [*] In the wider sense, approximately 99.99% of shell users are insufficiently sophisticated, including myself. In the narrower sense of being prone to fall into the '?' trap, the fraction is lower, but still significant.
On 07.06.2018 08:57, Markus Armbruster wrote: > Thomas Huth <thuth@redhat.com> writes: > >> On 05.06.2018 00:40, Eric Blake wrote: >>> On 06/04/2018 05:34 AM, Thomas Huth wrote: >>>> On 04.06.2018 09:18, Markus Armbruster wrote: >>>>> Roman Kagan <rkagan@virtuozzo.com> writes: >>>>> >>>>>> Add helper functions to query the block drivers actually supported by >>>>>> QEMU using "-drive format=?". This allows to skip certain tests that >>>>>> require drivers not built in or whitelisted in QEMU. >>>>>> >>> >>>>>> + supported_formats=$($QEMU_PROG $QEMU_OPTIONS -drive format=\? >>>>>> 2>&1 | \ >>>>> >>>>> Use of '?' to get help is deprecated. Please use 'format=help', and >>>>> update your commit message accordingly. >>>> >>>> Is it? Where did we document that? >>> >>> Hmm, we haven't documented it yet, but it's been that way since commit >>> c8057f95, in Aug 2012, when we added 'help' as the preferred synonym, >>> and have tried to avoid mention of '?' in new documentation (as it >>> requires additional shell quoting). I'm guessing we'll probably see a >>> patch from you to start an official deprecation window? >> >> I'm using '?' regularly on my own, so don't expect a patch from >> my side here ;-) >> Anyway, we still use the question mark in our documentation, e.g.: >> >> options >> >> is a comma separated list of format specific options in a name=value format. >> Use -o ? for an overview of the options supported by the used format or see >> the format descriptions below for details. >> >> or: >> >> -b, --blacklist=list >> >> Comma-separated list of RPCs to disable (no spaces, ‘?’ to list available RPCs) > > These are bugs, and we need to fix them. > >> So calling it deprecated sounds wrong to me. > > Our intent to deprecate it was pretty clear in commit c8057f95: > > /** > * is_help_option: > * @s: string to test > * > * Check whether @s is one of the standard strings which indicate > * that the user is asking for a list of the valid values for a > * command option like -cpu or -M. The current accepted strings > --> * are 'help' and '?'. '?' is deprecated (it is a shell wildcard > * which makes it annoying to use in a reliable way) but provided > * for backwards compatibility. > * > * Returns: true if @s is a request for a list. > */ > static inline bool is_help_option(const char *s) > > Predates today's more formal deprecation policy. Simpler times. Sure, but finding such messages in the sources can be quite cumbersome... > There's no real need to kill off '?', unless it gets in the way of > steering people towards 'help'. We should steer them toward 'help', > because '?' is a trap for insufficiently sophisticated users of > shell[*]. ... and I agree with your points here. => I think we need a second list of deprecated feature (maybe we should call them "legacy features" instead of "deprecated"?), i.e. a list of features which we don't recommend for new code / scripts anymore, but which we do not intend to remove via our official deprecation policy any time soon. Things like "--enable-kvm" / "-no-kvm" or maybe even "-net" go into the same category. If you agree, I can try to come up with a patch (should the list go into qemu-doc.texi or a separate document in the the documentation folder?). Thomas
On 07/06/2018 09:50, Thomas Huth wrote: > >> There's no real need to kill off '?', unless it gets in the way of >> steering people towards 'help'. We should steer them toward 'help', >> because '?' is a trap for insufficiently sophisticated users of >> shell[*]. > ... and I agree with your points here. > > => I think we need a second list of deprecated feature (maybe we should > call them "legacy features" instead of "deprecated"?), i.e. a list of > features which we don't recommend for new code / scripts anymore, but > which we do not intend to remove via our official deprecation policy any > time soon. Things like "--enable-kvm" / "-no-kvm" or maybe even "-net" > go into the same category. Yes, "-net" definitely goes there. > If you agree, I can try to come up with a patch (should the list go into > qemu-doc.texi or a separate document in the the documentation folder?). I think it should go in docs/devel. Paolo
On 07.06.2018 13:07, Paolo Bonzini wrote: > On 07/06/2018 09:50, Thomas Huth wrote: >> >>> There's no real need to kill off '?', unless it gets in the way of >>> steering people towards 'help'. We should steer them toward 'help', >>> because '?' is a trap for insufficiently sophisticated users of >>> shell[*]. >> ... and I agree with your points here. >> >> => I think we need a second list of deprecated feature (maybe we should >> call them "legacy features" instead of "deprecated"?), i.e. a list of >> features which we don't recommend for new code / scripts anymore, but >> which we do not intend to remove via our official deprecation policy any >> time soon. Things like "--enable-kvm" / "-no-kvm" or maybe even "-net" >> go into the same category. > > Yes, "-net" definitely goes there. > >> If you agree, I can try to come up with a patch (should the list go into >> qemu-doc.texi or a separate document in the the documentation folder?). > > I think it should go in docs/devel. I currently rather tend to put it into a new appendix in qemu-doc.texi, since this is useful information for the normal users, too. Or how shall we communicate this to the users that the old options that they are used to are still there, but should not be used for new scripts anymore? Thomas
On 07/06/2018 13:10, Thomas Huth wrote: > On 07.06.2018 13:07, Paolo Bonzini wrote: >> On 07/06/2018 09:50, Thomas Huth wrote: >>> >>>> There's no real need to kill off '?', unless it gets in the way of >>>> steering people towards 'help'. We should steer them toward 'help', >>>> because '?' is a trap for insufficiently sophisticated users of >>>> shell[*]. >>> ... and I agree with your points here. >>> >>> => I think we need a second list of deprecated feature (maybe we should >>> call them "legacy features" instead of "deprecated"?), i.e. a list of >>> features which we don't recommend for new code / scripts anymore, but >>> which we do not intend to remove via our official deprecation policy any >>> time soon. Things like "--enable-kvm" / "-no-kvm" or maybe even "-net" >>> go into the same category. >> >> Yes, "-net" definitely goes there. >> >>> If you agree, I can try to come up with a patch (should the list go into >>> qemu-doc.texi or a separate document in the the documentation folder?). >> >> I think it should go in docs/devel. > > I currently rather tend to put it into a new appendix in qemu-doc.texi, > since this is useful information for the normal users, too. Or how shall > we communicate this to the users that the old options that they are used > to are still there, but should not be used for new scripts anymore? I think we should tell them directly in the text for things like "-net". The new document would put things together and be mostly about command-line (anti)patterns. As to "-enable-kvm", I don't see anything wrong with users using it, or even with occasionally adding more options like it. However, we should warn developers that such simple options should be syntactic sugar for a structured (i.e. QemuOpts-based) option like "-accel", and that it should only be done for similarity with existing options. Basically the same reason why new options have both "?" and "help", even though "?" is disliked. Paolo
On 07.06.2018 13:18, Paolo Bonzini wrote: [...] > As to "-enable-kvm", I don't see anything wrong with users using it, or > even with occasionally adding more options like it. However, we should > warn developers that such simple options should be syntactic sugar for a > structured (i.e. QemuOpts-based) option like "-accel", and that it > should only be done for similarity with existing options. Honestly, in this case I think it's just confusing for the normal users, and not sugar (anymore). If I'm an unexperienced user who wants to enable KVM, and I see multiple options that seem to be related, I wonder whether they do the same or whether there's a difference, and which one is preferred. And "-accel kvm" is even less to type than "-enable-kvm", so there is really no advantage for "-enable-kvm" anymore. I think we should remove "-enable-kvm" and "-enable-hax" from qemu-doc.texi and only list it in the new legacy chapter / document. Thomas
Thomas Huth <thuth@redhat.com> writes: > On 07.06.2018 08:57, Markus Armbruster wrote: >> Thomas Huth <thuth@redhat.com> writes: >> >>> On 05.06.2018 00:40, Eric Blake wrote: >>>> On 06/04/2018 05:34 AM, Thomas Huth wrote: >>>>> On 04.06.2018 09:18, Markus Armbruster wrote: >>>>>> Roman Kagan <rkagan@virtuozzo.com> writes: >>>>>> >>>>>>> Add helper functions to query the block drivers actually supported by >>>>>>> QEMU using "-drive format=?". This allows to skip certain tests that >>>>>>> require drivers not built in or whitelisted in QEMU. >>>>>>> >>>> >>>>>>> + supported_formats=$($QEMU_PROG $QEMU_OPTIONS -drive format=\? >>>>>>> 2>&1 | \ >>>>>> >>>>>> Use of '?' to get help is deprecated. Please use 'format=help', and >>>>>> update your commit message accordingly. >>>>> >>>>> Is it? Where did we document that? >>>> >>>> Hmm, we haven't documented it yet, but it's been that way since commit >>>> c8057f95, in Aug 2012, when we added 'help' as the preferred synonym, >>>> and have tried to avoid mention of '?' in new documentation (as it >>>> requires additional shell quoting). I'm guessing we'll probably see a >>>> patch from you to start an official deprecation window? >>> >>> I'm using '?' regularly on my own, so don't expect a patch from >>> my side here ;-) >>> Anyway, we still use the question mark in our documentation, e.g.: >>> >>> options >>> >>> is a comma separated list of format specific options in a name=value format. >>> Use -o ? for an overview of the options supported by the used format or see >>> the format descriptions below for details. >>> >>> or: >>> >>> -b, --blacklist=list >>> >>> Comma-separated list of RPCs to disable (no spaces, ‘?’ to list available RPCs) >> >> These are bugs, and we need to fix them. >> >>> So calling it deprecated sounds wrong to me. >> >> Our intent to deprecate it was pretty clear in commit c8057f95: >> >> /** >> * is_help_option: >> * @s: string to test >> * >> * Check whether @s is one of the standard strings which indicate >> * that the user is asking for a list of the valid values for a >> * command option like -cpu or -M. The current accepted strings >> --> * are 'help' and '?'. '?' is deprecated (it is a shell wildcard >> * which makes it annoying to use in a reliable way) but provided >> * for backwards compatibility. >> * >> * Returns: true if @s is a request for a list. >> */ >> static inline bool is_help_option(const char *s) >> >> Predates today's more formal deprecation policy. Simpler times. > > Sure, but finding such messages in the sources can be quite cumbersome... > >> There's no real need to kill off '?', unless it gets in the way of >> steering people towards 'help'. We should steer them toward 'help', >> because '?' is a trap for insufficiently sophisticated users of >> shell[*]. > > ... and I agree with your points here. > > => I think we need a second list of deprecated feature (maybe we should > call them "legacy features" instead of "deprecated"?), i.e. a list of > features which we don't recommend for new code / scripts anymore, but > which we do not intend to remove via our official deprecation policy any > time soon. Things like "--enable-kvm" / "-no-kvm" or maybe even "-net" > go into the same category. > > If you agree, I can try to come up with a patch (should the list go into > qemu-doc.texi or a separate document in the the documentation folder?). I'm not sure it's worthwhile. The boundary between "deprecated with intent to remove" and "deprecated without such intent" is going to be a fuzzy one. Could it be a useful one anyway? Formal deprecation is largely for the benefit of people writing software that interfaces with QEMU. They really need to know in advance, and they are well advised to treat either kind of deprecation as "should move to the replacement interface in an orderly manner". If you use QEMU manually, it's easier to just keep using stuff until it's gone, then look for the replacement.
On 07/06/2018 13:27, Thomas Huth wrote: >> As to "-enable-kvm", I don't see anything wrong with users using it, or >> even with occasionally adding more options like it. However, we should >> warn developers that such simple options should be syntactic sugar for a >> structured (i.e. QemuOpts-based) option like "-accel", and that it >> should only be done for similarity with existing options. > Honestly, in this case I think it's just confusing for the normal users, > and not sugar (anymore). If I'm an unexperienced user who wants to > enable KVM, and I see multiple options that seem to be related, I wonder > whether they do the same or whether there's a difference, and which one > is preferred. And "-accel kvm" is even less to type than "-enable-kvm", > so there is really no advantage for "-enable-kvm" anymore. I think we > should remove "-enable-kvm" and "-enable-hax" from qemu-doc.texi and > only list it in the new legacy chapter / document. Well, there's also the issue of distros shipping qemu-kvm binaries. I think those should be provided by upstream. If we do that, then we're perhaps in a better position to place --enable-kvm under the rug. Paolo
On 07.06.2018 13:42, Paolo Bonzini wrote: > On 07/06/2018 13:27, Thomas Huth wrote: >>> As to "-enable-kvm", I don't see anything wrong with users using it, or >>> even with occasionally adding more options like it. However, we should >>> warn developers that such simple options should be syntactic sugar for a >>> structured (i.e. QemuOpts-based) option like "-accel", and that it >>> should only be done for similarity with existing options. >> Honestly, in this case I think it's just confusing for the normal users, >> and not sugar (anymore). If I'm an unexperienced user who wants to >> enable KVM, and I see multiple options that seem to be related, I wonder >> whether they do the same or whether there's a difference, and which one >> is preferred. And "-accel kvm" is even less to type than "-enable-kvm", >> so there is really no advantage for "-enable-kvm" anymore. I think we >> should remove "-enable-kvm" and "-enable-hax" from qemu-doc.texi and >> only list it in the new legacy chapter / document. > > Well, there's also the issue of distros shipping qemu-kvm binaries. I > think those should be provided by upstream. If we do that, then we're > perhaps in a better position to place --enable-kvm under the rug. You don't need -enable-kvm for those qemu-kvm binaries, only -no-kvm. And -no-kvm has never been documented in our qemu-doc user documentation anyway (apart from the fact that it is now mentioned in the deprecation chapter). So if we'd now mentioned -no-kvm in a "legacy feature" chapter instead of the deprecation chapter, that would even improve the situation for downstream qemu-kvms :-) Thomas
On Thu, Jun 07, 2018 at 09:50:41AM +0200, Thomas Huth wrote: > On 07.06.2018 08:57, Markus Armbruster wrote: > > Thomas Huth <thuth@redhat.com> writes: > > > >> On 05.06.2018 00:40, Eric Blake wrote: > >>> On 06/04/2018 05:34 AM, Thomas Huth wrote: > >>>> On 04.06.2018 09:18, Markus Armbruster wrote: > >>>>> Roman Kagan <rkagan@virtuozzo.com> writes: > >>>>> > >>>>>> Add helper functions to query the block drivers actually supported by > >>>>>> QEMU using "-drive format=?". This allows to skip certain tests that > >>>>>> require drivers not built in or whitelisted in QEMU. > >>>>>> > >>> > >>>>>> + supported_formats=$($QEMU_PROG $QEMU_OPTIONS -drive format=\? > >>>>>> 2>&1 | \ > >>>>> > >>>>> Use of '?' to get help is deprecated. Please use 'format=help', and > >>>>> update your commit message accordingly. > >>>> > >>>> Is it? Where did we document that? > >>> > >>> Hmm, we haven't documented it yet, but it's been that way since commit > >>> c8057f95, in Aug 2012, when we added 'help' as the preferred synonym, > >>> and have tried to avoid mention of '?' in new documentation (as it > >>> requires additional shell quoting). I'm guessing we'll probably see a > >>> patch from you to start an official deprecation window? > >> > >> I'm using '?' regularly on my own, so don't expect a patch from > >> my side here ;-) > >> Anyway, we still use the question mark in our documentation, e.g.: > >> > >> options > >> > >> is a comma separated list of format specific options in a name=value format. > >> Use -o ? for an overview of the options supported by the used format or see > >> the format descriptions below for details. > >> > >> or: > >> > >> -b, --blacklist=list > >> > >> Comma-separated list of RPCs to disable (no spaces, ‘?’ to list available RPCs) > > > > These are bugs, and we need to fix them. > > > >> So calling it deprecated sounds wrong to me. > > > > Our intent to deprecate it was pretty clear in commit c8057f95: > > > > /** > > * is_help_option: > > * @s: string to test > > * > > * Check whether @s is one of the standard strings which indicate > > * that the user is asking for a list of the valid values for a > > * command option like -cpu or -M. The current accepted strings > > --> * are 'help' and '?'. '?' is deprecated (it is a shell wildcard > > * which makes it annoying to use in a reliable way) but provided > > * for backwards compatibility. > > * > > * Returns: true if @s is a request for a list. > > */ > > static inline bool is_help_option(const char *s) > > > > Predates today's more formal deprecation policy. Simpler times. > > Sure, but finding such messages in the sources can be quite cumbersome... > > > There's no real need to kill off '?', unless it gets in the way of > > steering people towards 'help'. We should steer them toward 'help', > > because '?' is a trap for insufficiently sophisticated users of > > shell[*]. > > ... and I agree with your points here. > > => I think we need a second list of deprecated feature (maybe we should > call them "legacy features" instead of "deprecated"?), i.e. a list of > features which we don't recommend for new code / scripts anymore, but > which we do not intend to remove via our official deprecation policy any > time soon. Things like "--enable-kvm" / "-no-kvm" or maybe even "-net" > go into the same category. I don't see much point to be honest. If --enable-kvm works for the user and we're not intending to removal it, I don't see why they should care about using something else instead. The other thing might have a more flexible syntax, but if they don't need those extra features it really doesn't matter. IOW, I think it is sufficient to just document all the options that exist, and when documenting them simply make a note inline that a particular option is merely syntax suger for an alternative option. Regards, Daniel
diff --git a/tests/qemu-iotests/common.rc b/tests/qemu-iotests/common.rc index 9a65a11026..fe5a4d1cfd 100644 --- a/tests/qemu-iotests/common.rc +++ b/tests/qemu-iotests/common.rc @@ -493,5 +493,24 @@ _require_command() [ -x "$c" ] || _notrun "$1 utility required, skipped this test" } +# this test requires support for specific formats +# +_require_format() +{ + supported_formats=$($QEMU_PROG $QEMU_OPTIONS -drive format=\? 2>&1 | \ + head -1 | cut -d : -f 2) + for f; do + found=false + for sf in $supported_formats; do + if [ "$f" = "$sf" ]; then + found=true + break + fi + done + + $found || _notrun "$QEMU_PROG doesn't support format $f" + done +} + # make sure this script returns success true diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py index e2abf0cb53..698ef2b2c0 100644 --- a/tests/qemu-iotests/iotests.py +++ b/tests/qemu-iotests/iotests.py @@ -119,6 +119,17 @@ def qemu_io(*args): sys.stderr.write('qemu-io received signal %i: %s\n' % (-subp.returncode, ' '.join(args))) return output +def qemu_pipe(*args): + '''Run qemu with an option to print something and exit (e.g. a help option), + and return its output''' + args = [qemu_prog] + qemu_opts + list(args) + subp = subprocess.Popen(args, stdout=subprocess.PIPE, + stderr=subprocess.STDOUT) + output = subp.communicate()[0] + if subp.returncode < 0: + sys.stderr.write('qemu received signal %i: %s\n' % (-subp.returncode, ' '.join(args))) + return output + class QemuIoInteractive: def __init__(self, *args): @@ -550,13 +561,26 @@ def verify_cache_mode(supported_cache_modes=[]): if supported_cache_modes and (cachemode not in supported_cache_modes): notrun('not suitable for this cache mode: %s' % cachemode) +rw_formats = None + +def supports_format(format_name): + format_message = qemu_pipe('-drive', 'format=?') + global rw_formats + if rw_formats is None: + rw_formats = format_message.splitlines()[0].split(':')[1].split() + return format_name in rw_formats + +def require_formats(*formats): + for fmt in formats: + if not supports_format(fmt): + notrun('%s does not support format %s' % (qemu_prog, fmt)) + def supports_quorum(): - return 'quorum' in qemu_img_pipe('--help') + return supports_format('quorum') def verify_quorum(): '''Skip test suite if quorum support is not available''' - if not supports_quorum(): - notrun('quorum support missing') + require_formats('quorum') def main(supported_fmts=[], supported_oses=['linux'], supported_cache_modes=[], unsupported_fmts=[]):
Add helper functions to query the block drivers actually supported by QEMU using "-drive format=?". This allows to skip certain tests that require drivers not built in or whitelisted in QEMU. Signed-off-by: Roman Kagan <rkagan@virtuozzo.com> --- tests/qemu-iotests/common.rc | 19 +++++++++++++++++++ tests/qemu-iotests/iotests.py | 30 +++++++++++++++++++++++++++--- 2 files changed, 46 insertions(+), 3 deletions(-)