[PATCH] virsh: rework command parsing (was: Re: Question, how to use virDomainQemuMonitorCommand())

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 09/09/2010 09:07 PM, Daniel P. Berrange wrote:
> On Thu, Sep 09, 2010 at 08:52:13AM -0400, Chris Lalancette wrote:
>> On 09/09/10 - 04:52:25PM, Lai Jiangshan wrote:
>>> On 09/07/2010 09:22 PM, Chris Lalancette wrote:
>>>> On 09/07/10 - 04:08:13PM, Lai Jiangshan wrote:
>>>>> Hi, Chris,
>>>>>
>>>>> I saw virDomainQemuMonitorCommand() in libvirt-qemu.c,
>>>>> I think it will help me to send arbitrary qemu-monitor command to
>>>>> qemu via libvirtd.
>>>>>
>>>>> But how can I use virDomainQemuMonitorCommand()?
>>>>> Can I use it by just using current tools(virsh or other) without writing any code?
>>>>
>>>> Unfortunately, no.  There is a bug in the current virsh command that prevents
>>>> it from properly parsing the command-lines necessary to send monitor commands
>>>> to the qemu monitor.  Until we fix that bug, we won't push the support into
>>>> virsh.
>>>>
>>>
>>> Thanks,
>>>
>>> We need this feature, could you tell me the detail of the bug,
>>> we will try to fix it or do assists.
>>
>> I've outlined it before on this list, but the gist of it is that the way that
>> virsh parses command-line arguments loses the formatting.  Thus, if you were
>> enter a command like:
>>
>> # virsh qemu-monitor-command f13guest "info cpus"
>>
>> Then virsh main() will get 4 arguments:
>>
>> argv[0] = "virsh";
>> argv[1] = "qemu-monitor-command";
>> argv[2] = "f13guest";
>> argv[3] = "info cpus";
>>
>> So far, all is good.  However, during the parsing of these command-line
>> arguments, virsh takes all of these arguments and smashes them back together
>> as a single string:
>>
>> command = "virsh qemu-monitor-command f13guest info cpus";
>>
>> And then it reparses the whole thing.  Notice that we've lost the quoting,
>> though, so now it's an invalid command.
>>
>> The problem is further complicated by some of the other features of virsh,
>> including the support for separating multiple commands with semicolons.  For
>> example, the following is a legal command:
>>
>> # virsh 'define D.xml; dumpxml D'
>>
>> In any case, the answer is probably to re-write the command-line parsing of
>> virsh to not lose quoting.  I have not had time to do this, so if you have
>> the time to look at it and make it work, patches are definitely appreciated!
> 
> While re-writing the command line mashing to not loose quotes would be
> nice, the more obvious fix is to not mash all the args back into a
> string at all. The virsh command ultimately wants char **argv, and we 
> already have char **argv. So doing a char ** -> char * -> char **
> conversion is just insanity.
> 
> Daniel

I did it as your suggested! Thanks a lot, see the following patch:

Subject: [PATCH] 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.

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]... [commands string]
virsh [options]... [<command> [command options]...]

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 changes:
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.

Signed-off-by: Lai Jiangshan <laijs@xxxxxxxxxxxxxx>
---
 virsh.c |  351 ++++++++++++++++++++++++++++++----------------------------------
 1 file changed, 170 insertions(+), 181 deletions(-)
--- libvirt-0.8.3.old/tools/virsh.c	2010-08-04 19:35:58.000000000 +0800
+++ libvirt-0.8.3/tools/virsh.c	2010-09-13 14:34:15.000000000 +0800
@@ -10159,90 +10159,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);
@@ -10254,130 +10330,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;
 }
 
@@ -10936,7 +10924,8 @@ static void
 vshUsage(void)
 {
     const vshCmdDef *cmd;
-    fprintf(stdout, _("\n%s [options] [commands]\n\n"
+    fprintf(stdout, _("\n%s [options]... [commands string]"
+                      "\n%s [options]... [<command> [command options]...]\n\n"
                       "  options:\n"
                       "    -c | --connect <uri>    hypervisor connection URI\n"
                       "    -r | --readonly         connect readonly\n"
@@ -10946,7 +10935,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,
@@ -11066,25 +11055,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;

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list


[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]