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.