Re: RFC: remote.c incoming packet cleanups

[Eli Zaretskii <eliz at gnu dot org>]

> Date: Thu, 23 Feb 2006 09:56:21 -0500
> From: Daniel Jacobowitz <drow@false.org>
> 
> The basic change is to convert getpkt from (char *buf, int len,
> int forever) to (char **buf_p, int *len, int forever) and allocate
> *buf_p with xmalloc, so that we can pass it to xrealloc.  All
> the rest is cascading changes.

This is fine with me, but you wrote this code in get_memory_packet_size:

> @@ -436,6 +455,15 @@ get_memory_packet_size (struct memory_pa
>      what_they_get = MAX_REMOTE_PACKET_SIZE;
>    if (what_they_get < MIN_REMOTE_PACKET_SIZE)
>      what_they_get = MIN_REMOTE_PACKET_SIZE;
> +
> +  /* Make sure there is room in the global buffer for this packet
> +     (including its trailing NUL byte).  */
> +  if (rs->buf_size < what_they_get + 1)
> +    {
> +      rs->buf_size = what_they_get + 1;
> +      rs->buf = xrealloc (rs->buf, what_they_get + 1);
> +    }
> +
>    return what_they_get;
>  }

I wonder whether it would be better to expand by multiplying the
previous size by 2, instead of just making enough room for the current
packet.  The latter could cause bad memory fragmentation under some
pathological patterns of packet size sequences.  (And in read_frame you
indeed multiply by two when you realloc the buffer.)

> I proofread as best I could that there are no overlapping uses
> of the global buffer (there was exactly one overlap, which is
> one of the alloca uses I left).  I also tested using gdbserver
> and the gdb testsuite, and by hand receiving an oversized (20K)
> packet.

Would it make sense to add gdb_assert in strategic places to make sure
any overlapping uses will be caught red-handed?

> It all looks good, but I'd appreciate both comments on the approach and
> additional pairs of eyes looking over the changes - it was very
> mechanical, but not quite enough so to automate.
> 
> Look OK?

Looks okay to me, thanks.