This is the mail archive of the
gdb-patches@sources.redhat.com
mailing list for the GDB project.
Re: [RFC/Patch] replace sprintf calls in remote.c with xsnprintf
- From: Daniel Jacobowitz <drow at false dot org>
- To: Wu Zhou <woodzltc at cn dot ibm dot com>
- Cc: gdb-patches at sources dot redhat dot com, eliz at gnu dot org
- Date: Sat, 28 May 2005 14:39:25 -0400
- Subject: Re: [RFC/Patch] replace sprintf calls in remote.c with xsnprintf
- References: <Pine.LNX.4.63.0505260626580.22654@plinuxt18.cn.ibm.com>
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 <woodzltc@cn.ibm.com>
>
> * 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