11:25 p.m.
On 09/16/10 - 05:36:11PM, Lai Jiangshan wrote:
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 which is very useful.
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 is some nice work, and indeed does seem to fix the behavior for
qemu-monitor-command. I have a few comments inline.
<snip>
--- libvirt-0.8.4.old/tools/virsh.c 2010-09-10 20:47:06.000000000
+0800
+++ libvirt-0.8.4/tools/virsh.c 2010-09-16 17:13:55.000000000 +0800
...
@@ -10257,130 +10333,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];
Why argv[20] here? It seems like an arbitrary number, and the check below
seems like an arbitrary check. Can we just make this unlimited and allocate
memory as needed?
+ 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;
-
<snip>
@@ -10939,7 +10927,8 @@ static void
vshUsage(void)
{
const vshCmdDef *cmd;
- fprintf(stdout, _("\n%s [options] [commands]\n\n"
+ fprintf(stdout, _("\n%s [options]... [<command_name> args...]"
+ "\n%s [options]... <command_string>\n\n"
" options:\n"
" -c | --connect <uri> hypervisor connection
URI\n"
" -r | --readonly connect readonly\n"
@@ -10949,7 +10938,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,
@@ -11069,25 +11058,25 @@ vshParseArgv(vshControl *ctl, int argc,
Very minor note, but I think you should be able to remove the forward prototype
of vshParseArgv at the top of virsh.c.
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;
+ }
I don't think you need to free up ctl->cmd here. From what I can tell
vshParseArgv can only be called during virsh startup, so there should never
be a previous command.
Other than that, it looks pretty good. I did some basic testing of it with
my qemu-monitor-command patch, and things seemed to work pretty well with it.
The code is still a bit more complicated than I would like, but given what it
has to do that is probably unavoidable. Once you've made the changes I
suggested above (or tell me why they aren't needed), I would be happy to ACK
this.
Thanks!
--
Chris Lalancette