Re: [libvirt] [PATCH V2] virsh: rework command parsing

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!