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

[Lai Jiangshan]

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.