[libvirt] [PATCH v2] virsh: fix incorrect argument errors for long options

For long options, print: * the option as specified by the user if it's unknown * the cannoncial long option if its argument is not a number (and should be) And for missing arguments, print both the short and the long option name. (Doing only one of those would require either parsing argv ourselves or let getopt print the errors, since we can't tell long and short options apart by optopt or longindex) https://bugzilla.redhat.com/show_bug.cgi?id=949373 --- v1: https://www.redhat.com/archives/libvir-list/2013-April/msg01653.html tools/virsh.c | 23 +++++++++++++++++------ 1 file changed, 17 insertions(+), 6 deletions(-) diff --git a/tools/virsh.c b/tools/virsh.c index 6ec2f7b..ac86608 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -2978,7 +2978,8 @@ vshAllowedEscapeChar(char c) static bool vshParseArgv(vshControl *ctl, int argc, char **argv) { - int arg, len, debug; + int arg, len, debug, i; + int longindex = -1; struct option opt[] = { {"debug", required_argument, NULL, 'd'}, {"help", no_argument, NULL, 'h'}, @@ -2995,11 +2996,12 @@ vshParseArgv(vshControl *ctl, int argc, char **argv) /* Standard (non-command) options. The leading + ensures that no * argument reordering takes place, so that command options are * not confused with top-level virsh options. */ - while ((arg = getopt_long(argc, argv, "+:d:hqtc:vVrl:e:", opt, NULL)) != -1) { + while ((arg = getopt_long(argc, argv, "+:d:hqtc:vVrl:e:", opt, &longindex)) != -1) { switch (arg) { case 'd': if (virStrToLong_i(optarg, NULL, 10, &debug) < 0) { - vshError(ctl, "%s", _("option -d takes a numeric argument")); + vshError(ctl, _("option %s takes a numeric argument"), + longindex == -1 ? "-d" : "--debug"); exit(EXIT_FAILURE); } if (debug < VSH_ERR_DEBUG || debug > VSH_ERR_ERROR) @@ -3050,15 +3052,24 @@ vshParseArgv(vshControl *ctl, int argc, char **argv) } break; case ':': - vshError(ctl, _("option '-%c' requires an argument"), optopt); - exit(EXIT_FAILURE); + for (i = 0; opt[i].name != NULL; i++) { + if (opt[i].val == optopt) { + vshError(ctl, _("option '-%c'/'--%s' requires an argument"), + optopt, opt[i].name); + exit(EXIT_FAILURE); + } + } case '?': - vshError(ctl, _("unsupported option '-%c'. See --help."), optopt); + if (optopt) + vshError(ctl, _("unsupported option '-%c'. See --help."), optopt); + else + vshError(ctl, _("unsupported option '%s'. See --help."), argv[optind - 1]); exit(EXIT_FAILURE); default: vshError(ctl, _("unknown option")); exit(EXIT_FAILURE); } + longindex = -1; } if (argc > optind) { -- 1.8.1.5

On 04/29/2013 11:27 AM, Ján Tomko wrote:
For long options, print: * the option as specified by the user if it's unknown * the cannoncial long option if its argument is not
s/cannonical/canonical/
a number (and should be)
And for missing arguments, print both the short and the long option name. (Doing only one of those would require either parsing argv ourselves or let getopt print the errors, since we can't tell long and short options apart by optopt or longindex)
Listing some before-and-after in the commit message would be very helpful. Here's what I tried: Unknown option: Before: $ virsh -x error: unsupported option '-x'. See --help. $ virsh --x error: unsupported option '- $ virsh -rx error: unsupported option '-x'. See --help. After: $ tools/virsh -x error: unsupported option '-x'. See --help. $ tools/virsh --x error: unsupported option '--x'. See --help. $ tools/virsh -rx error: unsupported option '-x'. See --help. No worse behavior, and definite improvement on the unknown long option. Incorrect argument: Before: $ virsh --d=a error: option -d takes a numeric argument $ virsh --deb=a error: option -d takes a numeric argument After: $ tools/virsh -da error: option -d takes a numeric argument $ tools/virsh --deb=a error: option --debug takes a numeric argument Much nicer on the long option :) Missing argument: Before: $ virsh -d error: option '-d' requires an argument $ virsh --deb error: option '-d' requires an argument After: $ tools/virsh -d error: option '-d'/'--debug' requires an argument $ tools/virsh --deb error: option '-d'/'--debug' requires an argument A bit more verbose, but now it is obvious when a long option was typed, rather than making you figure out what short option matches your long option. And I agree that improving it would require reimplementing too much of getopt_long, so I can live with it. ACK. Reasonable for 1.0.5 -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

On 04/29/2013 10:48 PM, Eric Blake wrote:
On 04/29/2013 11:27 AM, Ján Tomko wrote:
For long options, print: * the option as specified by the user if it's unknown * the cannoncial long option if its argument is not
s/cannonical/canonical/
...
a number (and should be)
And for missing arguments, print both the short and the long option name. (Doing only one of those would require either parsing argv ourselves or let getopt print the errors, since we can't tell long and short options apart by optopt or longindex)
Listing some before-and-after in the commit message would be very helpful. Here's what I tried:
...
ACK. Reasonable for 1.0.5
Thank you, I've fixed the typo, added examples of the changed errors to the commit message and pushed it. Jan
participants (2)
-
Eric Blake
-
Ján Tomko