From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 14820 invoked by alias); 5 Oct 2011 04:19:05 -0000 Received: (qmail 14797 invoked by uid 22791); 5 Oct 2011 04:19:01 -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-yw0-f41.google.com (HELO mail-yw0-f41.google.com) (209.85.213.41) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Wed, 05 Oct 2011 04:18:46 +0000 Received: by ywe9 with SMTP id 9so1415471ywe.0 for ; Tue, 04 Oct 2011 21:18:46 -0700 (PDT) MIME-Version: 1.0 Received: by 10.68.29.228 with SMTP id n4mr15171123pbh.64.1317788325765; Tue, 04 Oct 2011 21:18:45 -0700 (PDT) Received: by 10.142.52.7 with HTTP; Tue, 4 Oct 2011 21:18:45 -0700 (PDT) In-Reply-To: References: Date: Wed, 05 Oct 2011 04:19: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/msg00123.txt.bz2 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 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 a= cross > 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. > Okay, I will do this change. I was just waiting for people's comment on thi= s. > > 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> =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 o= ur 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