Re: PING: [PATCH] PR-10034 Bad space handling in `set remote exec-file' command.

[Jan Kratochvil <jan dot kratochvil at redhat dot com>]

On Thu, 20 Oct 2011 15:02:43 +0200, Abhijit Halder wrote:
> Content-Type: application/octet-stream; name="gdb-space-issue.patch"
> Content-Disposition: attachment; filename="gdb-space-issue.patch"
> Content-Transfer-Encoding: base64

 * It should be Content-Type: text/plain, so that one can reply it for
   a review.
 * It should be Content-Disposition: inline, so that one can see the file
   while reading mails.
 * It should be Content-Transfer-Encoding: 7bit, it is reported people may use
   non-MIME MUAs.
 * It is preferred as inlined with text, without any MIME parts = without any
   attachment(s), but this is not always suitable for the sender's MUA so
   a single attachment is OK.  But when attachment then it should be
   1 attachment, having to save + merge 4 files before one starts manipulating
   with the patch is a needless burden. 
 * It is partially a standard to have filenames for patch -p1, that is
   ./gdb/linux-nat.c etc., they same way they are created by GIT, so that all
   patches one can apply the same way.


> My patch will fix that problem.

This patch has a regression.  Please check + fix testsuite regressions before
posting a patch for review.

-PASS: gdb.python/python.exp: set basic extended prompt
-PASS: gdb.python/python.exp: set extended prompt working directory
-PASS: gdb.python/python.exp: set extended prompt parameter
+FAIL: gdb.python/python.exp: set basic extended prompt (timeout)
+FAIL: gdb.python/python.exp: set extended prompt working directory (timeout)
+FAIL: gdb.python/python.exp: set extended prompt parameter (timeout)


> gdb/
> 2011-09-13  Abhijit Halder  <abhijit.k.halder@gmail.com>
> 
> 	Fix PR remote/10034:
> 	* cli/cli-setshow.c (do_setshow_command): Clear trailing whitespace
> 	from command argument strings.
> 
> gdb/testsuite/
> 2011-10-14  Abhijit Halder  <abhijit.k.halder@gmail.com>
> 
> 	* gdb.base/setshow.exp: Add test-cases for `set remote exec-file' and
> 	`show remote exec-file' commands.
> 
> --- ./gdb/cli/cli-setshow.c	4 Aug 2011 19:10:13 -0000	1.46
> +++ ./gdb/cli/cli-setshow.c	29 Sep 2011 07:39:45 -0000
> @@ -177,15 +177,18 @@ do_setshow_command (char *arg, int from_
>  	  }
>  	  break;
>  	case var_string_noescape:
> -	  if (arg == NULL)
> -	    arg = "";
> -	  if (*(char **) c->var != NULL)
> -	    xfree (*(char **) c->var);
> -	  *(char **) c->var = xstrdup (arg);
> -	  break;

There is already a bug.  The bug should be rather fixed.
var_optional_filename should use tilde_expand like var_filename does.

Tom already said so.


>  	case var_optional_filename:
>  	  if (arg == NULL)
>  	    arg = "";
> +	  else
> +	    {
> +	      /* Clear trailing whitespace.  */
> +	      char *ptr = arg + strlen (arg) - 1;
> +
> +	      while (ptr >= arg && (*ptr == ' ' || *ptr == '\t'))
> +		ptr--;
> +	      *(ptr + 1) = '\0';

Tom Tromey suggested to use remove_trailing_whitespace instead, you said you
will do so but it did not happen.  OTOH if you do not want to use it
remove_trailing_whitespace gets dropped as it is unused now (I have removed
its last callers by 67e102403d7a0b16395389587713e703b0267ab3 as I see now).

Also maybe when remove_trailing_whitespace has no longer any users a different
function would be suitable here (which does the final *ptr = '\0';).


> +	    }
>  	  if (*(char **) c->var != NULL)
>  	    xfree (*(char **) c->var);
>  	  *(char **) c->var = xstrdup (arg);
> @@ -193,16 +196,17 @@ do_setshow_command (char *arg, int from_
>  	case var_filename:
>  	  if (arg == NULL)
>  	    error_no_arg (_("filename to set it to."));
> +	  else
> +	    {
> +	      /* Clear trailing whitespace.  */
> +	      char *ptr = arg + strlen (arg) - 1;
> +
> +	      while (ptr >= arg && (*ptr == ' ' || *ptr == '\t'))
> +		ptr--;
> +	      *(ptr + 1) = '\0';
> +	    }

If you calear all the spaces and nothing remains you should call error_no_arg
and it should have a new testcase.

Tom already said so.

>  	  if (*(char **) c->var != NULL)
>  	    xfree (*(char **) c->var);
> -	  {
> -	    /* Clear trailing whitespace of filename.  */
> -	    char *ptr = arg + strlen (arg) - 1;
> -
> -	    while (ptr >= arg && (*ptr == ' ' || *ptr == '\t'))
> -	      ptr--;
> -	    *(ptr + 1) = '\0';
> -	  }
>  	  *(char **) c->var = tilde_expand (arg);
>  	  break;
>  	case var_boolean:


> @@ -419,7 +423,7 @@ cmd_show_list (struct cmd_list_element *
>    for (; list != NULL; list = list->next)
>      {
>        /* If we find a prefix, run its list, prefixing our output by its
> -         prefix (with "show " skipped).  */
> +	 prefix (with "show " skipped).  */
>        if (list->prefixlist && !list->abbrev_flag)
>  	{
>  	  struct cleanup *optionlist_chain

This is a "code cleanup" only part, it should go separately.


> --- ./gdb/testsuite/gdb.base/setshow.exp	20 Apr 2011 14:56:49 -0000	1.21
> +++ ./gdb/testsuite/gdb.base/setshow.exp	14 Oct 2011 11:33:49 -0000
> @@ -260,3 +260,7 @@ gdb_test "show verbose" "Verbose printin
>  gdb_test_no_output "set verbose off" "set verbose off" 
>  #test show verbose off
>  gdb_test "show verbose" "Verbosity is off..*" "show verbose (off)" 
> +#test set remote exec-file
> +gdb_test_no_output "set remote exec-file xxx " 
> +#test show remote exec-file

> +gdb_test "show remote exec-file" "The remote pathname for \"run\" is \"xxx\"." 

More strict match is either:
gdb_test "show remote exec-file" "The remote pathname for \"run\" is \"xxx\"\\."
or:
gdb_test "show remote exec-file" {The remote pathname for "run" is "xxx"\.}


Thanks,
Jan