[libvirt] [PATCH 1/8] virsh: better support double quote

[Eric Blake]

On 10/12/2010 01:13 AM, Lai Jiangshan wrote:
>
> In origin code, double quote is only allowed at the begin or end
> "complicated argument"
> --some_opt="complicated string"  (we split this argument into 2 parts,
> option and data, the data is "complicated string").
>
> This patch makes it allow double quote at any position of
> an argument:
> complicated" argument"
> complicated" "argument
> --"some opt=complicated string"
>
> This patch is also needed for the following patches,
> the following patches will not split option argument into 2 parts,
> so we have to allow double quote at any position of an argument.
>
> Signed-off-by: Lai Jiangshan<laijs at cn.fujitsu.com>

I had to add your name to AUTHORS to make syntax-check happy.

> ---
> diff --git a/tools/virsh.c b/tools/virsh.c
> index 57ea618..7b6f2b6 100644
> --- a/tools/virsh.c
> +++ b/tools/virsh.c
> @@ -10172,9 +10172,9 @@ static int
>   vshCommandGetToken(vshControl *ctl, char *str, char **end, char **res)
>   {
>       int tk = VSH_TK_NONE;
> -    int quote = FALSE;
> +    bool double_quote = false;
>       int sz = 0;
> -    char *p = str;
> +    char *p = str, *q;

Maybe it's just me, but I tend to declare multiple pointers on separate 
lines.  But no big deal.

> -        } else if (quote&&  *p == '"') {
> -            quote = FALSE;
> +        }
> +
> +	if (*p == '"') {

'make syntax-check' would have caught this TAB.

> +            double_quote = !double_quote;
>               p++;
> -            break;              /* end of "..." token */
> +            continue;
>           }
> +
> +        if (*res)
> +            (*res)[sz] = *p;

That's a lot of indirection, for every byte of the loop.  Wouldn't it be 
better to have a local temporary, and only assign to *res at the end?

>           p++;
>           sz++;
>       }
> -    if (quote) {
> +    if (double_quote) {
>           vshError(ctl, "%s", _("missing \""));
>           return VSH_TK_ERROR;
>       }
> @@ -10231,8 +10234,12 @@ vshCommandGetToken(vshControl *ctl, char *str, char **end, char **res)
>       if (sz == 0)
>           return VSH_TK_END;
>
> -    *res = vshMalloc(ctl, sz + 1);
> -    memcpy(*res, tkstr, sz);
> +    if (!*res) {
> +        *res = vshMalloc(ctl, sz + 1);
> +        sz = 0;
> +        p = q;
> +        goto copy;
> +    }

Hmm, a backwards jump, which means we parse every string twice - once to 
figure out how long it is, and again to strip quotes.  Yes, that avoids 
over-allocating, but are we really that tight on memory?  I find it 
quicker to just strdup() up front, then edit in-place on a single pass.

Hmm, one thing you _don't_ recognize is:

-"-"option

as an option.  In the shell, quotes are stripped before arguments are 
recognized as a particular token.  I think that's pretty easy to support 
- delay VSH_TK_OPTION checking until after we've stripped quotes, but 
I'll show it as a separate patch.

Hmm, I also noticed that we're inconsistent on strdup/vshStrdup in this 
file; separate cleanup patch for that coming up soon.

But, with those nits fixed, ACK.  Here's what I'm squashing in to my 
local tree; I'll push it once I complete my review of your other 7 
patches, as well as getting approval to my promised followup patches.

diff --git i/AUTHORS w/AUTHORS
index 09169f2..a8f96df 100644
--- i/AUTHORS
+++ w/AUTHORS
@@ -129,6 +129,7 @@ Patches have also been contributed by:
diff --git i/tools/virsh.c w/tools/virsh.c
index 0a28c99..4f70724 100644
--- i/tools/virsh.c
+++ w/tools/virsh.c
@@ -10124,16 +10124,18 @@ vshCommandRun(vshControl *ctl, const vshCmd *cmd)
  #define VSH_TK_DATA    2
  #define VSH_TK_END    3

-static int
+static int ATTRIBUTE_NONNULL(3) ATTRIBUTE_NONNULL(4)
  vshCommandGetToken(vshControl *ctl, char *str, char **end, char **res)
  {
      int tk = VSH_TK_NONE;
      bool double_quote = false;
      int sz = 0;
-    char *p = str, *q;
+    char *p = str;
+    char *q = vshStrdup(ctl, str);
      char *tkstr = NULL;

      *end = NULL;
+    *res = q;

      while (p && *p && (*p == ' ' || *p == '\t'))
          p++;
@@ -10145,9 +10147,6 @@ vshCommandGetToken(vshControl *ctl, char *str, 
char **end, char **res)
          return VSH_TK_END;
      }

-    q = p;
-    *res = NULL;
-copy:
      while (*p) {
          /* end of token is blank space or ';' */
          if (!double_quote && (*p == ' ' || *p == '\t' || *p == ';'))
@@ -10170,34 +10169,23 @@ copy:
              tkstr = p;          /* begin of token */
          }

-	if (*p == '"') {
+        if (*p == '"') {
              double_quote = !double_quote;
              p++;
              continue;
          }

-        if (*res)
-            (*res)[sz] = *p;
-        p++;
+        *q++ = *p++;
          sz++;
      }
      if (double_quote) {
          vshError(ctl, "%s", _("missing \""));
          return VSH_TK_ERROR;
      }
-    if (tkstr == NULL || *tkstr == '\0' || p == NULL)
-        return VSH_TK_END;
-    if (sz == 0)
+    if (tkstr == NULL || *tkstr == '\0' || sz == 0)
          return VSH_TK_END;

-    if (!*res) {
-        *res = vshMalloc(ctl, sz + 1);
-        sz = 0;
-        p = q;
-        goto copy;
-    }
-    *(*res + sz) = '\0';
-
+    *q = '\0';
      *end = p;
      return tk;
  }


-- 
Eric Blake   eblake at redhat.com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org