Re: [patch] Parse quoted string for restore/dump commands.

Sergio Durigan Junior
2013-06-15T18:37:33Z

On Friday, June 07 2013, Wei-cheng Wang wrote:

> This is a patch for parsing quoted string for restore/dump commands.
> Test cases are also added.

Thanks for the patch, Wei-cheng. I don't see your name on
gdb/MAINTAINERS, do you have copyright assignment on file? You will
need one in order to commit the changes. If you don't have it, please
contact me off-list and I can send you the forms.

> diff --git a/gdb/cli/cli-dump.c b/gdb/cli/cli-dump.c
> index 208916c..1fc9f3f 100644
> --- a/gdb/cli/cli-dump.c
> +++ b/gdb/cli/cli-dump.c
> @@ -80,14 +80,54 @@ scan_filename_with_cleanup (char **cmd, const char *defname)
> }
> else
> {
> - /* FIXME: should parse a possibly quoted string. */
> - char *end;
> + char *inp, *outp;
> + char quote = '\0';
> + int escape = 0;
> + int capacity;

size_t capacity;

> +
> + inp = (*cmd) = skip_spaces (*cmd);

I don't really like such multi-variables attributions, I think the code
reads better if you write:

(*cmd) = skip_spaces (*cmd);
inp = (*cmd);

I also understand that you used the same style as the old code,
however.

> + /* Allocate initial buffer for filename.
> + If there is no space in quoted string,
> + this should be the exactly size what we need. */

You could write this comment using larger lines. The soft limit is 72
chars, and the hard limit is 80 chars, so you could write:

/* Allocate initial buffer for filename. If there is no space
in quoted string, this should be the exactly size what we
need. */

> + capacity = strcspn (*cmd, " \t") + 1;
> + outp = filename = (char *) xmalloc (capacity);

Same comment about the attribution as above. Also, you don't need to
cast xmalloc's return.

> +
> + /* Quoting character. */
> + if (*inp == '\'' || *inp == '"')
> + quote = *inp++;
> +
> + for (; *inp != '\0'; inp++)
> + {
> + /* Stop by whitespace, if not quoted or escaped. */
> + if (whitespace (*inp) && !escape && !quote)

Hm, "whitespace" is readline-specific. Please use "isspace" instead.
Also, since you are explicitly checking every char variable, I'd also do
it with "quote", i.e., quote != '\0'.

> + break;
> +
> + if (escape)
> + {
> + escape = 0;
> + *outp++ = *inp;
> + }
> + else if (*inp == '\\')
> + escape = 1;
> + else if (*inp == quote)
> + quote = 0;
> + else
> + *outp++ = *inp;
> +
> + /* Expand when needed. */
> + if (outp - filename >= capacity)
> + {
> + int len = outp - filename;

size_t len;

>
> - (*cmd) = skip_spaces (*cmd);
> - end = *cmd + strcspn (*cmd, " \t");
> - filename = savestring ((*cmd), end - (*cmd));
> + capacity *= 2;
> + filename = (char *) xrealloc (filename, capacity);
> + outp = filename + len;
> + }
> + }
> +
> + *outp = '\0';

Just for completeness, you could xrealloc (filename, outp - filename +
1) to save space.

> make_cleanup (xfree, filename);
> - (*cmd) = skip_spaces (end);
> + (*cmd) = skip_spaces (inp);
> }
> gdb_assert (filename != NULL);
>
> diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog
> index 698f116..e28d75e 100644
> --- a/gdb/testsuite/ChangeLog
> +++ b/gdb/testsuite/ChangeLog
> @@ -1,3 +1,7 @@
> +2013-06-07 Wei-cheng Wang <cole945@gmail.com>
> +
> + * gdb.base/dump.exp: Test tests for filenames with spaces.
> +
> 2013-06-06 Doug Evans <dje@google.com>
>
> * gdb.cp/derivation.exp: Make tests have unique names.
> diff --git a/gdb/testsuite/gdb.base/dump.exp b/gdb/testsuite/gdb.base/dump.exp
> index b2d0b3a..ab0481c 100644
> --- a/gdb/testsuite/gdb.base/dump.exp
> +++ b/gdb/testsuite/gdb.base/dump.exp
> @@ -105,6 +105,18 @@ make_dump_file "dump bin val intarr1b.bin intarray" \
> make_dump_file "dump bin val intstr1b.bin intstruct" \
> "dump struct as value, binary"
>
> +make_dump_file "dump bin val \"intarr1c .bin\" intarray" \
> + "dump array as value, binary, quoted whitespace"
> +
> +make_dump_file "dump bin val \"intstr1c .bin\" intstruct" \
> + "dump struct as value, binary, quoted whitespace"
> +
> +make_dump_file "dump bin val intarr1d\\ .bin intarray" \
> + "dump array as value, binary, escaped whitespace"
> +
> +make_dump_file "dump bin val intstr1d\\ .bin intstruct" \
> + "dump struct as value, binary, escaped whitespace"
> +
> make_dump_file "dump srec val intarr1.srec intarray" \
> "dump array as value, srec"
>
> @@ -300,6 +312,26 @@ test_restore_saved_value "intstr1.bin binary $struct_start" \
>
> gdb_test "print zero_all ()" ".*"
>
> +test_restore_saved_value "intarr1c\\ .bin binary $array_start" \
> + "array as value, binary, quoted whitespace" \
> + $array_val "intarray"
> +
> +test_restore_saved_value "intstr1c\\ .bin binary $struct_start" \
> + "struct as value, binary, quoted whitespace" \
> + $struct_val "intstruct"
> +
> +gdb_test "print zero_all ()" ".*"
> +
> +test_restore_saved_value "\"intarr1d .bin\" binary $array_start" \
> + "array as value, binary, escape whitespace" \
> + $array_val "intarray"
> +
> +test_restore_saved_value "\"intstr1d .bin\" binary $struct_start" \
> + "struct as value, binary, escape whitespace" \
> + $struct_val "intstruct"
> +
> +gdb_test "print zero_all ()" ".*"
> +
> test_restore_saved_value "intarr2.bin binary $array_start" \
> "array as memory, binary" \
> $array_val "intarray"
> @@ -505,4 +537,4 @@ if ![string compare $is64bitonly "no"] then {
>
> # clean up files
>
> -remote_exec build "rm -f intarr1.bin intarr1b.bin intarr1.ihex
> intarr1.srec intarr1.tekhex intarr2.bin intarr2b.bin intarr2.ihex
> intarr2.srec intarr2.tekhex intstr1.bin intstr1b.bin intstr1.ihex
> intstr1.srec intstr1.tekhex intstr2.bin intstr2b.bin intstr2.ihex
> intstr2.srec intstr2.tekhex intarr3.srec"
> +remote_exec build "rm -f intarr1.bin intarr1b.bin \"intarr1c .bin\"
> \"intarr1d .bin\" intarr1.ihex intarr1.srec intarr1.tekhex intarr2.bin
> intarr2b.bin intarr2.ihex intarr2.srec intarr2.tekhex intstr1.bin
> intstr1b.bin \"intstr1c .bin\" \"intstr1d .bin\" intstr1.ihex
> intstr1.srec intstr1.tekhex intstr2.bin intstr2b.bin intstr2.ihex
> intstr2.srec intstr2.tekhex intarr3.srec"
> --

The rest of the patch looks good to me. I am not a maintainer, you will
need to wait for one to review the patch and give the OK.

Thanks,

--
Sergio