[PATCH] PR-10034 Bad space handling in set remote exec-file command

[PATCH] PR-10034 Bad space handling in set remote exec-file command

[Abhijit Halder]

[-- Attachment #1: Type: text/plain, Size: 710 bytes --]

Hi,

In the set remote exec-file command if we provide space at the end of
the file-name the space is not being cleared. This behaviour is
inconsistent across similar set commands like set logging file etc. My
patch will fix that behaviour. Please review this patch.

Further, I have found that there is a function in cli/cli-utils.c
called remove_trailing_whitespace that never used. In many times we
have removed trailing spaces and for that inline code is written. In
my next patch I am planning to modify the remove_trailing_whitespace
function and use it whenever possible in that. Since that patch will
be relevant to current fix I am proposing, I have mentioned here that
point.

Thanks,
Abhijit Halder

[-- Attachment #2: ChangeLog.txt --]
[-- Type: text/plain, Size: 181 bytes --]

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.

[-- Attachment #3: gdb-space-issue.patch --]
[-- Type: text/x-patch, Size: 1167 bytes --]

Index: gdb/cli/cli-setshow.c
===================================================================
RCS file: /cvs/src/src/gdb/cli/cli-setshow.c,v
retrieving revision 1.46
diff -a -p -u -r1.46 cli-setshow.c
--- gdb/cli/cli-setshow.c	4 Aug 2011 19:10:13 -0000	1.46
+++ gdb/cli/cli-setshow.c	25 Sep 2011 08:28:52 -0000
@@ -181,6 +181,14 @@ do_setshow_command (char *arg, int from_
 	    arg = "";
 	  if (*(char **) c->var != NULL)
 	    xfree (*(char **) c->var);
+	  {
+	    /* Clear trailing whitespace of string.  */
+	    char *ptr = arg + strlen (arg) - 1;
+
+	    while (ptr >= arg && (*ptr == ' ' || *ptr == '\t'))
+	      ptr--;
+	    *(ptr + 1) = '\0';
+	  }
 	  *(char **) c->var = xstrdup (arg);
 	  break;
 	case var_optional_filename:
@@ -188,6 +196,14 @@ do_setshow_command (char *arg, int from_
 	    arg = "";
 	  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 = xstrdup (arg);
 	  break;
 	case var_filename:

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

[Abhijit Halder]

On Sun, Sep 25, 2011 at 2:21 PM, Abhijit Halder
<abhijit.k.halder@gmail.com> wrote:
> Hi,
>
> In the set remote exec-file command if we provide space at the end of
> the file-name the space is not being cleared. This behaviour is
> inconsistent across similar set commands like set logging file etc. My
> patch will fix that behaviour. Please review this patch.
>
> Further, I have found that there is a function in cli/cli-utils.c
> called remove_trailing_whitespace that never used. In many times we
> have removed trailing spaces and for that inline code is written. In
> my next patch I am planning to modify the remove_trailing_whitespace
> function and use it whenever possible in that. Since that patch will
> be relevant to current fix I am proposing, I have mentioned here that
> point.
>
> Thanks,
> Abhijit Halder
>

Oops! A mistake. Correcting the same.

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

[Abhijit Halder]

[-- Attachment #1: Type: text/plain, Size: 2446 bytes --]

On Mon, Sep 26, 2011 at 10:52 AM, Abhijit Halder
<abhijit.k.halder@gmail.com> wrote:
> On Sun, Sep 25, 2011 at 2:21 PM, Abhijit Halder
> <abhijit.k.halder@gmail.com> wrote:
>> Hi,
>>
>> In the set remote exec-file command if we provide space at the end of
>> the file-name the space is not being cleared. This behaviour is
>> inconsistent across similar set commands like set logging file etc. My
>> patch will fix that behaviour. Please review this patch.
>>
>> Further, I have found that there is a function in cli/cli-utils.c
>> called remove_trailing_whitespace that never used. In many times we
>> have removed trailing spaces and for that inline code is written. In
>> my next patch I am planning to modify the remove_trailing_whitespace
>> function and use it whenever possible in that. Since that patch will
>> be relevant to current fix I am proposing, I have mentioned here that
>> point.
>>
>> Thanks,
>> Abhijit Halder
>>
>
> Oops! A mistake. Correcting the same.
>


Index: gdb/cli/cli-setshow.c
===================================================================
RCS file: /cvs/src/src/gdb/cli/cli-setshow.c,v
retrieving revision 1.46
diff -a -p -u -r1.46 cli-setshow.c
--- gdb/cli/cli-setshow.c	4 Aug 2011 19:10:13 -0000	1.46
+++ gdb/cli/cli-setshow.c	25 Sep 2011 08:28:52 -0000
@@ -181,6 +181,14 @@ do_setshow_command (char *arg, int from_
 	    arg = "";
 	  if (*(char **) c->var != NULL)
 	    xfree (*(char **) c->var);
+	  {
+	    /* Clear trailing whitespace of string.  */
+	    char *ptr = arg + strlen (arg) - 1;
+
+	    while (ptr >= arg && (*ptr == ' ' || *ptr == '\t'))
+	      ptr--;
+	    *(ptr + 1) = '\0';

>>>>>>>>>>>>Here 'ptr' may point to a read-only memory when 'arg' is passed as NULL pointer.

+	  }
 	  *(char **) c->var = xstrdup (arg);
 	  break;
 	case var_optional_filename:

This case contains same code block as the earlier case. We can fall
through here from earlier case.

@@ -188,6 +196,14 @@ do_setshow_command (char *arg, int from_
 	    arg = "";
 	  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 = xstrdup (arg);
 	  break;
 	case var_filename:

I have made the corrections. Please ignore the earlier patch sent in hurry.


Thanks,
Abhijit Halder

[-- Attachment #2: ChangeLog.txt --]
[-- Type: text/plain, Size: 181 bytes --]

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.

[-- Attachment #3: gdb-space-issue.patch --]
[-- Type: text/x-patch, Size: 1801 bytes --]

Index: gdb/cli/cli-setshow.c
===================================================================
RCS file: /cvs/src/src/gdb/cli/cli-setshow.c,v
retrieving revision 1.46
diff -a -p -u -r1.46 cli-setshow.c
--- gdb/cli/cli-setshow.c	4 Aug 2011 19:10:13 -0000	1.46
+++ gdb/cli/cli-setshow.c	26 Sep 2011 05:36:01 -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;
 	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';
+            }
 	  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 (*(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:

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

[Abhijit Halder]

[-- Attachment #1: Type: text/plain, Size: 2955 bytes --]

On Mon, Sep 26, 2011 at 11:22 AM, Abhijit Halder
<abhijit.k.halder@gmail.com> wrote:
> On Mon, Sep 26, 2011 at 10:52 AM, Abhijit Halder
> <abhijit.k.halder@gmail.com> wrote:
>> On Sun, Sep 25, 2011 at 2:21 PM, Abhijit Halder
>> <abhijit.k.halder@gmail.com> wrote:
>>> Hi,
>>>
>>> In the set remote exec-file command if we provide space at the end of
>>> the file-name the space is not being cleared. This behaviour is
>>> inconsistent across similar set commands like set logging file etc. My
>>> patch will fix that behaviour. Please review this patch.
>>>
>>> Further, I have found that there is a function in cli/cli-utils.c
>>> called remove_trailing_whitespace that never used. In many times we
>>> have removed trailing spaces and for that inline code is written. In
>>> my next patch I am planning to modify the remove_trailing_whitespace
>>> function and use it whenever possible in that. Since that patch will
>>> be relevant to current fix I am proposing, I have mentioned here that
>>> point.
>>>
>>> Thanks,
>>> Abhijit Halder
>>>
>>
>> Oops! A mistake. Correcting the same.
>>
>
>
> Index: gdb/cli/cli-setshow.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/cli/cli-setshow.c,v
> retrieving revision 1.46
> diff -a -p -u -r1.46 cli-setshow.c
> --- gdb/cli/cli-setshow.c       4 Aug 2011 19:10:13 -0000       1.46
> +++ gdb/cli/cli-setshow.c       25 Sep 2011 08:28:52 -0000
> @@ -181,6 +181,14 @@ do_setshow_command (char *arg, int from_
>            arg = "";
>          if (*(char **) c->var != NULL)
>            xfree (*(char **) c->var);
> +         {
> +           /* Clear trailing whitespace of string.  */
> +           char *ptr = arg + strlen (arg) - 1;
> +
> +           while (ptr >= arg && (*ptr == ' ' || *ptr == '\t'))
> +             ptr--;
> +           *(ptr + 1) = '\0';
>
>>>>>>>>>>>>>Here 'ptr' may point to a read-only memory when 'arg' is passed as NULL pointer.
>
> +         }
>          *(char **) c->var = xstrdup (arg);
>          break;
>        case var_optional_filename:
>
> This case contains same code block as the earlier case. We can fall
> through here from earlier case.
>
> @@ -188,6 +196,14 @@ do_setshow_command (char *arg, int from_
>            arg = "";
>          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 = xstrdup (arg);
>          break;
>        case var_filename:
>
> I have made the corrections. Please ignore the earlier patch sent in hurry.
>
>
> Thanks,
> Abhijit Halder
>

Correcting the indentation.

Regards,
Abhijit Halder

[-- Attachment #2: gdb-space-issue.patch --]
[-- Type: text/x-patch, Size: 2034 bytes --]

Index: gdb/cli/cli-setshow.c
===================================================================
RCS file: /cvs/src/src/gdb/cli/cli-setshow.c,v
retrieving revision 1.46
diff -a -p -u -r1.46 cli-setshow.c
--- 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;
 	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';
+	    }
 	  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 (*(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

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

[Abhijit Halder]

[-- Attachment #1: Type: text/plain, Size: 361 bytes --]

Hi,

In the `set remote exec-file' command if we provide space at the end
of the file-name, the space is not being chopped off and being
considered as part of file-name. This behavior is inconsistent across
similar set commands like `set logging file' etc. My patch will fix
that problem. Please review this patch and put your comments.

Thanks,
Abhijit Halder

[-- Attachment #2: ChangeLog.txt --]
[-- Type: text/plain, Size: 181 bytes --]

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.

[-- Attachment #3: gdb-space-issue.patch --]
[-- Type: application/octet-stream, Size: 2034 bytes --]

Index: gdb/cli/cli-setshow.c
===================================================================
RCS file: /cvs/src/src/gdb/cli/cli-setshow.c,v
retrieving revision 1.46
diff -a -p -u -r1.46 cli-setshow.c
--- 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;
 	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';
+	    }
 	  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 (*(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

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

[Tom Tromey]

>>>>> "Abhijit" == Abhijit Halder <abhijit.k.halder@gmail.com> writes:

Abhijit> In the `set remote exec-file' command if we provide space at the end
Abhijit> of the file-name, the space is not being chopped off and being
Abhijit> considered as part of file-name. This behavior is inconsistent across
Abhijit> similar set commands like `set logging file' etc. My patch will fix
Abhijit> that problem. Please review this patch and put your comments.

I can't tell if you re-posted the same patch or if it has changes.
Please:

* If you are sending a ping, just send a ping, as a followup to the
  patch being pinged, so that it threads properly in a threading mail
  reader.

* If you are sending a new patch, again send it as a followup, and also
  indicate what changed and why.


This patch doesn't have a test case, but should.

Abhijit>  	case var_string_noescape:
Abhijit> -	  if (arg == NULL)
Abhijit> -	    arg = "";
Abhijit> -	  if (*(char **) c->var != NULL)
Abhijit> -	    xfree (*(char **) c->var);
Abhijit> -	  *(char **) c->var = xstrdup (arg);
Abhijit> -	  break;

Why are you changing this case?

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

Why not use remove_trailing_whitespace?  You mentioned it in an earlier
note, it seems best to just use it from the start.

What happens if the resulting string is empty?
At least for var_filename this should give an error.

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

This hunk looks like a gratuitous change.

Tom

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

[Abhijit Halder]

On Tue, Oct 4, 2011 at 9:20 PM, Tom Tromey <tromey@redhat.com> wrote:
>>>>>> "Abhijit" == Abhijit Halder <abhijit.k.halder@gmail.com> writes:
>
> Abhijit> In the `set remote exec-file' command if we provide space at the end
> Abhijit> of the file-name, the space is not being chopped off and being
> Abhijit> considered as part of file-name. This behavior is inconsistent across
> Abhijit> similar set commands like `set logging file' etc. My patch will fix
> Abhijit> that problem. Please review this patch and put your comments.
>
> I can't tell if you re-posted the same patch or if it has changes.
> Please:
>
> * If you are sending a ping, just send a ping, as a followup to the
>  patch being pinged, so that it threads properly in a threading mail
>  reader.
>
> * If you are sending a new patch, again send it as a followup, and also
>  indicate what changed and why.
>
Okay, I will follow this in future.
>
> This patch doesn't have a test case, but should.
>
Okay, I will write a test-case.
> Abhijit>        case var_string_noescape:
> Abhijit> -        if (arg == NULL)
> Abhijit> -          arg = "";
> Abhijit> -        if (*(char **) c->var != NULL)
> Abhijit> -          xfree (*(char **) c->var);
> Abhijit> -        *(char **) c->var = xstrdup (arg);
> Abhijit> -        break;
>
> Why are you changing this case?
>
The idea was that the same code block is common between two adjacent
cases and hence can be combined together.
> Abhijit>        case var_optional_filename:
> Abhijit>          if (arg == NULL)
> Abhijit>            arg = "";
> Abhijit> +        else
> Abhijit> +          {
> Abhijit> +            /* Clear trailing whitespace.  */
> Abhijit> +            char *ptr = arg + strlen (arg) - 1;
> Abhijit> +
> Abhijit> +            while (ptr >= arg && (*ptr == ' ' || *ptr == '\t'))
> Abhijit> +              ptr--;
> Abhijit> +            *(ptr + 1) = '\0';
>
> Why not use remove_trailing_whitespace?  You mentioned it in an earlier
> note, it seems best to just use it from the start.
>
Okay, I will do this change. I was just waiting for people's comment on this.
>
> What happens if the resulting string is empty?
> At least for var_filename this should give an error.
>
Okay.
> Abhijit> @@ -419,7 +423,7 @@ cmd_show_list (struct cmd_list_element *
> Abhijit>    for (; list != NULL; list = list->next)
> Abhijit>      {
> Abhijit>        /* If we find a prefix, run its list, prefixing our output by its
> Abhijit> -         prefix (with "show " skipped).  */
> Abhijit> +       prefix (with "show " skipped).  */
> Abhijit>        if (list->prefixlist && !list->abbrev_flag)
> Abhijit>        {
> Abhijit>          struct cleanup *optionlist_chain
>
> This hunk looks like a gratuitous change.
>
This is a 8 space to tab conversion that we followed in rest of the
code. If suggested I may revert back this change.
>
> Tom
>

Regards,
Abhijit Halder

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

[Tom Tromey]

>>>>> "Abhijit" == Abhijit Halder <abhijit.k.halder@gmail.com> writes:

Tom> Why are you changing this case?

Abhijit> The idea was that the same code block is common between two adjacent
Abhijit> cases and hence can be combined together.

I see.

It isn't clear to me that 'string' types should be treated the same as
filenames in this way.  I guess I would be inclined not to change it,
but I don't feel very strongly about it.

Tom

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

[Abhijit Halder]

On Wed, Oct 5, 2011 at 9:48 AM, Abhijit Halder
<abhijit.k.halder@gmail.com> wrote:
> On Tue, Oct 4, 2011 at 9:20 PM, Tom Tromey <tromey@redhat.com> wrote:
>>>>>>> "Abhijit" == Abhijit Halder <abhijit.k.halder@gmail.com> writes:
>>
>> Abhijit> In the `set remote exec-file' command if we provide space at the end
>> Abhijit> of the file-name, the space is not being chopped off and being
>> Abhijit> considered as part of file-name. This behavior is inconsistent across
>> Abhijit> similar set commands like `set logging file' etc. My patch will fix
>> Abhijit> that problem. Please review this patch and put your comments.
>>
>> I can't tell if you re-posted the same patch or if it has changes.
>> Please:
>>
>> * If you are sending a ping, just send a ping, as a followup to the
>>  patch being pinged, so that it threads properly in a threading mail
>>  reader.
>>
>> * If you are sending a new patch, again send it as a followup, and also
>>  indicate what changed and why.
>>
> Okay, I will follow this in future.
>>
>> This patch doesn't have a test case, but should.
>>
> Okay, I will write a test-case.
>> Abhijit>        case var_string_noescape:
>> Abhijit> -        if (arg == NULL)
>> Abhijit> -          arg = "";
>> Abhijit> -        if (*(char **) c->var != NULL)
>> Abhijit> -          xfree (*(char **) c->var);
>> Abhijit> -        *(char **) c->var = xstrdup (arg);
>> Abhijit> -        break;
>>
>> Why are you changing this case?
>>
> The idea was that the same code block is common between two adjacent
> cases and hence can be combined together.
>> Abhijit>        case var_optional_filename:
>> Abhijit>          if (arg == NULL)
>> Abhijit>            arg = "";
>> Abhijit> +        else
>> Abhijit> +          {
>> Abhijit> +            /* Clear trailing whitespace.  */
>> Abhijit> +            char *ptr = arg + strlen (arg) - 1;
>> Abhijit> +
>> Abhijit> +            while (ptr >= arg && (*ptr == ' ' || *ptr == '\t'))
>> Abhijit> +              ptr--;
>> Abhijit> +            *(ptr + 1) = '\0';
>>
>> Why not use remove_trailing_whitespace?  You mentioned it in an earlier
>> note, it seems best to just use it from the start.
Since this same function has similar code block in several places used
for the purpose of removing trailing whiltespaces, I think now we
should go with the current approach of using this code block instead
of using remove_trailing_whitespace function. This may cause
confusion. Please comment on this.
>>
> Okay, I will do this change. I was just waiting for people's comment on this.
>>
>> What happens if the resulting string is empty?
>> At least for var_filename this should give an error.
>>
I did not make any change for var_filename in my patch. As in existing
code in the patch also the empty string will throw error.
> Okay.
>> Abhijit> @@ -419,7 +423,7 @@ cmd_show_list (struct cmd_list_element *
>> Abhijit>    for (; list != NULL; list = list->next)
>> Abhijit>      {
>> Abhijit>        /* If we find a prefix, run its list, prefixing our output by its
>> Abhijit> -         prefix (with "show " skipped).  */
>> Abhijit> +       prefix (with "show " skipped).  */
>> Abhijit>        if (list->prefixlist && !list->abbrev_flag)
>> Abhijit>        {
>> Abhijit>          struct cleanup *optionlist_chain
>>
>> This hunk looks like a gratuitous change.
>>
> This is a 8 space to tab conversion that we followed in rest of the
> code. If suggested I may revert back this change.
>>
>> Tom
>>
>
> Regards,
> Abhijit Halder
>

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

[Abhijit Halder]

[-- Attachment #1: Type: text/plain, Size: 3818 bytes --]

On Fri, Oct 14, 2011 at 3:23 PM, Abhijit Halder
<abhijit.k.halder@gmail.com> wrote:
> On Wed, Oct 5, 2011 at 9:48 AM, Abhijit Halder
> <abhijit.k.halder@gmail.com> wrote:
>> On Tue, Oct 4, 2011 at 9:20 PM, Tom Tromey <tromey@redhat.com> wrote:
>>>>>>>> "Abhijit" == Abhijit Halder <abhijit.k.halder@gmail.com> writes:
>>>
>>> Abhijit> In the `set remote exec-file' command if we provide space at the end
>>> Abhijit> of the file-name, the space is not being chopped off and being
>>> Abhijit> considered as part of file-name. This behavior is inconsistent across
>>> Abhijit> similar set commands like `set logging file' etc. My patch will fix
>>> Abhijit> that problem. Please review this patch and put your comments.
>>>
>>> I can't tell if you re-posted the same patch or if it has changes.
>>> Please:
>>>
>>> * If you are sending a ping, just send a ping, as a followup to the
>>>  patch being pinged, so that it threads properly in a threading mail
>>>  reader.
>>>
>>> * If you are sending a new patch, again send it as a followup, and also
>>>  indicate what changed and why.
>>>
>> Okay, I will follow this in future.
>>>
>>> This patch doesn't have a test case, but should.
>>>
>> Okay, I will write a test-case.
>>> Abhijit>        case var_string_noescape:
>>> Abhijit> -        if (arg == NULL)
>>> Abhijit> -          arg = "";
>>> Abhijit> -        if (*(char **) c->var != NULL)
>>> Abhijit> -          xfree (*(char **) c->var);
>>> Abhijit> -        *(char **) c->var = xstrdup (arg);
>>> Abhijit> -        break;
>>>
>>> Why are you changing this case?
>>>
>> The idea was that the same code block is common between two adjacent
>> cases and hence can be combined together.
>>> Abhijit>        case var_optional_filename:
>>> Abhijit>          if (arg == NULL)
>>> Abhijit>            arg = "";
>>> Abhijit> +        else
>>> Abhijit> +          {
>>> Abhijit> +            /* Clear trailing whitespace.  */
>>> Abhijit> +            char *ptr = arg + strlen (arg) - 1;
>>> Abhijit> +
>>> Abhijit> +            while (ptr >= arg && (*ptr == ' ' || *ptr == '\t'))
>>> Abhijit> +              ptr--;
>>> Abhijit> +            *(ptr + 1) = '\0';
>>>
>>> Why not use remove_trailing_whitespace?  You mentioned it in an earlier
>>> note, it seems best to just use it from the start.
> Since this same function has similar code block in several places used
> for the purpose of removing trailing whiltespaces, I think now we
> should go with the current approach of using this code block instead
> of using remove_trailing_whitespace function. This may cause
> confusion. Please comment on this.
>>>
>> Okay, I will do this change. I was just waiting for people's comment on this.
>>>
>>> What happens if the resulting string is empty?
>>> At least for var_filename this should give an error.
>>>
> I did not make any change for var_filename in my patch. As in existing
> code in the patch also the empty string will throw error.
>> Okay.
>>> Abhijit> @@ -419,7 +423,7 @@ cmd_show_list (struct cmd_list_element *
>>> Abhijit>    for (; list != NULL; list = list->next)
>>> Abhijit>      {
>>> Abhijit>        /* If we find a prefix, run its list, prefixing our output by its
>>> Abhijit> -         prefix (with "show " skipped).  */
>>> Abhijit> +       prefix (with "show " skipped).  */
>>> Abhijit>        if (list->prefixlist && !list->abbrev_flag)
>>> Abhijit>        {
>>> Abhijit>          struct cleanup *optionlist_chain
>>>
>>> This hunk looks like a gratuitous change.
>>>
>> This is a 8 space to tab conversion that we followed in rest of the
>> code. If suggested I may revert back this change.
>>>
>>> Tom
>>>
>>
>> Regards,
>> Abhijit Halder
>>
>



Adding testcases.

Regards,
Abhijit Halder

[-- Attachment #2: ChangeLog-testsuite.txt --]
[-- Type: text/plain, Size: 164 bytes --]

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.

[-- Attachment #3: gdb-space-issue-testcase.patch --]
[-- Type: text/x-patch, Size: 721 bytes --]

Index: gdb.base/setshow.exp
===================================================================
RCS file: /cvs/src/src/gdb/testsuite/gdb.base/setshow.exp,v
retrieving revision 1.21
diff -a -p -u -r1.21 setshow.exp
--- gdb.base/setshow.exp	20 Apr 2011 14:56:49 -0000	1.21
+++ 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\"."