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

[Abhijit Halder <abhijit.k.halder@gmail.com>]

[-- 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\"."