From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 24393 invoked by alias); 16 Jan 2009 22:08:47 -0000 Received: (qmail 24383 invoked by uid 22791); 16 Jan 2009 22:08:46 -0000 X-SWARE-Spam-Status: No, hits=-0.1 required=5.0 tests=AWL,BAYES_00,J_CHICKENPOX_65,J_CHICKENPOX_74,MISSING_HEADERS,SARE_MSGID_LONG40,SPF_PASS X-Spam-Check-By: sourceware.org Received: from yw-out-1718.google.com (HELO yw-out-1718.google.com) (74.125.46.155) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Fri, 16 Jan 2009 22:08:09 +0000 Received: by yw-out-1718.google.com with SMTP id 9so869236ywk.48 for ; Fri, 16 Jan 2009 14:08:07 -0800 (PST) MIME-Version: 1.0 Received: by 10.100.138.10 with SMTP id l10mr2427328and.25.1232143686779; Fri, 16 Jan 2009 14:08:06 -0800 (PST) In-Reply-To: References: Date: Fri, 16 Jan 2009 22:08:00 -0000 Message-ID: Subject: Re: Use external editor in 'commands' command From: Alfredo Ortega Cc: gdb-patches@sourceware.org Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit 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: 2009-01/txt/msg00389.txt.bz2 2009/1/16 Eli Zaretskii : >> Date: Fri, 16 Jan 2009 05:38:41 -0200 >> From: Alfredo Ortega >> >> Thanks for the corrections. Here are are both changelogs and the >> updated diff, I hope there are less errors now... >> I promise that my next patches will be better! > > No one is born with this, so there's nothing wrong in making such > minor mistakes. > > I have several comments to your code: > >> + char vitmp[50]; > > vitmp[] is a file name, so 50 is not nearly enough characters to hold > the longest possible name. At the very least, please use > FILENAME_MAX, or (better) some dynamic code that grows it as needed, > because some hosts (e.g., Hurd) don't have any limitations on file > name length. > >> + char cmdline[100]; > > Same here: 100 is not enough, because $EDITOR holds a file name. > >> + if (!strcmp(COMMANDS_EDCOMMAND,p)) { > > This is not GNU style of laying out brace-delimited blocks; please use > the same style and indentation as elsewhere in GDB sources. > >> + strcpy(vitmp,"/tmp/.gdbXXXXXX"); > > Please leave a blank between the function name and the left > parenthesis, and also between the comma and the following argument in > argument lists. Like this: > > strcpy (vitmp, "/tmp/.gdbXXXXXX"); > > Btw, using "/tmp/.gdbXXXXXX" is non-portable to Windows, where there's > no guarantee there will be a "/tmp" on the current drive, and it will > not work at all in the DJGPP (a.k.a. DOS) port of GDB, because DOS > filesystems do not allow file names with a leading dot. > >> + /* Generates the temporal file name*/ > ^^^^^^^^ > "temporary" > >> + /* vitmp = tempnam(NULL,".gdb"); this is more secure according to man mkstemp, but gcc complains... */ > > What complaint do you see? > >> + if (mkstemp(vitmp)<0) return; > > Are you using mkstemp from libiberty? Because otherwise it may not be > available on the host. > >> + if (fsizeline)+1) { >> + fclose(tmpstream); >> + unlink(vitmp); >> + return; >> + }; > > Don't we want some error message in this case? > >> + if ((editor = (char *) getenv ("EDITOR")) == NULL) >> + editor = "/bin/ex"; > > This is likewise non-portable: "/bin/ex" is only guaranteed to exist > on Posix platforms. > In the next patch I will adress all those issues (The fixed lenght buffers are ugly but you can never overflow them, the mkstemp template and the snprintf don't allow for it, but then some bad truncation and unpredictable execution may happen). gcc will complain about tmpnamp() like this: test.c:(.text+0x13): warning: the use of `tempnam' is dangerous, better use `mkstemp' The man page of FreeBSD explains it much better than the Debian one, now I know it's because a possible race condition. About the "/bin/ex" issue, to maintain consistency I did it the same way that the EDIT command, here: gdb/cli/cli-cmds.c:695 if ((editor = (char *) getenv ("EDITOR")) == NULL) editor = "/bin/ex"; BTW "/bin/ex" exists on solaris 9, but not Debian or *BSD (is /usr/bin/ex there), as a result text editing won't work by default on most systems. It would be difficult to choose a portable editor...maybe with some #defines