Re: [libvirt] Question, how to use virDomainQemuMonitorCommand()
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
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.
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
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 :|
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;
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@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@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
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@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.
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@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
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 :|
participants (4)
-
Chris Lalancette -
Daniel P. Berrange -
Eric Blake -
Lai Jiangshan