[libvirt] [PATCH V2] virsh: rework command parsing
Eric Blake
eblake at redhat.com
Thu Sep 23 18:14:46 UTC 2010
On 09/16/2010 03:36 AM, Lai Jiangshan wrote:
> The first step is needed when we use virsh as a shell.
>
> And the usage was changed:
> old:
> virsh [options] [commands]
>
> new:
> virsh [options]... [<command_name> args...]
> virsh [options]...<command_string>
Actually, thinking about it more, maybe it looks better with this synopsis:
virsh [options]... [<command_string>]
virsh [options]... <command> [args...]
That is, with zero arguments, we get the interactive shell, with one
argument, we get a <command_string> except in the special case where a
<command> is recognized, and with more than one argument, the first
argument must be a <command>.
>
> 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 changed behavior:
> 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.
All good changes, in my mind.
Here's some additional review, on top of Chris's comments. I'm hoping
to see a v3 soon!
> +++ libvirt-0.8.4/tools/virsh.c 2010-09-16 17:13:55.000000000 +0800
> @@ -10162,90 +10162,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)
Just because there wasn't a comment before should not be an excuse for
you. A brief comment describing this method would help; here's what I
came up with reading the code, although it can probably be improved, and
I didn't check callers might have expected other semantics:
/* Parse the command-string STR for the next argument, respecting
quotes. If a command separator is encountered, set *END to the start of
the next command and return NULL; otherwise return the argument parsed.
Also set *LAST_ARG to true if the argument returned is followed by a
command separator. */
Do we need both *end and *last_arg? And why is last_arg int instead of
bool?
> {
> - int tk = VSH_TK_NONE;
> - int quote = FALSE;
> - int sz = 0;
> + int quote = 0;
s/int/bool/
> 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 */
s/begin/&ning/
> + return NULL;
> }
> +
> + res = argstr = p;
> while (*p) {
> - /* end of token is blank space or ';' */
> - if ((quote == FALSE&& (*p == ' ' || *p == '\t')) || *p == ';')
Yeah, one less use of the pointless FALSE.
> + 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' */
s/braught/implied/
> + } 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? */
Delete the comment, keep the error. Why? Extensibility reasons:
if we introduce a command in the future, and someone writes a
command-string that takes advantage of the new command, then tries to
run that script on an older virsh that doesn't recognize the command,
then blindly skipping the unknown command can have disastrous effects -
subsequent commands may do the wrong thing because the system state is
wrong due to the skipped command. So flat out aborting the
command_string is the best thing to do.
>
> @@ -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"
See my above thoughts.
But the bulk of the patch makes sense to me.
--
Eric Blake eblake at redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
More information about the libvir-list
mailing list