Re: [RFC/Patch] replace sprintf calls in remote.c with xsnprintf

[Daniel Jacobowitz <drow@false.org>]

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