From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 22690 invoked by alias); 14 Oct 2011 11:47:23 -0000 Received: (qmail 22681 invoked by uid 22791); 14 Oct 2011 11:47:21 -0000 X-SWARE-Spam-Status: No, hits=-2.5 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-qw0-f41.google.com (HELO mail-qw0-f41.google.com) (209.85.216.41) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Fri, 14 Oct 2011 11:47:05 +0000 Received: by qadb17 with SMTP id b17so938126qad.0 for ; Fri, 14 Oct 2011 04:47:04 -0700 (PDT) MIME-Version: 1.0 Received: by 10.68.10.70 with SMTP id g6mr16464353pbb.65.1318592823966; Fri, 14 Oct 2011 04:47:03 -0700 (PDT) Received: by 10.142.112.8 with HTTP; Fri, 14 Oct 2011 04:47:03 -0700 (PDT) In-Reply-To: References: Date: Fri, 14 Oct 2011 11:47: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: multipart/mixed; boundary=bcaec53af23ae03cab04af40d029 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/msg00411.txt.bz2 --bcaec53af23ae03cab04af40d029 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable Content-length: 4017 On Fri, Oct 14, 2011 at 3:23 PM, Abhijit Halder wrote: > 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 write= s: >>> >>> Abhijit> In the `set remote exec-file' command if we provide space at t= he 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 wil= l 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 earli= er >>> 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> =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 >> > Adding testcases. Regards, Abhijit Halder --bcaec53af23ae03cab04af40d029 Content-Type: text/plain; charset=US-ASCII; name="ChangeLog-testsuite.txt" Content-Disposition: attachment; filename="ChangeLog-testsuite.txt" Content-Transfer-Encoding: base64 X-Attachment-Id: f_gtr3t94c0 Content-length: 224 MjAxMS0xMC0xNCAgQWJoaWppdCBIYWxkZXIgIDxhYmhpaml0LmsuaGFsZGVy QGdtYWlsLmNvbT4KCgkqIGdkYi5iYXNlL3NldHNob3cuZXhwOiBBZGQgdGVz dC1jYXNlcyBmb3IgYHNldCByZW1vdGUgZXhlYy1maWxlJyBhbmQKCWBzaG93 IHJlbW90ZSBleGVjLWZpbGUnIGNvbW1hbmRzLgo= --bcaec53af23ae03cab04af40d029 Content-Type: text/x-patch; charset=US-ASCII; name="gdb-space-issue-testcase.patch" Content-Disposition: attachment; filename="gdb-space-issue-testcase.patch" Content-Transfer-Encoding: base64 X-Attachment-Id: f_gtr3u52v1 Content-length: 981 SW5kZXg6IGdkYi5iYXNlL3NldHNob3cuZXhwCj09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT0KUkNTIGZpbGU6IC9jdnMvc3JjL3NyYy9nZGIvdGVzdHN1aXRlL2dk Yi5iYXNlL3NldHNob3cuZXhwLHYKcmV0cmlldmluZyByZXZpc2lvbiAxLjIx CmRpZmYgLWEgLXAgLXUgLXIxLjIxIHNldHNob3cuZXhwCi0tLSBnZGIuYmFz ZS9zZXRzaG93LmV4cAkyMCBBcHIgMjAxMSAxNDo1Njo0OSAtMDAwMAkxLjIx CisrKyBnZGIuYmFzZS9zZXRzaG93LmV4cAkxNCBPY3QgMjAxMSAxMTozMzo0 OSAtMDAwMApAQCAtMjYwLDMgKzI2MCw3IEBAIGdkYl90ZXN0ICJzaG93IHZl cmJvc2UiICJWZXJib3NlIHByaW50aW4KIGdkYl90ZXN0X25vX291dHB1dCAi c2V0IHZlcmJvc2Ugb2ZmIiAic2V0IHZlcmJvc2Ugb2ZmIiAKICN0ZXN0IHNo b3cgdmVyYm9zZSBvZmYKIGdkYl90ZXN0ICJzaG93IHZlcmJvc2UiICJWZXJi b3NpdHkgaXMgb2ZmLi4qIiAic2hvdyB2ZXJib3NlIChvZmYpIiAKKyN0ZXN0 IHNldCByZW1vdGUgZXhlYy1maWxlCitnZGJfdGVzdF9ub19vdXRwdXQgInNl dCByZW1vdGUgZXhlYy1maWxlIHh4eCAiIAorI3Rlc3Qgc2hvdyByZW1vdGUg ZXhlYy1maWxlCitnZGJfdGVzdCAic2hvdyByZW1vdGUgZXhlYy1maWxlIiAi VGhlIHJlbW90ZSBwYXRobmFtZSBmb3IgXCJydW5cIiBpcyBcInh4eFwiLiIg Cg== --bcaec53af23ae03cab04af40d029--