9:22 p.m.
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.
For that reason you will need to write a custom program to call
virDomainQemuMonitorCommand as appropriate. The absolute easiest program you
can write looks something like (untested):
#include <stdio.h>
#include <stdlib.h>
#include <libvirt/libvirt.h>
int main()
{
virConnectPtr conn;
virDomainPtr dom;
char *reply;
conn = virConnectOpen(NULL);
dom = virDomainLookupByName(conn, "mydomain");
virDomainQemuMonitorCommand(dom, "info cpus", &reply, 0);
fprintf(stderr, "Reply: %s\n", reply);
free(reply);
virConnectClose(conn);
return 0;
}
--
Chris Lalancette
4:52 p.m.
New subject: [libvirt] Question, how to use virDomainQemuMonitorCommand()
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.
Thanks a lot.
Lai.
8:52 p.m.
New subject: [libvirt] Question, how to use virDomainQemuMonitorCommand()
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!
--
Chris Lalancette
9:07 p.m.
New subject: [libvirt] Question, how to use virDomainQemuMonitorCommand()
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
--
|: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
3:41 p.m.
New subject: [libvirt] [PATCH] virsh: rework command parsing (was: Re: Question, how to use virDomainQemuMonitorCommand())
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;
11:03 p.m.
New subject: [libvirt] [PATCH] virsh: rework command parsing
On 09/13/2010 01:41 AM, Lai Jiangshan wrote:
And the usage was changed:
old:
virsh [options] [commands]
new:
virsh [options]... [commands string]
virsh [options]... [<command> [command options]...]
This needs a tweak, otherwise, executing:
virsh
ambiguously matches both forms. In general, you should list the
shortest possible form first. Also, it seems like passing multiple
command strings would be easy to do with this format, such that these
two are equivalent:
virsh "define D.xml" "dumpxml D"
virsh "define D.xml; dumpxml D"
So, I think it should be:
virsh [options]... [<command> [command options]...]
virsh [options]... <commands_string>...
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.
Should any of these be split into separate patches? It's easier to
review a series of small patches that each do one thing than it is to
review 350 lines of multiple changes smashed together.
Signed-off-by: Lai Jiangshan<laijs(a)cn.fujitsu.com>
---
virsh.c | 351 ++++++++++++++++++++++++++++++----------------------------------
Therefore, I haven't closely reviewed this patch yet, but just from the
commit message, I like the direction it is headed in.
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
10:04 a.m.
New subject: [libvirt] [PATCH] virsh: rework command parsing
On 09/13/2010 11:03 PM, Eric Blake wrote:
On 09/13/2010 01:41 AM, Lai Jiangshan wrote:
> And the usage was changed:
> old:
> virsh [options] [commands]
>
> new:
> virsh [options]... [commands string]
> virsh [options]... [<command> [command options]...]
This needs a tweak, otherwise, executing:
virsh
ambiguously matches both forms. In general, you should list the
shortest possible form first. Also, it seems like passing multiple
command strings would be easy to do with this format, such that these
two are equivalent:
virsh "define D.xml" "dumpxml D"
virsh "define D.xml; dumpxml D"
virsh "define D.xml" "dumpxml D"
is not supported in old parser nor new parser,
in new parser, "define D.xml" is command-name
"dumpxml D" is its parameter, we have no such command-name.
So, I think it should be:
virsh [options]... [<command> [command options]...]
virsh [options]... <commands_string>...
It looks good.
> 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.
Should any of these be split into separate patches? It's easier to
review a series of small patches that each do one thing than it is to
review 350 lines of multiple changes smashed together.
It is hard to split them into separate patches. because "Misc change 1)"
and "Misc change 2)" is done by new parser's step 1.
In old parser, '=' is missed after we have got VSH_TK_OPTION token,
if we fix it for "Misc change 3)", we need a lot code, and this code
will be removed after this new parser applied.
>
> Signed-off-by: Lai Jiangshan<laijs(a)cn.fujitsu.com>
> ---
> virsh.c | 351
> ++++++++++++++++++++++++++++++----------------------------------
Therefore, I haven't closely reviewed this patch yet, but just from the
commit message, I like the direction it is headed in.
Thanks, Lai.
10:52 a.m.
New subject: [libvirt] [PATCH] virsh: rework command parsing
On 09/13/2010 11:03 PM, Eric Blake wrote:
On 09/13/2010 01:41 AM, Lai Jiangshan wrote:
> And the usage was changed:
> old:
> virsh [options] [commands]
>
> new:
> virsh [options]... [commands string]
> virsh [options]... [<command> [command options]...]
This needs a tweak, otherwise, executing:
virsh
ambiguously matches both forms. In general, you should list the
shortest possible form first. Also, it seems like passing multiple
command strings would be easy to do with this format, such that these
two are equivalent:
virsh "define D.xml" "dumpxml D"
virsh "define D.xml; dumpxml D"
So, I think it should be:
virsh [options]... [<command> [command options]...]
virsh [options]... <commands_string>...
> 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.
Should any of these be split into separate patches? It's easier to
review a series of small patches that each do one thing than it is to
review 350 lines of multiple changes smashed together.
>
> Signed-off-by: Lai Jiangshan<laijs(a)cn.fujitsu.com>
> ---
> virsh.c | 351
> ++++++++++++++++++++++++++++++----------------------------------
Therefore, I haven't closely reviewed this patch yet, but just from the
commit message, I like the direction it is headed in.
I am still waiting for review comments, I will collect comments
and fix the patch for next version.
Thanks, Lai
11:18 p.m.
New subject: [libvirt] [PATCH] virsh: rework command parsing (was: Re: Question, how to use virDomainQemuMonitorCommand())
On Mon, Sep 13, 2010 at 03:41:51PM +0800, Lai Jiangshan wrote:
On 09/09/2010 09:07 PM, Daniel P. Berrange wrote:
>
> 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.
This sounds good, though I can barely understand the patch diff with
the hairy hand-crafted parser we have there :-)
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.
Daniel
--
|: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
5159
days inactive
5166
days old
8 comments
4 participants
participants (4)
-
Chris Lalancette -
Daniel P. Berrange -
Eric Blake -
Lai Jiangshan