Re: [PATCH v2 2/2] gdbstub: do not split gdb_monitor_write payload

["Alex Bennée" <alex.bennee@linaro.org>]

Damien Hedde <damien.hedde@greensocs.com> writes:

> On 12/11/19 7:59 PM, Alex Bennée wrote:
>> 
>> Damien Hedde <damien.hedde@greensocs.com> writes:
>> 
>>> Since we can now send packets of arbitrary length:
>>> simplify gdb_monitor_write() and send the whole payload
>>> in one packet.
>> 
>> Do we know gdb won't barf on us. Does the negotiated max packet size
>> only apply to data sent to the gdbserver?
>
> Yes the negociated packet size is only about packet we can receive.
> Qutoting the gdb doc:
> | ‘PacketSize=bytes’
> |
> |    The remote stub can accept packets up to at least bytes in length.
> | GDB will send packets up to this size for bulk transfers, and will
> | never send larger packets.
>
> The qSupported doc also says that "Any GDB which sends a ‘qSupported’
> packet supports receiving packets of unlimited length".
> I did some digging and qSupported appeared in gdb 6.6 (december 2006).
> And gdb supported arbitrary sized packet even before that (6.4.9 2006
> too).

I think that is worth a comment for the function gdb_monitor_write
quoting the spec and the versions. With that comment:

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

>
>> 
>>>
>>> Suggested-by: Luc Michel <luc.michel@greensocs.com>
>>> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
>>> ---
>>>  gdbstub.c | 23 +++--------------------
>>>  1 file changed, 3 insertions(+), 20 deletions(-)
>>>
>>> diff --git a/gdbstub.c b/gdbstub.c
>>> index 93b26f1b86..ef999abee2 100644
>>> --- a/gdbstub.c
>>> +++ b/gdbstub.c
>>> @@ -3200,28 +3200,11 @@ static void gdb_chr_event(void *opaque, int event)
>>>      }
>>>  }
>>>  
>>> -static void gdb_monitor_output(GDBState *s, const char *msg, int len)
>>> -{
>>> -    g_autoptr(GString) buf = g_string_new("O");
>>> -    memtohex(buf, (uint8_t *)msg, len);
>>> -    put_packet(buf->str);
>>> -}
>>> -
>>>  static int gdb_monitor_write(Chardev *chr, const uint8_t *buf, int len)
>>>  {
>>> -    const char *p = (const char *)buf;
>>> -    int max_sz;
>>> -
>>> -    max_sz = (MAX_PACKET_LENGTH / 2) + 1;
>>> -    for (;;) {
>>> -        if (len <= max_sz) {
>>> -            gdb_monitor_output(&gdbserver_state, p, len);
>>> -            break;
>>> -        }
>>> -        gdb_monitor_output(&gdbserver_state, p, max_sz);
>>> -        p += max_sz;
>>> -        len -= max_sz;
>>> -    }
>>> +    g_autoptr(GString) hex_buf = g_string_new("O");
>>> +    memtohex(hex_buf, buf, len);
>>> +    put_packet(hex_buf->str);
>>>      return len;
>>>  }
>> 
>> 


-- 
Alex Bennée