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@cn.fujitsu.com> --- 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;