From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 4367 invoked by alias); 28 May 2005 18:39:43 -0000 Mailing-List: contact gdb-patches-help@sources.redhat.com; run by ezmlm Precedence: bulk List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sources.redhat.com Received: (qmail 4343 invoked by uid 22791); 28 May 2005 18:39:38 -0000 Received: from nevyn.them.org (HELO nevyn.them.org) (66.93.172.17) by sourceware.org (qpsmtpd/0.30-dev) with ESMTP; Sat, 28 May 2005 18:39:38 +0000 Received: from drow by nevyn.them.org with local (Exim 4.50) id 1Dc6DW-00051u-Bp; Sat, 28 May 2005 14:39:26 -0400 Date: Sat, 28 May 2005 18:53:00 -0000 From: Daniel Jacobowitz To: Wu Zhou Cc: gdb-patches@sources.redhat.com, eliz@gnu.org Subject: Re: [RFC/Patch] replace sprintf calls in remote.c with xsnprintf Message-ID: <20050528183924.GE26806@nevyn.them.org> Mail-Followup-To: Wu Zhou , gdb-patches@sources.redhat.com, eliz@gnu.org References: Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.8i X-SW-Source: 2005-05/txt/msg00585.txt.bz2 On Thu, May 26, 2005 at 07:00:16AM -0700, Wu Zhou wrote: > Hello all, > > I coded a patch to replace sprintf calls (mainly these error-prone > and easy-to-fix ones) in remote.c with xsnprintf. The fixs for two > typos are also included. Had tested it on ppc64. No regression. > > Please review and comment. Thanks. Thanks for doing this. It needs a couple of small changes. > <2005-05-26> Wu Zhou > > * remote.c: Fix two comment typos. Please do this as a separate patch. > #define MAGIC_NULL_PID 42000 > + #define REMOTE_PACKET_SIZE (rs->remote_packet_size) Please don't do this. Macros which reference local variables are very bad style, and confusing. > *************** remote_threads_extra_info (struct thread > *** 1859,1865 **** > int set; > threadref id; > struct gdb_ext_thread_info threadinfo; > ! static char display_buf[100]; /* arbitrary... */ > char *bufp = alloca (rs->remote_packet_size); > int n = 0; /* position in display_buf */ > > --- 1860,1868 ---- > int set; > threadref id; > struct gdb_ext_thread_info threadinfo; > ! /* change the buffer size to 600 to hold shortname, dispaly > ! and more_display of threadinfo. */ > ! static char display_buf[600]; > char *bufp = alloca (rs->remote_packet_size); > int n = 0; /* position in display_buf */ > Comments are full sentences (and you spelled display wrong). You didn't mention in the changelog that you changed the size of the buffer; I don't think it's necessary. Remember that remote_packet_size is target-dependent. If 100 has been good enough so far, it probably still is (though using xsnprintf is still a good thing). > --- 1927,1933 ---- > /* Send the restart command; for reasons I don't understand the > remote side really expects a number after the "R". */ > buf[0] = 'R'; > ! xsnprintf (&buf[1], REMOTE_PACKET_SIZE - 1, "%x", 0); > putpkt (buf); > > /* Now query for status so this looks just like we restarted Might as well use "R%x" while you're here. -- Daniel Jacobowitz CodeSourcery, LLC