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

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

[Wei-cheng Wang]

Hi,

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

Without this fix, we cannot dump to or restore from a file in a path that has spaces.

Before fixing.

$ make check RUNTESTFLAGS="dump.exp" | grep "FAIL" | sort -u | uniq -c
   1 FAIL: gdb.base/dump.exp: array as value, binary, escape whitespace; file restored ok
   1 FAIL: gdb.base/dump.exp: array as value, binary, escape whitespace; value restored ok
   1 FAIL: gdb.base/dump.exp: array as value, binary, quoted whitespace; file restored ok
   1 FAIL: gdb.base/dump.exp: array as value, binary, quoted whitespace; value restored ok
   1 FAIL: gdb.base/dump.exp: dump array as value, binary, escaped whitespace
   1 FAIL: gdb.base/dump.exp: dump array as value, binary, quoted whitespace
   1 FAIL: gdb.base/dump.exp: dump struct as value, binary, escaped whitespace
   1 FAIL: gdb.base/dump.exp: dump struct as value, binary, quoted whitespace
   1 FAIL: gdb.base/dump.exp: struct as value, binary, escape whitespace; file restored ok
   1 FAIL: gdb.base/dump.exp: struct as value, binary, escape whitespace; value restored ok
   1 FAIL: gdb.base/dump.exp: struct as value, binary, quoted whitespace; file restored ok
   1 FAIL: gdb.base/dump.exp: struct as value, binary, quoted whitespace; value restored ok

After fixing.

# of expected passes            199


---
  gdb/ChangeLog                   |    5 ++++
  gdb/cli/cli-dump.c              |   52 ++++++++++++++++++++++++++++++++++-----
  gdb/testsuite/ChangeLog         |    4 +++
  gdb/testsuite/gdb.base/dump.exp |   34 ++++++++++++++++++++++++-
  4 files changed, 88 insertions(+), 7 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 5fdb1f8..e0b1c53 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,8 @@
+2013-06-07  Wei-cheng Wang  <cole945@gmail.com>
+
+	* cli/cli-dump.c (scan_filename_with_cleanup): Parse quoted or
+	escaped string when scanning filename.
+
  2013-06-05  Doug Evans  <dje@google.com>
  	    Keith Seitz  <keiths@redhat.com>

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

-      (*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';
        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"
-- 

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

[Wei-cheng Wang]

Ping

Any suggestion?

--
Wei-cheng

On Fri, Jun 7, 2013 at 11:49 PM, Wei-cheng Wang <cole945@gmail.com> wrote:
> Hi,
>
> This is a patch for parsing quoted string for restore/dump commands.
> Test cases are also added.
>
> Without this fix, we cannot dump to or restore from a file in a path that
> has spaces.
>
> Before fixing.
>
> $ make check RUNTESTFLAGS="dump.exp" | grep "FAIL" | sort -u | uniq -c
>   1 FAIL: gdb.base/dump.exp: array as value, binary, escape whitespace; file
> restored ok
>   1 FAIL: gdb.base/dump.exp: array as value, binary, escape whitespace;
> value restored ok
>   1 FAIL: gdb.base/dump.exp: array as value, binary, quoted whitespace; file
> restored ok
>   1 FAIL: gdb.base/dump.exp: array as value, binary, quoted whitespace;
> value restored ok
>   1 FAIL: gdb.base/dump.exp: dump array as value, binary, escaped whitespace
>   1 FAIL: gdb.base/dump.exp: dump array as value, binary, quoted whitespace
>   1 FAIL: gdb.base/dump.exp: dump struct as value, binary, escaped
> whitespace
>   1 FAIL: gdb.base/dump.exp: dump struct as value, binary, quoted whitespace
>   1 FAIL: gdb.base/dump.exp: struct as value, binary, escape whitespace;
> file restored ok
>   1 FAIL: gdb.base/dump.exp: struct as value, binary, escape whitespace;
> value restored ok
>   1 FAIL: gdb.base/dump.exp: struct as value, binary, quoted whitespace;
> file restored ok
>   1 FAIL: gdb.base/dump.exp: struct as value, binary, quoted whitespace;
> value restored ok
>
> After fixing.
>
> # of expected passes            199
>
>
> ---
>  gdb/ChangeLog                   |    5 ++++
>  gdb/cli/cli-dump.c              |   52
> ++++++++++++++++++++++++++++++++++-----
>  gdb/testsuite/ChangeLog         |    4 +++
>  gdb/testsuite/gdb.base/dump.exp |   34 ++++++++++++++++++++++++-
>  4 files changed, 88 insertions(+), 7 deletions(-)
>
> diff --git a/gdb/ChangeLog b/gdb/ChangeLog
> index 5fdb1f8..e0b1c53 100644
> --- a/gdb/ChangeLog
> +++ b/gdb/ChangeLog
> @@ -1,3 +1,8 @@
> +2013-06-07  Wei-cheng Wang  <cole945@gmail.com>
> +
> +       * cli/cli-dump.c (scan_filename_with_cleanup): Parse quoted or
> +       escaped string when scanning filename.
> +
>  2013-06-05  Doug Evans  <dje@google.com>
>             Keith Seitz  <keiths@redhat.com>
>
> 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;
> +
> +      inp = (*cmd) = skip_spaces (*cmd);
> +      /* 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);
> +
> +      /* Quoting character.  */
> +      if (*inp == '\'' || *inp == '"')
> +       quote = *inp++;
> +
> +      for (; *inp != '\0'; inp++)
> +       {
> +         /* Stop by whitespace, if not quoted or escaped. */
> +         if (whitespace (*inp) && !escape && !quote)
> +           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;
>
> -      (*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';
>        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"
> --

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

[Sergio Durigan Junior]

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

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

[Wei-cheng Wang]

Hi Sergio,

Thank you for you review.
I've fixed them accordingly at the end of the mail.

On Sun, Jun 16, 2013 at 2:37 AM, Sergio Durigan Junior
<sergiodj@redhat.com> wrote:
> On Friday, June 07 2013, Wei-cheng Wang wrote:
>
> 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.

  We've signed the copryright assignment in GDB, on behalf of Andes Technology
  in Dec. 2009.  Is that sufficent?

>>
>> -      (*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.

  I was considering the trade-off between time and space.
  Assuming in most cases, there is no space in the path,
  the initial size of the buffer is the length to the first delimiter.
  If we expand the buffer only 1 more byte a time, xrealloc might be
  called much more times than double the size, so I use the same policy
  of buildargv.

  Any suggestion?

--
Wei-cheng

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 7ae0797..a061f27 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,9 @@
+2013-06-16  Wei-cheng Wang  <cole945@gmail.com>
+	    Sergio Durigan Junior  <sergiodj@redhat.com>
+
+	* cli/cli-dump.c (scan_filename_with_cleanup): Parse quoted or
+	escaped string when scanning filename.
+
 2013-06-13  Doug Evans  <dje@google.com>

 	* dwarf2read.c (try_open_dwop_file): Work around behaviour of
diff --git a/gdb/cli/cli-dump.c b/gdb/cli/cli-dump.c
index 208916c..06003a6 100644
--- a/gdb/cli/cli-dump.c
+++ b/gdb/cli/cli-dump.c
@@ -80,14 +80,55 @@ 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;
+      size_t capacity;
+
+      inp = skip_spaces (*cmd);
+      /* Allocate initial buffer for filename.  If there is no space
+	 in quoted string, this should be the exactly size what we
+	 need.  */
+      capacity = strcspn (inp, " \t") + 1;
+      filename = xmalloc (capacity);
+      outp = filename;
+
+      /* Quoting character.  */
+      if (*inp == '\'' || *inp == '"')
+	quote = *inp++;
+
+      for (; *inp != '\0'; inp++)
+	{
+	  /* Stop by whitespace, if not quoted or escaped.  */
+	  if (isspace (*inp) && !escape && 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)
+	    {
+	      size_t len = outp - filename;

-      (*cmd) = skip_spaces (*cmd);
-      end = *cmd + strcspn (*cmd, " \t");
-      filename = savestring ((*cmd), end - (*cmd));
+	      capacity *= 2;
+	      filename = xrealloc (filename, capacity);
+	      outp = filename + len;
+	    }
+	}
+
+     *outp = '\0';
       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 06217f9..77bc653 100644
--- a/gdb/testsuite/ChangeLog
+++ b/gdb/testsuite/ChangeLog
@@ -1,3 +1,7 @@
+2013-06-16  Wei-cheng Wang  <cole945@gmail.com>
+
+	* gdb.base/dump.exp: Test tests for filenames with spaces.
+
 2013-06-07  Pedro Alves  <palves@redhat.com>

 	* boards/native-extended-gdbserver.exp: Remove semicolon.
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"

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

[Sergio Durigan Junior]

On Monday, June 17 2013, Wei-cheng Wang wrote:

> Hi Sergio,
>
> Thank you for you review.
> I've fixed them accordingly at the end of the mail.

Thanks, Wei.

> On Sun, Jun 16, 2013 at 2:37 AM, Sergio Durigan Junior
> <sergiodj@redhat.com> wrote:
>> On Friday, June 07 2013, Wei-cheng Wang wrote:
>>
>> 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.
>
>   We've signed the copryright assignment in GDB, on behalf of Andes Technology
>   in Dec. 2009.  Is that sufficent?

It is probably fine then, but I am not sure.  Some companies have
copyright assignments for all of its employees without restrictions,
while others make copyright assignments that don't cover everything.  A
maintainer is the right person to check this.

Can some maintainer take a look for Wei, please?

>>>
>>> -      (*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.
>
>   I was considering the trade-off between time and space.
>   Assuming in most cases, there is no space in the path,
>   the initial size of the buffer is the length to the first delimiter.
>   If we expand the buffer only 1 more byte a time, xrealloc might be
>   called much more times than double the size, so I use the same policy
>   of buildargv.

Oh, sorry, I should have been clearer.  I was not referring to the
xrealloc inside the for/if, but rather I was saying that you could call
xrealloc *again* after you set "*outp = '\0'", after the loop finishes.

When the loop finishes you might end up with "filename" being larger
than you need.

> diff --git a/gdb/ChangeLog b/gdb/ChangeLog
> index 7ae0797..a061f27 100644
> --- a/gdb/ChangeLog
> +++ b/gdb/ChangeLog
> @@ -1,3 +1,9 @@
> +2013-06-16  Wei-cheng Wang  <cole945@gmail.com>
> +	    Sergio Durigan Junior  <sergiodj@redhat.com>
> +
> +	* cli/cli-dump.c (scan_filename_with_cleanup): Parse quoted or
> +	escaped string when scanning filename.
> +

The rest of the patch looks OK to me now.  You will have to wait for a
maintainer to (a) say if you're clear to commit the patch (due to the
copyright assignment matter), and (b) actually approve the patch.

Also, when you commit the patch, please remember to include your info on
gdb/MAINTAINERS, since you will be able to commit after approval.

Thanks,

-- 
Sergio

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

[Sergio Durigan Junior]

On Thursday, June 20 2013, I wrote:

> Also, when you commit the patch, please remember to include your info on
> gdb/MAINTAINERS, since you will be able to commit after approval.

Just to be totally clear: this modification to gdb/MAINTAINERS should be
considered a totally different patch, and should not be
included/committed with this one :-).

-- 
Sergio

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

[Wei-cheng Wang]

Hi Sergio,

On Thu, Jun 20, 2013 at 1:11 PM, Sergio Durigan Junior
<sergiodj@redhat.com> wrote:
> On Thursday, June 20 2013, I wrote:
> Oh, sorry, I should have been clearer.  I was not referring to the
> xrealloc inside the for/if, but rather I was saying that you could call
> xrealloc *again* after you set "*outp = '\0'", after the loop finishes.
>
> When the loop finishes you might end up with "filename" being larger
> than you need.

  I've fixed it accordingly at the end of the mail.
  Thanks for you suggestion.

>
>> Also, when you commit the patch, please remember to include your info on
>> gdb/MAINTAINERS, since you will be able to commit after approval.
>
> Just to be totally clear: this modification to gdb/MAINTAINERS should be
> considered a totally different patch, and should not be
> included/committed with this one :-).

  Actually, I didn't have CVS write access yet.  I may need a sponsor.
  Before that, I need someone help me checking this patch in, if approved :-)

--

 gdb/ChangeLog                   |    6 +++++
 gdb/cli/cli-dump.c              |   54 ++++++++++++++++++++++++++++++++++-----
 gdb/testsuite/ChangeLog         |    4 +++
 gdb/testsuite/gdb.base/dump.exp |   34 +++++++++++++++++++++++-
 4 files changed, 91 insertions(+), 7 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 1efd3f9..b7a87eb 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,9 @@
+2013-06-24  Wei-cheng Wang  <cole945@gmail.com>
+	    Sergio Durigan Junior  <sergiodj@redhat.com>
+
+	* cli/cli-dump.c (scan_filename_with_cleanup): Parse quoted or
+	escaped string when scanning filename.
+
 2013-06-21  Joel Brobecker  <brobecker@adacore.com>

 	* gdb/gnulib/Makefile.in: Update date in copyright header.
diff --git a/gdb/cli/cli-dump.c b/gdb/cli/cli-dump.c
index 208916c..70fdfee 100644
--- a/gdb/cli/cli-dump.c
+++ b/gdb/cli/cli-dump.c
@@ -80,14 +80,56 @@ 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;
+      size_t capacity;
+
+      inp = skip_spaces (*cmd);
+      /* Allocate initial buffer for filename.  If there is no space
+	 in quoted string, this should be the exactly size what we
+	 need.  */
+      capacity = strcspn (inp, " \t") + 1;
+      filename = xmalloc (capacity);
+      outp = filename;
+
+      /* Quoting character.  */
+      if (*inp == '\'' || *inp == '"')
+	quote = *inp++;
+
+      for (; *inp != '\0'; inp++)
+	{
+	  /* Stop by whitespace, if not quoted or escaped.  */
+	  if (isspace (*inp) && !escape && 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)
+	    {
+	      size_t len = outp - filename;

-      (*cmd) = skip_spaces (*cmd);
-      end = *cmd + strcspn (*cmd, " \t");
-      filename = savestring ((*cmd), end - (*cmd));
+	      capacity *= 2;
+	      filename = xrealloc (filename, capacity);
+	      outp = filename + len;
+	    }
+	}
+
+      *outp = '\0';
+      filename = xrealloc (filename, outp - filename + 1);
       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 9abc780..bdfa065 100644
--- a/gdb/testsuite/ChangeLog
+++ b/gdb/testsuite/ChangeLog
@@ -1,3 +1,7 @@
+2013-06-24  Wei-cheng Wang  <cole945@gmail.com>
+
+	* gdb.base/dump.exp: Test tests for filenames with spaces.
+
 2013-06-21  Tom Tromey  <tromey@redhat.com>

 	* gdb.trace/actions.exp (check_tracepoint): Don't use a full file
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"
--

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

[Sergio Durigan Junior]

Hi Wei-cheng,

On Monday, June 24 2013, Wei-cheng Wang wrote:

>>> Also, when you commit the patch, please remember to include your info on
>>> gdb/MAINTAINERS, since you will be able to commit after approval.
>>
>> Just to be totally clear: this modification to gdb/MAINTAINERS should be
>> considered a totally different patch, and should not be
>> included/committed with this one :-).
>
>   Actually, I didn't have CVS write access yet.  I may need a sponsor.

Yeah, a sponsor needs to be a maintainer, so I can't help you with that
:-(.

>   Before that, I need someone help me checking this patch in, if approved :-)

If the patch is approved, I can check it in for you, no problem :-).

> --
>
>  gdb/ChangeLog                   |    6 +++++
>  gdb/cli/cli-dump.c              |   54 ++++++++++++++++++++++++++++++++++-----
>  gdb/testsuite/ChangeLog         |    4 +++
>  gdb/testsuite/gdb.base/dump.exp |   34 +++++++++++++++++++++++-
>  4 files changed, 91 insertions(+), 7 deletions(-)
>
> diff --git a/gdb/ChangeLog b/gdb/ChangeLog
> index 1efd3f9..b7a87eb 100644
> --- a/gdb/ChangeLog
> +++ b/gdb/ChangeLog
> @@ -1,3 +1,9 @@
> +2013-06-24  Wei-cheng Wang  <cole945@gmail.com>
> +	    Sergio Durigan Junior  <sergiodj@redhat.com>

No need to put my name in the entry :-).  I just reviewed your patch,
but the idea and the work are still yours.

-- 
Sergio

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

[Joel Brobecker]

> It is probably fine then, but I am not sure.  Some companies have
> copyright assignments for all of its employees without restrictions,
> while others make copyright assignments that don't cover everything.  A
> maintainer is the right person to check this.
> 
> Can some maintainer take a look for Wei, please?

Sure. Just done. AFAICT, the copyright assignment is inclusive of
all employees.

-- 
Joel