[PATCH] guestfish: Enable grouping in string lists
This change adds the ability to group entries in a string list with single quotes. So the string: "'foo bar'" becomes 1 token rather than 2. Consequently single quotes must now be escaped: "\'" resolves to a literal single quote. Incidentally, this change also alters another, probably unintentional behaviour of the previous implementation, in that tokens are separated by any amount of whitespace rather than a single whitespace character. I.e.: "a b" resolves to: 'a' 'b' rather than: 'a' '' 'b' That last syntax can be used if an empty argument is still desired. Whitespace is now also defined to include tabs. parse_string_list can also now fail if it contains an unmatched open quote. --- fish/fish.c | 131 +++++++++++++++++++++++++++++++++++++++++++++-------- guestfish.pod | 6 ++- src/generator.ml | 3 +- 3 files changed, 118 insertions(+), 22 deletions(-) diff --git a/fish/fish.c b/fish/fish.c index a4069d6..86c0dfa 100644 --- a/fish/fish.c +++ b/fish/fish.c @@ -1082,30 +1082,121 @@ is_true (const char *str) strcasecmp (str, "no") != 0; } -/* XXX We could improve list parsing. */ char ** parse_string_list (const char *str) { - char **argv; - const char *p, *pend; - int argc, i; - - argc = 1; - for (i = 0; str[i]; ++i) - if (str[i] == ' ') argc++; - - argv = malloc (sizeof (char *) * (argc+1)); - if (argv == NULL) { perror ("malloc"); exit (1); } - - p = str; - i = 0; - while (*p) { - pend = strchrnul (p, ' '); - argv[i] = strndup (p, pend-p); - i++; - p = *pend == ' ' ? pend+1 : pend; + char **argv = NULL; + size_t argv_len = 0; + + /* Current position pointer */ + const char *p = str; + + /* Token might be simple: + * Token + * or be quoted: + * 'This is a single token' + * or contain embedded single-quoted sections: + * This' is a sing'l'e to'ken + * + * The latter may seem over-complicated, but it's what a normal shell does. + * Not doing it risks surprising somebody. + * + * This outer loop is over complete tokens. + */ + while(*p) { + char *tok = NULL; + size_t tok_len = 0; + + /* Skip leading whitespace */ + p += strspn (p, " \t"); + + char in_quote = 0; + + /* This loop is over token 'fragments'. A token can be in multiple bits if + * it contains single quotes. We also treat both sides of an escaped quote + * as separate fragments because we can't just copy it: we have to remove + * the \. + */ + while (*p && (!isblank (*p) || in_quote)) { + const char *end = p; + + /* Check if the fragment starts with a quote */ + if ('\'' == *p) { + /* Toggle in_quote */ + in_quote = !in_quote; + + /* Skip the quote */ + p++; end++; + } + + /* If we're in a quote, look for an end quote */ + if (in_quote) { + end += strcspn (end, "'"); + } + + /* Otherwise, look for whitespace or a quote */ + else { + end += strcspn (end, " \t'"); + } + + /* Grow the token to accommodate the fragment */ + size_t tok_end = tok_len; + tok_len += end - p; + tok = realloc (tok, tok_len + 1); + if (NULL == tok) { perror ("realloc"); exit (1); } + + /* Check if we stopped on an escaped quote */ + if ('\'' == *end && end != p && *(end-1) == '\\') { + /* Add everything before \' to the token */ + memcpy (&tok[tok_end], p, end - p - 1); + + /* Add the quote */ + tok[tok_len-1] = '\''; + + /* Already processed the quote */ + p = end + 1; + } + + else { + /* Add the whole fragment */ + memcpy (&tok[tok_end], p, end - p); + + p = end; + } + } + + /* We've reached the end of a token. We shouldn't still be in quotes. */ + if (in_quote) { + fprintf(stderr, _("Runaway quote in string \"%s\"\n"), str); + + /* Can't use free_strings here because argv isn't NULL terminated yet */ + for (size_t i = 0; i < argv_len; i++) { + free (argv[i]); + } + free(argv); + + return NULL; + } + + /* Add this token if there is one. There might not be if there was + * whitespace at the end of the input string */ + if(tok) { + /* Add the NULL terminator */ + tok[tok_len] = '\0'; + + /* Add the argument to the argument list */ + argv_len++; + argv = realloc(argv, sizeof(*argv) * argv_len); + if (NULL == argv) { perror ("realloc"); exit (1); } + argv[argv_len-1] = tok; + } } - argv[i] = NULL; + + /* NULL terminate the argument list */ + argv_len++; + argv = realloc(argv, sizeof(*argv) * argv_len); + if (NULL == argv) { perror ("realloc"); exit (1); } + argv[argv_len-1] = NULL; return argv; } diff --git a/guestfish.pod b/guestfish.pod index d24c162..affb83b 100644 --- a/guestfish.pod +++ b/guestfish.pod @@ -250,9 +250,13 @@ quotes. For example: rm '/"' A few commands require a list of strings to be passed. For these, use -a space-separated list, enclosed in quotes. For example: +a whitespace-separated list, enclosed in quotes. Strings containing whitespace +to be passed through must be enclosed in single quotes. A literal single quote +must be escaped with a backslash. vgcreate VG "/dev/sda1 /dev/sdb1" + command "/bin/echo 'foo bar'" + command "/bin/echo \'foo\'" =head1 WILDCARDS AND GLOBBING diff --git a/src/generator.ml b/src/generator.ml index db37228..9b2600f 100755 --- a/src/generator.ml +++ b/src/generator.ml @@ -6361,7 +6361,8 @@ and generate_fish_cmds () = pr " %s = strcmp (argv[%d], \"-\") != 0 ? argv[%d] : \"/dev/stdout\";\n" name i i | StringList name | DeviceList name -> - pr " %s = parse_string_list (argv[%d]);\n" name i + pr " %s = parse_string_list (argv[%d]);\n" name i; + pr " if (%s == NULL) return -1;\n" name; | Bool name -> pr " %s = is_true (argv[%d]) ? 1 : 0;\n" name i | Int name -> -- 1.6.2.5
On Fri, Sep 11, 2009 at 05:54:44PM +0100, Matthew Booth wrote:
+ tok = realloc (tok, tok_len + 1); + if (NULL == tok) { perror ("realloc"); exit (1); }
As before, this leaves tok allocated on the error path. [...] Inclusion of documentation is good, but this patch still needs comprehensive tests. Rich. -- Richard Jones, Emerging Technologies, Red Hat http://et.redhat.com/~rjones New in Fedora 11: Fedora Windows cross-compiler. Compile Windows programs, test, and build Windows installers. Over 70 libraries supprt'd http://fedoraproject.org/wiki/MinGW http://www.annexia.org/fedora_mingw
On 12/09/09 11:06, Richard W.M. Jones wrote:
On Fri, Sep 11, 2009 at 05:54:44PM +0100, Matthew Booth wrote:
+ tok = realloc (tok, tok_len + 1); + if (NULL == tok) { perror ("realloc"); exit (1); }
As before, this leaves tok allocated on the error path.
As discussed on IRC, although this strictly isn't necessary here because of the exit(), I'll add this anyway for robustness in case the exit is changed later.
[...]
Inclusion of documentation is good, but this patch still needs comprehensive tests.
New patch coming up includes comprehensive tests. Matt -- Matthew Booth, RHCA, RHCSS Red Hat Engineering, Virtualisation Team M: +44 (0)7977 267231 GPG ID: D33C3490 GPG FPR: 3733 612D 2D05 5458 8A8A 1600 3441 EA19 D33C 3490
This change adds the ability to group entries in a string list with single quotes. So the string: "'foo bar'" becomes 1 token rather than 2. Consequently single quotes must now be escaped: "\'" resolves to a literal single quote. Incidentally, this change also alters another, probably unintentional behaviour of the previous implementation, in that tokens are separated by any amount of whitespace rather than a single whitespace character. I.e.: "a b" resolves to: 'a' 'b' rather than: 'a' '' 'b' That last syntax can be used if an empty argument is still desired. Whitespace is now also defined to include tabs. parse_string_list can also now fail if it contains an unmatched open quote. --- fish/fish.c | 150 +++++++++++++++++++++++++++++++++++----- guestfish.pod | 6 ++- regressions/Makefile.am | 3 +- regressions/test-stringlist.sh | 60 ++++++++++++++++ src/generator.ml | 3 +- 5 files changed, 202 insertions(+), 20 deletions(-) create mode 100755 regressions/test-stringlist.sh diff --git a/fish/fish.c b/fish/fish.c index a4069d6..9316eda 100644 --- a/fish/fish.c +++ b/fish/fish.c @@ -1082,30 +1082,146 @@ is_true (const char *str) strcasecmp (str, "no") != 0; } -/* XXX We could improve list parsing. */ +/* Free strings from a non-NULL terminated char** */ +static void +free_n_strings (char **str, size_t len) +{ + for (size_t i = 0; i < len; i++) { + free (str[i]); + } + free (str); +} + char ** parse_string_list (const char *str) { - char **argv; - const char *p, *pend; - int argc, i; + char **argv = NULL; + size_t argv_len = 0; + + /* Current position pointer */ + const char *p = str; + + /* Token might be simple: + * Token + * or be quoted: + * 'This is a single token' + * or contain embedded single-quoted sections: + * This' is a sing'l'e to'ken + * + * The latter may seem over-complicated, but it's what a normal shell does. + * Not doing it risks surprising somebody. + * + * This outer loop is over complete tokens. + */ + while (*p) { + char *tok = NULL; + size_t tok_len = 0; - argc = 1; - for (i = 0; str[i]; ++i) - if (str[i] == ' ') argc++; + /* Skip leading whitespace */ + p += strspn (p, " \t"); - argv = malloc (sizeof (char *) * (argc+1)); - if (argv == NULL) { perror ("malloc"); exit (1); } + char in_quote = 0; - p = str; - i = 0; - while (*p) { - pend = strchrnul (p, ' '); - argv[i] = strndup (p, pend-p); - i++; - p = *pend == ' ' ? pend+1 : pend; + /* This loop is over token 'fragments'. A token can be in multiple bits if + * it contains single quotes. We also treat both sides of an escaped quote + * as separate fragments because we can't just copy it: we have to remove + * the \. + */ + while (*p && (!isblank (*p) || in_quote)) { + const char *end = p; + + /* Check if the fragment starts with a quote */ + if ('\'' == *p) { + /* Toggle in_quote */ + in_quote = !in_quote; + + /* Skip the quote */ + p++; end++; + } + + /* If we're in a quote, look for an end quote */ + if (in_quote) { + end += strcspn (end, "'"); + } + + /* Otherwise, look for whitespace or a quote */ + else { + end += strcspn (end, " \t'"); + } + + /* Grow the token to accommodate the fragment */ + size_t tok_end = tok_len; + tok_len += end - p; + char *tok_new = realloc (tok, tok_len + 1); + if (NULL == tok_new) { + perror ("realloc"); + free_n_strings (argv, argv_len); + free (tok); + exit (1); + } + tok = tok_new; + + /* Check if we stopped on an escaped quote */ + if ('\'' == *end && end != p && *(end-1) == '\\') { + /* Add everything before \' to the token */ + memcpy (&tok[tok_end], p, end - p - 1); + + /* Add the quote */ + tok[tok_len-1] = '\''; + + /* Already processed the quote */ + p = end + 1; + } + + else { + /* Add the whole fragment */ + memcpy (&tok[tok_end], p, end - p); + + p = end; + } + } + + /* We've reached the end of a token. We shouldn't still be in quotes. */ + if (in_quote) { + fprintf (stderr, _("Runaway quote in string \"%s\"\n"), str); + + free_n_strings (argv, argv_len); + + return NULL; + } + + /* Add this token if there is one. There might not be if there was + * whitespace at the end of the input string */ + if (tok) { + /* Add the NULL terminator */ + tok[tok_len] = '\0'; + + /* Add the argument to the argument list */ + argv_len++; + char **argv_new = realloc (argv, sizeof (*argv) * argv_len); + if (NULL == argv_new) { + perror ("realloc"); + free_n_strings (argv, argv_len-1); + free (tok); + exit (1); + } + argv = argv_new; + + argv[argv_len-1] = tok; + } } - argv[i] = NULL; + + /* NULL terminate the argument list */ + argv_len++; + char **argv_new = realloc (argv, sizeof (*argv) * argv_len); + if (NULL == argv_new) { + perror ("realloc"); + free_n_strings (argv, argv_len-1); + exit (1); + } + argv = argv_new; + + argv[argv_len-1] = NULL; return argv; } diff --git a/guestfish.pod b/guestfish.pod index d24c162..affb83b 100644 --- a/guestfish.pod +++ b/guestfish.pod @@ -250,9 +250,13 @@ quotes. For example: rm '/"' A few commands require a list of strings to be passed. For these, use -a space-separated list, enclosed in quotes. For example: +a whitespace-separated list, enclosed in quotes. Strings containing whitespace +to be passed through must be enclosed in single quotes. A literal single quote +must be escaped with a backslash. vgcreate VG "/dev/sda1 /dev/sdb1" + command "/bin/echo 'foo bar'" + command "/bin/echo \'foo\'" =head1 WILDCARDS AND GLOBBING diff --git a/regressions/Makefile.am b/regressions/Makefile.am index 3279f95..9112b94 100644 --- a/regressions/Makefile.am +++ b/regressions/Makefile.am @@ -32,7 +32,8 @@ TESTS = \ test-qemudie-synch.sh \ test-read_file.sh \ test-remote.sh \ - test-reopen.sh + test-reopen.sh \ + test-stringlist.sh SKIPPED_TESTS = \ test-bootbootboot.sh diff --git a/regressions/test-stringlist.sh b/regressions/test-stringlist.sh new file mode 100755 index 0000000..0b0c476 --- /dev/null +++ b/regressions/test-stringlist.sh @@ -0,0 +1,60 @@ +#!/bin/sh - +# libguestfs +# Copyright (C) 2009 Red Hat Inc. +# +# This program is free software; you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation; either version 2 of the License, or +# (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program; if not, write to the Free Software +# Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA. + +# Test remote control of guestfish. + +set -e + +rm -f test.img + +eval `../fish/guestfish --listen` + +error=0 + +function check_echo { + test=$1 + expected=$2 + + local echo + + echo=$(../fish/guestfish --remote echo_daemon "$test") + if [ "$echo" != "$expected" ]; then + echo "Expected \"$expected\", got \"$echo\"" + error=1 + fi +} + +../fish/guestfish --remote alloc test.img 10M +../fish/guestfish --remote run + +check_echo "' '" " " +check_echo "\'" "'" +check_echo "'\''" "'" +check_echo "'\' '" "' " +check_echo "'\'foo\''" "'foo'" +check_echo "foo' 'bar" "foo bar" +check_echo "foo' 'bar" "foo bar" +check_echo "'foo' 'bar'" "foo bar" +check_echo "'foo' " "foo" +check_echo " 'foo'" "foo" + +../fish/guestfish --remote exit + +rm -f test.img + +exit $error diff --git a/src/generator.ml b/src/generator.ml index 71aeeed..fa08688 100755 --- a/src/generator.ml +++ b/src/generator.ml @@ -6362,7 +6362,8 @@ and generate_fish_cmds () = pr " %s = strcmp (argv[%d], \"-\") != 0 ? argv[%d] : \"/dev/stdout\";\n" name i i | StringList name | DeviceList name -> - pr " %s = parse_string_list (argv[%d]);\n" name i + pr " %s = parse_string_list (argv[%d]);\n" name i; + pr " if (%s == NULL) return -1;\n" name; | Bool name -> pr " %s = is_true (argv[%d]) ? 1 : 0;\n" name i | Int name -> -- 1.6.2.5
On Mon, Sep 14, 2009 at 11:44:43AM +0100, Matthew Booth wrote:
This change adds the ability to group entries in a string list with single quotes. So the string: "'foo bar'" becomes 1 token rather than 2. Consequently single quotes must now be escaped: "\'" resolves to a literal single quote.
Incidentally, this change also alters another, probably unintentional behaviour of the previous implementation, in that tokens are separated by any amount of whitespace rather than a single whitespace character. I.e.: "a b" resolves to: 'a' 'b' rather than: 'a' '' 'b' That last syntax can be used if an empty argument is still desired. Whitespace is now also defined to include tabs.
parse_string_list can also now fail if it contains an unmatched open quote.
ACK. Rich. -- Richard Jones, Emerging Technologies, Red Hat http://et.redhat.com/~rjones virt-p2v converts physical machines to virtual machines. Boot with a live CD or over the network (PXE) and turn machines into Xen guests. http://et.redhat.com/~rjones/virt-p2v
participants (2)
-
Matthew Booth -
Richard W.M. Jones