[libvirt] [PATCH V2] virsh: rework command parsing
Old virsh command parsing mashes all the args back into a string and miss the quotes, this patch fixes it. It is also needed for introducing qemu-monitor-command which is very useful. This patch split the command-parsing into 2 phrases: 1) parse command string and make it into <args, argv> style arguments. 2) parse <args, argv> style arguments and make it into vshCmd structure. The first step is needed when we use virsh as a shell. And the usage was changed: old: virsh [options] [commands] new: virsh [options]... [<command_name> args...] virsh [options]... <command_string> So we still support commands like: # virsh "define D.xml; dumpxml D" "define D.xml; dumpxml D" was parsed as a commands-string. and support commands like: # virsh qemu-monitor-command f13guest "info cpus" we will not mash them into a string, we just skip the step 1 parsing and goto the step 2 parsing directly. But we don't support the command like: # virsh "define D.xml; dumpxml" D "define D.xml; dumpxml" was parsed as a command-name, but we have no such command-name. Misc changed behavior: 1) support escape '\' 2) a better quoting support, the following commands are now supported: virsh # dumpxml --"update-cpu" vm1 virsh # dumpxml --update-cpu vm"1" 3) better handling the boolean options, in old code the following commands are equivalent: virsh # dumpxml --update-cpu=vm1 virsh # dumpxml --update-cpu vm1 after this patch applied, the first one will become illegal. The idea of this patch is from Daniel P. Berrange. changed from V1: changed the usage as Eric Blake suggested. Signed-off-by: Lai Jiangshan <laijs@cn.fujitsu.com> --- virsh.c | 351 ++++++++++++++++++++++++++++++---------------------------------- 1 file changed, 170 insertions(+), 181 deletions(-) --- libvirt-0.8.4.old/tools/virsh.c 2010-09-10 20:47:06.000000000 +0800 +++ libvirt-0.8.4/tools/virsh.c 2010-09-16 17:13:55.000000000 +0800 @@ -10162,90 +10162,166 @@ vshCommandRun(vshControl *ctl, const vsh * Command string parsing * --------------- */ -#define VSH_TK_ERROR -1 -#define VSH_TK_NONE 0 -#define VSH_TK_OPTION 1 -#define VSH_TK_DATA 2 -#define VSH_TK_END 3 -static int -vshCommandGetToken(vshControl *ctl, char *str, char **end, char **res) +static char * +vshCmdStrGetArg(vshControl *ctl, char *str, char **end, int *last_arg) { - int tk = VSH_TK_NONE; - int quote = FALSE; - int sz = 0; + int quote = 0; char *p = str; - char *tkstr = NULL; + char *argstr = NULL; + char *res = NULL; *end = NULL; + *last_arg = 0; while (p && *p && (*p == ' ' || *p == '\t')) p++; if (p == NULL || *p == '\0') - return VSH_TK_END; + return NULL; + if (*p == ';') { - *end = ++p; /* = \0 or begin of next command */ - return VSH_TK_END; + *end = p + 1; /* = \0 or begin of next command */ + return NULL; } + + res = argstr = p; while (*p) { - /* end of token is blank space or ';' */ - if ((quote == FALSE && (*p == ' ' || *p == '\t')) || *p == ';') + if (*p == '\\') { /* escape */ + p++; + if (*p == '\0') + break; + } else if (*p == '"') { /* quote */ + quote = !quote; + p++; + continue; + } else if ((!quote && (*p == ' ' || *p == '\t')) || *p == ';') break; - /* end of option name could be '=' */ - if (tk == VSH_TK_OPTION && *p == '=') { - p++; /* skip '=' */ - break; - } + *argstr++ = *p++; + } + + if (*p != '\0') { + if (*p == ';') + *last_arg = 1; + *end = p + 1; /* skip the \0 braught by *argstr = '\0' */ + } else + *end = p; + *argstr = '\0'; + + if (quote) { + vshError(ctl, "%s", _("missing \"")); + return NULL; + } + + return res; +} - if (tk == VSH_TK_NONE) { - if (*p == '-' && *(p + 1) == '-' && *(p + 2) +static vshCmd * +vshCommandParseArgv(vshControl *ctl, int args, char *argv[]) +{ + vshCmdOpt *first = NULL, *last = NULL; + const vshCmdDef *cmd; + vshCmd *c; + int i; + int data_ct = 0; + + if (!(cmd = vshCmddefSearch(argv[0]))) { + vshError(ctl, _("unknown command: '%s'"), argv[0]); + goto syntaxError; /* ... or ignore this command only? */ + } + + for (i = 1; i < args; i++) { + vshCmdOpt *arg; + const vshCmdOptDef *opt; + char *p, *q; + + p = vshStrdup(ctl, argv[i]); + if (*p == '-' && *(p + 1) == '-' && *(p + 2) && c_isalnum(*(p + 2))) { - tk = VSH_TK_OPTION; - p += 2; - } else { - tk = VSH_TK_DATA; - if (*p == '"') { - quote = TRUE; - p++; - } else { - quote = FALSE; + q = strchr(p + 2, '='); + if (q) { + *q = '\0'; + q = vshStrdup(ctl, q + 1); + } + + if (!(opt = vshCmddefGetOption(cmd, p + 2))) { + vshError(ctl, + _("command '%s' doesn't support option --%s"), + cmd->name, p); + VIR_FREE(p); + VIR_FREE(q); + goto syntaxError; + } + VIR_FREE(p); + + if (opt->type == VSH_OT_BOOL) { + if (q) { + vshError(ctl, _("invalid '=' after option --%s"), + opt->name); + VIR_FREE(q); + goto syntaxError; } + } else if (!q) { + i++; + if (i == args) { + vshError(ctl, + _("expected syntax: --%s <%s>"), + opt->name, + opt->type == + VSH_OT_INT ? _("number") : _("string")); + goto syntaxError; + } + q = vshStrdup(ctl, argv[i]); + } + } else { + q = p; + if (!(opt = vshCmddefGetData(cmd, data_ct++))) { + vshError(ctl, _("unexpected data '%s'"), q); + VIR_FREE(q); + goto syntaxError; } - tkstr = p; /* begin of token */ - } else if (quote && *p == '"') { - quote = FALSE; - p++; - break; /* end of "..." token */ } - p++; - sz++; + + arg = vshMalloc(ctl, sizeof(vshCmdOpt)); + arg->def = opt; + arg->data = q; + arg->next = NULL; + + if (!first) + first = arg; + if (last) + last->next = arg; + last = arg; + + vshDebug(ctl, 4, "%s: %s(%s): %s\n", cmd->name, opt->name, + arg->data != p ? _("OPTION") : _("DATA"), arg->data); } - if (quote) { - vshError(ctl, "%s", _("missing \"")); - return VSH_TK_ERROR; + + c = vshMalloc(ctl, sizeof(vshCmd)); + + c->opts = first; + c->def = cmd; + c->next = NULL; + + if (!vshCommandCheckOpts(ctl, c)) { + VIR_FREE(c); + goto syntaxError; } - if (tkstr == NULL || *tkstr == '\0' || p == NULL) - return VSH_TK_END; - if (sz == 0) - return VSH_TK_END; - - *res = vshMalloc(ctl, sz + 1); - memcpy(*res, tkstr, sz); - *(*res + sz) = '\0'; + return c; - *end = p; - return tk; +syntaxError: + if (first) + vshCommandOptFree(first); + return NULL; } + static int vshCommandParse(vshControl *ctl, char *cmdstr) { char *str; - char *tkdata = NULL; vshCmd *clast = NULL; - vshCmdOpt *first = NULL; if (ctl->cmd) { vshCommandFree(ctl->cmd); @@ -10257,130 +10333,42 @@ vshCommandParse(vshControl *ctl, char *c str = cmdstr; while (str && *str) { - vshCmdOpt *last = NULL; - const vshCmdDef *cmd = NULL; - int tk = VSH_TK_NONE; - int data_ct = 0; - - first = NULL; - - while (tk != VSH_TK_END) { - char *end = NULL; - const vshCmdOptDef *opt = NULL; - - tkdata = NULL; - - /* get token */ - tk = vshCommandGetToken(ctl, str, &end, &tkdata); - - str = end; - - if (tk == VSH_TK_END) { - VIR_FREE(tkdata); - break; - } - if (tk == VSH_TK_ERROR) + vshCmd *c; + int args = 0; + char *argv[20]; + char *arg; + int last_arg = 0; + + while ((arg = vshCmdStrGetArg(ctl, str, &str, &last_arg)) != NULL) { + if (args == sizeof(argv) / sizeof(argv[0])) { + vshError(ctl, _("too many args")); goto syntaxError; - - if (cmd == NULL) { - /* first token must be command name */ - if (tk != VSH_TK_DATA) { - vshError(ctl, - _("unexpected token (command name): '%s'"), - tkdata); - goto syntaxError; - } - if (!(cmd = vshCmddefSearch(tkdata))) { - vshError(ctl, _("unknown command: '%s'"), tkdata); - goto syntaxError; /* ... or ignore this command only? */ - } - VIR_FREE(tkdata); - } else if (tk == VSH_TK_OPTION) { - if (!(opt = vshCmddefGetOption(cmd, tkdata))) { - vshError(ctl, - _("command '%s' doesn't support option --%s"), - cmd->name, tkdata); - goto syntaxError; - } - VIR_FREE(tkdata); /* option name */ - - if (opt->type != VSH_OT_BOOL) { - /* option data */ - tk = vshCommandGetToken(ctl, str, &end, &tkdata); - str = end; - if (tk == VSH_TK_ERROR) - goto syntaxError; - if (tk != VSH_TK_DATA) { - vshError(ctl, - _("expected syntax: --%s <%s>"), - opt->name, - opt->type == - VSH_OT_INT ? _("number") : _("string")); - goto syntaxError; - } - } - } else if (tk == VSH_TK_DATA) { - if (!(opt = vshCmddefGetData(cmd, data_ct++))) { - vshError(ctl, _("unexpected data '%s'"), tkdata); - goto syntaxError; - } - } - if (opt) { - /* save option */ - vshCmdOpt *arg = vshMalloc(ctl, sizeof(vshCmdOpt)); - - arg->def = opt; - arg->data = tkdata; - arg->next = NULL; - tkdata = NULL; - - if (!first) - first = arg; - if (last) - last->next = arg; - last = arg; - - vshDebug(ctl, 4, "%s: %s(%s): %s\n", - cmd->name, - opt->name, - tk == VSH_TK_OPTION ? _("OPTION") : _("DATA"), - arg->data); } - if (!str) + argv[args++] = arg; + if (last_arg) break; } + if (args == 0) + continue; - /* command parsed -- allocate new struct for the command */ - if (cmd) { - vshCmd *c = vshMalloc(ctl, sizeof(vshCmd)); - - c->opts = first; - c->def = cmd; - c->next = NULL; - - if (!vshCommandCheckOpts(ctl, c)) { - VIR_FREE(c); - goto syntaxError; - } - - if (!ctl->cmd) - ctl->cmd = c; - if (clast) - clast->next = c; - clast = c; - } + c = vshCommandParseArgv(ctl, args, argv); + if (!c) + goto syntaxError; + + if (!ctl->cmd) + ctl->cmd = c; + if (clast) + clast->next = c; + clast = c; } return TRUE; - syntaxError: +syntaxError: if (ctl->cmd) { vshCommandFree(ctl->cmd); ctl->cmd = NULL; } - if (first) - vshCommandOptFree(first); - VIR_FREE(tkdata); return FALSE; } @@ -10939,7 +10927,8 @@ static void vshUsage(void) { const vshCmdDef *cmd; - fprintf(stdout, _("\n%s [options] [commands]\n\n" + fprintf(stdout, _("\n%s [options]... [<command_name> args...]" + "\n%s [options]... <command_string>\n\n" " options:\n" " -c | --connect <uri> hypervisor connection URI\n" " -r | --readonly connect readonly\n" @@ -10949,7 +10938,7 @@ vshUsage(void) " -t | --timing print timing information\n" " -l | --log <file> output logging to file\n" " -v | --version program version\n\n" - " commands (non interactive mode):\n"), progname); + " commands (non interactive mode):\n"), progname, progname); for (cmd = commands; cmd->name; cmd++) fprintf(stdout, @@ -11069,25 +11058,25 @@ vshParseArgv(vshControl *ctl, int argc, if (argc > end) { /* parse command */ - char *cmdstr; - int sz = 0, ret; + int ret = TRUE; ctl->imode = FALSE; - for (i = end; i < argc; i++) - sz += strlen(argv[i]) + 1; /* +1 is for blank space between items */ - - cmdstr = vshCalloc(ctl, sz + 1, 1); - - for (i = end; i < argc; i++) { - strncat(cmdstr, argv[i], sz); - sz -= strlen(argv[i]); - strncat(cmdstr, " ", sz--); + if (argc - end == 1) { + char *cmdstr = vshStrdup(ctl, argv[end]); + vshDebug(ctl, 2, "commands: \"%s\"\n", cmdstr); + ret = vshCommandParse(ctl, cmdstr); + VIR_FREE(cmdstr); + } else { + if (ctl->cmd) { + vshCommandFree(ctl->cmd); + ctl->cmd = NULL; + } + ctl->cmd = vshCommandParseArgv(ctl, argc - end, argv + end); + if (!ctl->cmd) + ret = FALSE; } - vshDebug(ctl, 2, "command: \"%s\"\n", cmdstr); - ret = vshCommandParse(ctl, cmdstr); - VIR_FREE(cmdstr); return ret; } return TRUE;
On 09/16/10 - 05:36:11PM, Lai Jiangshan wrote:
Old virsh command parsing mashes all the args back into a string and miss the quotes, this patch fixes it. It is also needed for introducing qemu-monitor-command which is very useful.
This patch split the command-parsing into 2 phrases: 1) parse command string and make it into <args, argv> style arguments. 2) parse <args, argv> style arguments and make it into vshCmd structure.
This is some nice work, and indeed does seem to fix the behavior for qemu-monitor-command. I have a few comments inline. <snip>
--- libvirt-0.8.4.old/tools/virsh.c 2010-09-10 20:47:06.000000000 +0800 +++ libvirt-0.8.4/tools/virsh.c 2010-09-16 17:13:55.000000000 +0800 ... @@ -10257,130 +10333,42 @@ vshCommandParse(vshControl *ctl, char *c
str = cmdstr; while (str && *str) { - vshCmdOpt *last = NULL; - const vshCmdDef *cmd = NULL; - int tk = VSH_TK_NONE; - int data_ct = 0; - - first = NULL; - - while (tk != VSH_TK_END) { - char *end = NULL; - const vshCmdOptDef *opt = NULL; - - tkdata = NULL; - - /* get token */ - tk = vshCommandGetToken(ctl, str, &end, &tkdata); - - str = end; - - if (tk == VSH_TK_END) { - VIR_FREE(tkdata); - break; - } - if (tk == VSH_TK_ERROR) + vshCmd *c; + int args = 0; + char *argv[20];
Why argv[20] here? It seems like an arbitrary number, and the check below seems like an arbitrary check. Can we just make this unlimited and allocate memory as needed?
+ char *arg; + int last_arg = 0; + + while ((arg = vshCmdStrGetArg(ctl, str, &str, &last_arg)) != NULL) { + if (args == sizeof(argv) / sizeof(argv[0])) { + vshError(ctl, _("too many args")); goto syntaxError; -
<snip>
@@ -10939,7 +10927,8 @@ static void vshUsage(void) { const vshCmdDef *cmd; - fprintf(stdout, _("\n%s [options] [commands]\n\n" + fprintf(stdout, _("\n%s [options]... [<command_name> args...]" + "\n%s [options]... <command_string>\n\n" " options:\n" " -c | --connect <uri> hypervisor connection URI\n" " -r | --readonly connect readonly\n" @@ -10949,7 +10938,7 @@ vshUsage(void) " -t | --timing print timing information\n" " -l | --log <file> output logging to file\n" " -v | --version program version\n\n" - " commands (non interactive mode):\n"), progname); + " commands (non interactive mode):\n"), progname, progname);
for (cmd = commands; cmd->name; cmd++) fprintf(stdout, @@ -11069,25 +11058,25 @@ vshParseArgv(vshControl *ctl, int argc,
Very minor note, but I think you should be able to remove the forward prototype of vshParseArgv at the top of virsh.c.
if (argc > end) { /* parse command */ - char *cmdstr; - int sz = 0, ret; + int ret = TRUE;
ctl->imode = FALSE;
- for (i = end; i < argc; i++) - sz += strlen(argv[i]) + 1; /* +1 is for blank space between items */ - - cmdstr = vshCalloc(ctl, sz + 1, 1); - - for (i = end; i < argc; i++) { - strncat(cmdstr, argv[i], sz); - sz -= strlen(argv[i]); - strncat(cmdstr, " ", sz--); + if (argc - end == 1) { + char *cmdstr = vshStrdup(ctl, argv[end]); + vshDebug(ctl, 2, "commands: \"%s\"\n", cmdstr); + ret = vshCommandParse(ctl, cmdstr); + VIR_FREE(cmdstr); + } else { + if (ctl->cmd) { + vshCommandFree(ctl->cmd); + ctl->cmd = NULL; + }
I don't think you need to free up ctl->cmd here. From what I can tell vshParseArgv can only be called during virsh startup, so there should never be a previous command. Other than that, it looks pretty good. I did some basic testing of it with my qemu-monitor-command patch, and things seemed to work pretty well with it. The code is still a bit more complicated than I would like, but given what it has to do that is probably unavoidable. Once you've made the changes I suggested above (or tell me why they aren't needed), I would be happy to ACK this. Thanks! -- Chris Lalancette
On 09/20/2010 11:25 PM, Chris Lalancette wrote:
On 09/16/10 - 05:36:11PM, Lai Jiangshan wrote:
Old virsh command parsing mashes all the args back into a string and miss the quotes, this patch fixes it. It is also needed for introducing qemu-monitor-command which is very useful.
This patch split the command-parsing into 2 phrases: 1) parse command string and make it into <args, argv> style arguments. 2) parse <args, argv> style arguments and make it into vshCmd structure.
This is some nice work, and indeed does seem to fix the behavior for qemu-monitor-command. I have a few comments inline.
<snip>
--- libvirt-0.8.4.old/tools/virsh.c 2010-09-10 20:47:06.000000000 +0800 +++ libvirt-0.8.4/tools/virsh.c 2010-09-16 17:13:55.000000000 +0800 ... @@ -10257,130 +10333,42 @@ vshCommandParse(vshControl *ctl, char *c
str = cmdstr; while (str && *str) { - vshCmdOpt *last = NULL; - const vshCmdDef *cmd = NULL; - int tk = VSH_TK_NONE; - int data_ct = 0; - - first = NULL; - - while (tk != VSH_TK_END) { - char *end = NULL; - const vshCmdOptDef *opt = NULL; - - tkdata = NULL; - - /* get token */ - tk = vshCommandGetToken(ctl, str, &end, &tkdata); - - str = end; - - if (tk == VSH_TK_END) { - VIR_FREE(tkdata); - break; - } - if (tk == VSH_TK_ERROR) + vshCmd *c; + int args = 0; + char *argv[20];
Why argv[20] here? It seems like an arbitrary number, and the check below seems like an arbitrary check. Can we just make this unlimited and allocate memory as needed?
+ char *arg; + int last_arg = 0; + + while ((arg = vshCmdStrGetArg(ctl, str, &str, &last_arg)) != NULL) { + if (args == sizeof(argv) / sizeof(argv[0])) { + vshError(ctl, _("too many args")); goto syntaxError; -
<snip>
@@ -10939,7 +10927,8 @@ static void vshUsage(void) { const vshCmdDef *cmd; - fprintf(stdout, _("\n%s [options] [commands]\n\n" + fprintf(stdout, _("\n%s [options]... [<command_name> args...]" + "\n%s [options]... <command_string>\n\n" " options:\n" " -c | --connect <uri> hypervisor connection URI\n" " -r | --readonly connect readonly\n" @@ -10949,7 +10938,7 @@ vshUsage(void) " -t | --timing print timing information\n" " -l | --log <file> output logging to file\n" " -v | --version program version\n\n" - " commands (non interactive mode):\n"), progname); + " commands (non interactive mode):\n"), progname, progname);
for (cmd = commands; cmd->name; cmd++) fprintf(stdout, @@ -11069,25 +11058,25 @@ vshParseArgv(vshControl *ctl, int argc,
Very minor note, but I think you should be able to remove the forward prototype of vshParseArgv at the top of virsh.c.
if (argc > end) { /* parse command */ - char *cmdstr; - int sz = 0, ret; + int ret = TRUE;
ctl->imode = FALSE;
- for (i = end; i < argc; i++) - sz += strlen(argv[i]) + 1; /* +1 is for blank space between items */ - - cmdstr = vshCalloc(ctl, sz + 1, 1); - - for (i = end; i < argc; i++) { - strncat(cmdstr, argv[i], sz); - sz -= strlen(argv[i]); - strncat(cmdstr, " ", sz--); + if (argc - end == 1) { + char *cmdstr = vshStrdup(ctl, argv[end]); + vshDebug(ctl, 2, "commands: \"%s\"\n", cmdstr); + ret = vshCommandParse(ctl, cmdstr); + VIR_FREE(cmdstr); + } else { + if (ctl->cmd) { + vshCommandFree(ctl->cmd); + ctl->cmd = NULL; + }
I don't think you need to free up ctl->cmd here. From what I can tell vshParseArgv can only be called during virsh startup, so there should never be a previous command.
Other than that, it looks pretty good. I did some basic testing of it with my qemu-monitor-command patch, and things seemed to work pretty well with it. The code is still a bit more complicated than I would like, but given what it has to do that is probably unavoidable. Once you've made the changes I suggested above (or tell me why they aren't needed), I would be happy to ACK this.
Thanks!
Hi, Chris The V3 patchset(virsh: rework command parsing) was sent and accepted, I'm sorry that I forgot to CC you. Could you resend your qemu-monitor-command patch? We need it, and I will review and test it. Thank you very much. Lai.
On 11/05/10 - 05:20:02PM, Lai Jiangshan wrote:
Hi, Chris
The V3 patchset(virsh: rework command parsing) was sent and accepted, I'm sorry that I forgot to CC you.
Could you resend your qemu-monitor-command patch? We need it, and I will review and test it.
Thanks for reminding me. I've just sent the updated patch to the list. -- Chris Lalancette
On 09/16/2010 03:36 AM, Lai Jiangshan wrote:
The first step is needed when we use virsh as a shell.
And the usage was changed: old: virsh [options] [commands]
new: virsh [options]... [<command_name> args...] virsh [options]...<command_string>
Actually, thinking about it more, maybe it looks better with this synopsis: virsh [options]... [<command_string>] virsh [options]... <command> [args...] That is, with zero arguments, we get the interactive shell, with one argument, we get a <command_string> except in the special case where a <command> is recognized, and with more than one argument, the first argument must be a <command>.
So we still support commands like: # virsh "define D.xml; dumpxml D" "define D.xml; dumpxml D" was parsed as a commands-string.
and support commands like: # virsh qemu-monitor-command f13guest "info cpus" we will not mash them into a string, we just skip the step 1 parsing and goto the step 2 parsing directly.
But we don't support the command like: # virsh "define D.xml; dumpxml" D "define D.xml; dumpxml" was parsed as a command-name, but we have no such command-name.
Misc changed behavior: 1) support escape '\' 2) a better quoting support, the following commands are now supported: virsh # dumpxml --"update-cpu" vm1 virsh # dumpxml --update-cpu vm"1" 3) better handling the boolean options, in old code the following commands are equivalent: virsh # dumpxml --update-cpu=vm1 virsh # dumpxml --update-cpu vm1 after this patch applied, the first one will become illegal.
All good changes, in my mind. Here's some additional review, on top of Chris's comments. I'm hoping to see a v3 soon!
+++ libvirt-0.8.4/tools/virsh.c 2010-09-16 17:13:55.000000000 +0800 @@ -10162,90 +10162,166 @@ vshCommandRun(vshControl *ctl, const vsh * Command string parsing * --------------- */ -#define VSH_TK_ERROR -1 -#define VSH_TK_NONE 0 -#define VSH_TK_OPTION 1 -#define VSH_TK_DATA 2 -#define VSH_TK_END 3
-static int -vshCommandGetToken(vshControl *ctl, char *str, char **end, char **res) +static char * +vshCmdStrGetArg(vshControl *ctl, char *str, char **end, int *last_arg)
Just because there wasn't a comment before should not be an excuse for you. A brief comment describing this method would help; here's what I came up with reading the code, although it can probably be improved, and I didn't check callers might have expected other semantics: /* Parse the command-string STR for the next argument, respecting quotes. If a command separator is encountered, set *END to the start of the next command and return NULL; otherwise return the argument parsed. Also set *LAST_ARG to true if the argument returned is followed by a command separator. */ Do we need both *end and *last_arg? And why is last_arg int instead of bool?
{ - int tk = VSH_TK_NONE; - int quote = FALSE; - int sz = 0; + int quote = 0;
s/int/bool/
char *p = str; - char *tkstr = NULL; + char *argstr = NULL; + char *res = NULL;
*end = NULL; + *last_arg = 0;
while (p&& *p&& (*p == ' ' || *p == '\t')) p++;
if (p == NULL || *p == '\0') - return VSH_TK_END; + return NULL; + if (*p == ';') { - *end = ++p; /* = \0 or begin of next command */ - return VSH_TK_END; + *end = p + 1; /* = \0 or begin of next command */
s/begin/&ning/
+ return NULL; } + + res = argstr = p; while (*p) { - /* end of token is blank space or ';' */ - if ((quote == FALSE&& (*p == ' ' || *p == '\t')) || *p == ';')
Yeah, one less use of the pointless FALSE.
+ if (*p == '\\') { /* escape */ + p++; + if (*p == '\0') + break; + } else if (*p == '"') { /* quote */ + quote = !quote; + p++; + continue; + } else if ((!quote&& (*p == ' ' || *p == '\t')) || *p == ';') break;
- /* end of option name could be '=' */ - if (tk == VSH_TK_OPTION&& *p == '=') { - p++; /* skip '=' */ - break; - } + *argstr++ = *p++; + } + + if (*p != '\0') { + if (*p == ';') + *last_arg = 1; + *end = p + 1; /* skip the \0 braught by *argstr = '\0' */
s/braught/implied/
+ } else + *end = p; + *argstr = '\0'; + + if (quote) { + vshError(ctl, "%s", _("missing \"")); + return NULL; + } + + return res; +}
- if (tk == VSH_TK_NONE) { - if (*p == '-'&& *(p + 1) == '-'&& *(p + 2) +static vshCmd * +vshCommandParseArgv(vshControl *ctl, int args, char *argv[]) +{ + vshCmdOpt *first = NULL, *last = NULL; + const vshCmdDef *cmd; + vshCmd *c; + int i; + int data_ct = 0; + + if (!(cmd = vshCmddefSearch(argv[0]))) { + vshError(ctl, _("unknown command: '%s'"), argv[0]); + goto syntaxError; /* ... or ignore this command only? */
Delete the comment, keep the error. Why? Extensibility reasons: if we introduce a command in the future, and someone writes a command-string that takes advantage of the new command, then tries to run that script on an older virsh that doesn't recognize the command, then blindly skipping the unknown command can have disastrous effects - subsequent commands may do the wrong thing because the system state is wrong due to the skipped command. So flat out aborting the command_string is the best thing to do.
@@ -10939,7 +10927,8 @@ static void vshUsage(void) { const vshCmdDef *cmd; - fprintf(stdout, _("\n%s [options] [commands]\n\n" + fprintf(stdout, _("\n%s [options]... [<command_name> args...]" + "\n%s [options]...<command_string>\n\n"
See my above thoughts. But the bulk of the patch makes sense to me. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
participants (3)
-
Chris Lalancette -
Eric Blake -
Lai Jiangshan