2:41 a.m.
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(a)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;