qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH 1/5] gdbstub: replace singlestep q packets with


From: Edgar E. Iglesias
Subject: Re: [Qemu-devel] [PATCH 1/5] gdbstub: replace singlestep q packets with qRcmd packets
Date: Thu, 15 May 2008 23:33:48 +0200
User-agent: Mutt/1.5.16 (2007-06-09)

On Thu, May 15, 2008 at 09:11:29AM -0500, Jason Wessel wrote:
> At the gdbserial protocol level, using the qRcmd packets allows gdb to
> use the "monitor" command to access the controls for single stepping
> behavior.  Now you can use a gdb "monitor" command instead of a gdb
> "maintenance" command.
> 
> The qemu docs were updated to reflect this change.
> 
> Signed-off-by: Jason Wessel <address@hidden>

Thanks Jason, a few comments inlined.

> ---
>  gdbstub.c     |   82 
> +++++++++++++++++++++++++++++++++++++++++----------------
>  qemu-doc.texi |   34 +++++++++++------------
>  2 files changed, 75 insertions(+), 41 deletions(-)
> 
> diff --git a/gdbstub.c b/gdbstub.c
> index 9a361e3..4bc7999 100644
> --- a/gdbstub.c
> +++ b/gdbstub.c
> @@ -239,6 +239,27 @@ static int put_packet(GDBState *s, char *buf)
>      return 0;
>  }
>  
> +static void monitor_output(GDBState *s, const char *msg)
> +{
> +    char *buf;
> +    int len = strlen(msg);
> +
> +    buf = malloc(len * 2 + 2);
> +    buf[0] = 'O';
> +    memtohex(buf + 1, msg, len);
> +    put_packet(s, buf);
> +    free(buf);
> +}


It feels odd that the code path that ends up here has line_buf, buf and mem_buf 
available for parsing the query and generating the response and still we need 
to malloc for more. Isn't there a way to reuse some of that memory?


> +
> +static void monitor_help(GDBState *s)
> +{
> +    monitor_output(s, "gdbstub specific monitor commands:\n");
> +    monitor_output(s, "s_show -- Show gdbstub Single Stepping variables\n");
> +    monitor_output(s, "set s_step <0|1> -- Single Stepping enabled\n");
> +    monitor_output(s, "set s_irq <0|1> --  Single Stepping with qemu irq 
> handlers enabled\n");
> +    monitor_output(s, "set s_timer <0|1> -- Single Stepping with qemu timers 
> enabled\n");
> +}

I'd prefer if either have a show/set interface or we don't, this is kind of a 
mix.

Some suggestions:
monitor sstep enable/disable
monitor sstep irq/noirq
monitor sstep timers/notimers
monitor sstep show

or:
monitor set/show sstep
monitor set/show sirq
monitor set/show stimers

What do you think?

Best regards
-- 
Edgar E. Iglesias
Axis Communications AB




reply via email to

[Prev in Thread] Current Thread [Next in Thread]