From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 3964 invoked by alias); 14 Oct 2011 09:53:33 -0000 Received: (qmail 3946 invoked by uid 22791); 14 Oct 2011 09:53:30 -0000 X-SWARE-Spam-Status: No, hits=-2.4 required=5.0 tests=AWL,BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,FREEMAIL_FROM,RCVD_IN_DNSWL_LOW X-Spam-Check-By: sourceware.org Received: from mail-qy0-f169.google.com (HELO mail-qy0-f169.google.com) (209.85.216.169) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Fri, 14 Oct 2011 09:53:15 +0000 Received: by qyl38 with SMTP id 38so945035qyl.0 for ; Fri, 14 Oct 2011 02:53:14 -0700 (PDT) MIME-Version: 1.0 Received: by 10.68.34.226 with SMTP id c2mr15740437pbj.99.1318585993823; Fri, 14 Oct 2011 02:53:13 -0700 (PDT) Received: by 10.142.112.8 with HTTP; Fri, 14 Oct 2011 02:53:13 -0700 (PDT) In-Reply-To: References: Date: Fri, 14 Oct 2011 09:53:00 -0000 Message-ID: Subject: Re: [PATCH] PR-10034 Bad space handling in `set remote exec-file' command. From: Abhijit Halder To: Tom Tromey Cc: "gdb-patches@sourceware.org ml" Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable X-IsSubscribed: yes Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2011-10/txt/msg00410.txt.bz2 On Wed, Oct 5, 2011 at 9:48 AM, Abhijit Halder wrote: > On Tue, Oct 4, 2011 at 9:20 PM, Tom Tromey wrote: >>>>>>> "Abhijit" =3D=3D Abhijit Halder writes: >> >> Abhijit> In the `set remote exec-file' command if we provide space at th= e 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 >> =A0patch being pinged, so that it threads properly in a threading mail >> =A0reader. >> >> * If you are sending a new patch, again send it as a followup, and also >> =A0indicate 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> =A0 =A0 =A0 =A0case var_string_noescape: >> Abhijit> - =A0 =A0 =A0 =A0if (arg =3D=3D NULL) >> Abhijit> - =A0 =A0 =A0 =A0 =A0arg =3D ""; >> Abhijit> - =A0 =A0 =A0 =A0if (*(char **) c->var !=3D NULL) >> Abhijit> - =A0 =A0 =A0 =A0 =A0xfree (*(char **) c->var); >> Abhijit> - =A0 =A0 =A0 =A0*(char **) c->var =3D xstrdup (arg); >> Abhijit> - =A0 =A0 =A0 =A0break; >> >> 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> =A0 =A0 =A0 =A0case var_optional_filename: >> Abhijit> =A0 =A0 =A0 =A0 =A0if (arg =3D=3D NULL) >> Abhijit> =A0 =A0 =A0 =A0 =A0 =A0arg =3D ""; >> Abhijit> + =A0 =A0 =A0 =A0else >> Abhijit> + =A0 =A0 =A0 =A0 =A0{ >> Abhijit> + =A0 =A0 =A0 =A0 =A0 =A0/* Clear trailing whitespace. =A0*/ >> Abhijit> + =A0 =A0 =A0 =A0 =A0 =A0char *ptr =3D arg + strlen (arg) - 1; >> Abhijit> + >> Abhijit> + =A0 =A0 =A0 =A0 =A0 =A0while (ptr >=3D arg && (*ptr =3D=3D ' = ' || *ptr =3D=3D '\t')) >> Abhijit> + =A0 =A0 =A0 =A0 =A0 =A0 =A0ptr--; >> Abhijit> + =A0 =A0 =A0 =A0 =A0 =A0*(ptr + 1) =3D '\0'; >> >> Why not use remove_trailing_whitespace? =A0You 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 t= his. >> >> 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> =A0 =A0for (; list !=3D NULL; list =3D list->next) >> Abhijit> =A0 =A0 =A0{ >> Abhijit> =A0 =A0 =A0 =A0/* If we find a prefix, run its list, prefixing = our output by its >> Abhijit> - =A0 =A0 =A0 =A0 prefix (with "show " skipped). =A0*/ >> Abhijit> + =A0 =A0 =A0 prefix (with "show " skipped). =A0*/ >> Abhijit> =A0 =A0 =A0 =A0if (list->prefixlist && !list->abbrev_flag) >> Abhijit> =A0 =A0 =A0 =A0{ >> Abhijit> =A0 =A0 =A0 =A0 =A0struct 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 >