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

[Sergio Durigan Junior <sergiodj at redhat dot com>]

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