[libvirt] [PATCH 8/8] virsh: add -- support
"--" means no option at the following arguments. Signed-off-by: Lai Jiangshan <laijs@cn.fujitsu.com> --- diff --git a/tools/virsh.c b/tools/virsh.c index a5b438b..d01091f 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -10305,6 +10305,7 @@ vshCommandParse(vshControl *ctl, vshCommandParser *parser) vshCmdOpt *last = NULL; const vshCmdDef *cmd = NULL; int tk; + bool data_only = false; int data_ct = 0; first = NULL; @@ -10327,6 +10328,8 @@ vshCommandParse(vshControl *ctl, vshCommandParser *parser) goto syntaxError; /* ... or ignore this command only? */ } VIR_FREE(tkdata); + } else if (data_only) { + goto get_data; } else if (*tkdata == '-' && *(tkdata + 1) == '-' && *(tkdata + 2) && c_isalnum(*(tkdata + 2))) { char *optstr = strchr(tkdata + 2, '='); @@ -10368,7 +10371,12 @@ vshCommandParse(vshControl *ctl, vshCommandParser *parser) goto syntaxError; } } + } else if (*tkdata == '-' && *(tkdata + 1) == '-' + && *(tkdata + 2) == '\0') { + data_only = true; + continue; } else { +get_data: if (!(opt = vshCmddefGetData(cmd, data_ct++))) { vshError(ctl, _("unexpected data '%s'"), tkdata); goto syntaxError;
On 10/12/2010 01:14 AM, Lai Jiangshan wrote:
"--" means no option at the following arguments.
Signed-off-by: Lai Jiangshan<laijs@cn.fujitsu.com> --- diff --git a/tools/virsh.c b/tools/virsh.c index a5b438b..d01091f 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -10305,6 +10305,7 @@ vshCommandParse(vshControl *ctl, vshCommandParser *parser) vshCmdOpt *last = NULL; const vshCmdDef *cmd = NULL; int tk; + bool data_only = false; int data_ct = 0;
first = NULL; @@ -10327,6 +10328,8 @@ vshCommandParse(vshControl *ctl, vshCommandParser *parser) goto syntaxError; /* ... or ignore this command only? */ } VIR_FREE(tkdata); + } else if (data_only) { + goto get_data; } else if (*tkdata == '-'&& *(tkdata + 1) == '-'&& *(tkdata + 2) && c_isalnum(*(tkdata + 2))) { char *optstr = strchr(tkdata + 2, '='); @@ -10368,7 +10371,12 @@ vshCommandParse(vshControl *ctl, vshCommandParser *parser) goto syntaxError; } } + } else if (*tkdata == '-'&& *(tkdata + 1) == '-' +&& *(tkdata + 2) == '\0') {
Another case of line break convention. Also, I prefer tkdata[1] over *(tkdata + 1). Hmm, that means there's now two levels of -- parsing - one to mark the end of top-level virsh commands, and one for each command. That is: virsh -- help -- help should be the same as virsh help help Which also means that virsh should probably be avoiding argv rearrangement in getopt_long(). That is, given my theoretical echo command, virsh echo --help should echo "--help", rather than running 'virsh --help'. Or, more to the point, virsh dumpxml --update-cpu vm correctly avoids promoting --update-cpu to a top-level argument of virsh. Oh my - I just looked in the code, and virsh is re-doing option parsing by itself, instead of just telling getopt_long() to stop on the first non-option; but getting it wrong by not checking for abbreviations. Another patch or two coming up... ACK with the nit fixed. Here's what I'm squashing. diff --git i/tools/virsh.c w/tools/virsh.c index 79d7756..8c4a7bc 100644 --- i/tools/virsh.c +++ w/tools/virsh.c @@ -10347,8 +10347,8 @@ vshCommandParse(vshControl *ctl, vshCommandParser *parser) VIR_FREE(tkdata); } else if (data_only) { goto get_data; - } else if (*tkdata == '-' && *(tkdata + 1) == '-' && *(tkdata + 2) - && c_isalnum(*(tkdata + 2))) { + } else if (tkdata[0] == '-' && tkdata[1] == '-' && + c_isalnum(tkdata[2])) { char *optstr = strchr(tkdata + 2, '='); if (optstr) { *optstr = '\0'; /* convert the '=' to '\0' */ @@ -10388,8 +10388,8 @@ vshCommandParse(vshControl *ctl, vshCommandParser *parser) goto syntaxError; } } - } else if (*tkdata == '-' && *(tkdata + 1) == '-' - && *(tkdata + 2) == '\0') { + } else if (tkdata[0] == '-' && tkdata[1] == '-' && + tkdata[2] == '\0') { data_only = true; continue; } else { -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
This makes 'virsh --conn test:///default help help' work right; previously, the abbreviation confused our hand-rolled option parsing. * tools/virsh.c (vshParseArgv): Use getopt_long feature, rather than (incorrectly) reparsing options ourselves. ---
Oh my - I just looked in the code, and virsh is re-doing option parsing by itself, instead of just telling getopt_long() to stop on the first non-option; but getting it wrong by not checking for abbreviations. Another patch or two coming up...
I love patches that nuke more code than they add, all while fixing bugs at the same time! tools/virsh.c | 68 +++++++++++++------------------------------------------- 1 files changed, 16 insertions(+), 52 deletions(-) diff --git a/tools/virsh.c b/tools/virsh.c index 8c4a7bc..19a6087 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -10991,9 +10991,8 @@ vshUsage(void) static int vshParseArgv(vshControl *ctl, int argc, char **argv) { - char *last = NULL; - int i, end = 0, help = 0; - int arg, idx = 0; + bool help = false; + int arg; struct option opt[] = { {"debug", 1, 0, 'd'}, {"help", 0, 0, 'h'}, @@ -11006,46 +11005,10 @@ vshParseArgv(vshControl *ctl, int argc, char **argv) {0, 0, 0, 0} }; - - if (argc < 2) - return TRUE; - - /* look for begin of the command, for example: - * ./virsh --debug 5 -q command --cmdoption - * <--- ^ ---> - * getopt() stuff | command suff - */ - for (i = 1; i < argc; i++) { - if (*argv[i] != '-') { - int valid = FALSE; - - /* non "--option" argv, is it command? */ - if (last) { - struct option *o; - int sz = strlen(last); - - for (o = opt; o->name; o++) { - if (o->has_arg == 1){ - if (sz == 2 && *(last + 1) == o->val) - /* valid virsh short option */ - valid = TRUE; - else if (sz > 2 && STREQ(o->name, last + 2)) - /* valid virsh long option */ - valid = TRUE; - } - } - } - if (!valid) { - end = i; - break; - } - } - last = argv[i]; - } - end = end ? end : argc; - - /* standard (non-command) options */ - while ((arg = getopt_long(end, argv, "d:hqtc:vrl:", opt, &idx)) != -1) { + /* 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:vrl:", opt, NULL)) != -1) { switch (arg) { case 'd': if (virStrToLong_i(optarg, NULL, 10, &ctl->debug) < 0) { @@ -11054,7 +11017,7 @@ vshParseArgv(vshControl *ctl, int argc, char **argv) } break; case 'h': - help = 1; + help = true; break; case 'q': ctl->quiet = TRUE; @@ -11066,7 +11029,8 @@ vshParseArgv(vshControl *ctl, int argc, char **argv) ctl->name = vshStrdup(ctl, optarg); break; case 'v': - fprintf(stdout, "%s\n", VERSION); + /* FIXME - list a copyright blurb, as in GNU programs? */ + puts(VERSION); exit(EXIT_SUCCESS); case 'r': ctl->readonly = TRUE; @@ -11081,8 +11045,8 @@ vshParseArgv(vshControl *ctl, int argc, char **argv) } if (help) { - if (end < argc) { - vshError(ctl, _("extra argument '%s'. See --help."), argv[end]); + if (optind < argc) { + vshError(ctl, _("extra argument '%s'. See --help."), argv[optind]); exit(EXIT_FAILURE); } @@ -11091,14 +11055,14 @@ vshParseArgv(vshControl *ctl, int argc, char **argv) exit(EXIT_SUCCESS); } - if (argc > end) { + if (argc > optind) { /* parse command */ ctl->imode = FALSE; - if (argc - end == 1) { - vshDebug(ctl, 2, "commands: \"%s\"\n", argv[end]); - return vshCommandStringParse(ctl, argv[end]); + if (argc - optind == 1) { + vshDebug(ctl, 2, "commands: \"%s\"\n", argv[optind]); + return vshCommandStringParse(ctl, argv[optind]); } else { - return vshCommandArgvParse(ctl, argc - end, argv + end); + return vshCommandArgvParse(ctl, argc - optind, argv + optind); } } return TRUE; -- 1.7.2.3
On Tue, Oct 12, 2010 at 03:51:48PM -0600, Eric Blake wrote:
This makes 'virsh --conn test:///default help help' work right; previously, the abbreviation confused our hand-rolled option parsing.
* tools/virsh.c (vshParseArgv): Use getopt_long feature, rather than (incorrectly) reparsing options ourselves. ---
Oh my - I just looked in the code, and virsh is re-doing option parsing by itself, instead of just telling getopt_long() to stop on the first non-option; but getting it wrong by not checking for abbreviations. Another patch or two coming up...
I love patches that nuke more code than they add, all while fixing bugs at the same time!
tools/virsh.c | 68 +++++++++++++------------------------------------------- 1 files changed, 16 insertions(+), 52 deletions(-)
ACK, way cleaner ! Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
* tools/virsh.c: Update comments to match patch series. --- Noticed this one in reviewing the file once again; it's doc only, so I'll apply it without an ACK once the rest of the series is in place. tools/virsh.c | 10 +++++----- 1 files changed, 5 insertions(+), 5 deletions(-) diff --git a/tools/virsh.c b/tools/virsh.c index 4929f71..89c2e1e 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -100,18 +100,18 @@ typedef enum { * * command_line = <command>\n | <command>; <command>; ... * - * command = <keyword> <option> <data> + * command = <keyword> <option> [--] <data> * * option = <bool_option> | <int_option> | <string_option> * data = <string> * * bool_option = --optionname - * int_option = --optionname <number> - * string_option = --optionname <string> + * int_option = --optionname <number> | --optionname=<number> + * string_option = --optionname <string> | --optionname=<string> * - * keyword = [a-zA-Z] + * keyword = [a-zA-Z][a-zA-Z-]* * number = [0-9]+ - * string = [^[:blank:]] | "[[:alnum:]]"$ + * string = ('[^']*'|"([^\\"]|\\.)*"|([^ \t\n\\'"]|\\.))+ * */ -- 1.7.2.3
On Tue, Oct 12, 2010 at 04:26:01PM -0600, Eric Blake wrote:
* tools/virsh.c: Update comments to match patch series. ---
Noticed this one in reviewing the file once again; it's doc only, so I'll apply it without an ACK once the rest of the series is in place.
tools/virsh.c | 10 +++++----- 1 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/tools/virsh.c b/tools/virsh.c index 4929f71..89c2e1e 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -100,18 +100,18 @@ typedef enum { * * command_line = <command>\n | <command>; <command>; ... * - * command = <keyword> <option> <data> + * command = <keyword> <option> [--] <data> * * option = <bool_option> | <int_option> | <string_option> * data = <string> * * bool_option = --optionname - * int_option = --optionname <number> - * string_option = --optionname <string> + * int_option = --optionname <number> | --optionname=<number> + * string_option = --optionname <string> | --optionname=<string> * - * keyword = [a-zA-Z] + * keyword = [a-zA-Z][a-zA-Z-]* * number = [0-9]+ - * string = [^[:blank:]] | "[[:alnum:]]"$ + * string = ('[^']*'|"([^\\"]|\\.)*"|([^ \t\n\\'"]|\\.))+ * */
The last regexps makes the head spin a bit but if used to it that's logical :-) ACK Also ACK to the initial 8 patch series, so basically the whole set including your extra patches are fine for me, please commit, it will be easier for you :-) thanks to Lai and you ! Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
On 10/13/2010 06:21 AM, Daniel Veillard wrote:
+ * string = ('[^']*'|"([^\\"]|\\.)*"|([^ \t\n\\'"]|\\.))+ * */
The last regexps makes the head spin a bit but if used to it that's logical :-)
In lay terms, a concatenation of one or more: single-quoted strings (ends at first ') double-quoted strings (ends at first " but skipping \ escapes) regular characters (excluding whitespace separators or quotes) backslash escape sequences But yeah, ugly to read.
ACK
Also ACK to the initial 8 patch series, so basically the whole set including your extra patches are fine for me, please commit, it will be easier for you :-)
All pushed now. And I can get back to my vcpus series. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
participants (3)
-
Daniel Veillard -
Eric Blake -
Lai Jiangshan