Re: [Qemu-devel] [PATCH 2/5] gdbstub: gdb pass-through qemu monitor support

["Edgar E. Iglesias" <edgar.iglesias@axis.com>]

On Thu, May 15, 2008 at 09:11:30AM -0500, Jason Wessel wrote:
> This patch adds a feature to the gdbstub to allow gdb to issue monitor
> commands that can pass-through to the qemu monitor.
> 
> In order to make this work, the MAX_MON (the maximum number of monitor
> connections) had to get incremented by 1 to support the case when the
> gdbstub is setup along with all the other connections.  A small check
> was added avoid strange crashes when you allocate too many qemu
> monitor connections.
> 
> The monitor_handle_command had to be exported in order to allow
> gdbstub to pass the strings directly to the monitor.  The gdbstub
> registers as an output device of the qemu monitor such that no further
> changes were needed to the monitor other than to have a global
> variable in the gdbstub that controls when to transmit data from a
> pass through monitor command to an attached debugger.
> 
> The qemu docs were updated to reflect this change.
> 
> Signed-off-by: Jason Wessel <jason.wessel@windriver.com>


This sounds like a neat feature to me. AFAICS the patch looks mostly ok, just a few nitpicks.


> ---
>  console.h     |    1 +
>  gdbstub.c     |   47 +++++++++++++++++++++++++++++++++++++++++++----
>  monitor.c     |    8 ++++++--
>  qemu-doc.texi |   11 +++++++++++
>  4 files changed, 61 insertions(+), 6 deletions(-)
> 
> diff --git a/console.h b/console.h
> index c7f29f5..041b4ef 100644
> --- a/console.h
> +++ b/console.h
> @@ -159,6 +159,7 @@ extern uint8_t _translate_keycode(const int key);
>     does not need to include console.h  */
>  /* monitor.c */
>  void monitor_init(CharDriverState *hd, int show_banner);
> +void monitor_handle_command(const char *cmdline);
>  void term_puts(const char *str);
>  void term_vprintf(const char *fmt, va_list ap);
>  void term_printf(const char *fmt, ...) __attribute__ ((__format__ (__printf__, 1, 2)));
> diff --git a/gdbstub.c b/gdbstub.c
> index 4bc7999..833cdd9 100644
> --- a/gdbstub.c
> +++ b/gdbstub.c
> @@ -33,6 +33,7 @@
>  #include "qemu-char.h"
>  #include "sysemu.h"
>  #include "gdbstub.h"
> +#include "console.h"
>  #endif
>  
>  #include "qemu_socket.h"
> @@ -70,6 +71,8 @@ typedef struct GDBState {
>      int running_state;
>  #else
>      CharDriverState *chr;
> +    CharDriverState *mon;
> +    int allow_monitor;
>  #endif
>  } GDBState;
>  
> @@ -239,10 +242,11 @@ static int put_packet(GDBState *s, char *buf)
>      return 0;
>  }
>  
> -static void monitor_output(GDBState *s, const char *msg)
> +static void monitor_output_len(GDBState *s, const char *msg, int len)
>  {
>      char *buf;
> -    int len = strlen(msg);
> +    if (!len)
> +        len = strlen(msg);
>  
>      buf = malloc(len * 2 + 2);
>      buf[0] = 'O';
> @@ -251,6 +255,11 @@ static void monitor_output(GDBState *s, const char *msg)
>      free(buf);
>  }
>  
> +static void monitor_output(GDBState *s, const char *msg)
> +{
> +    monitor_output_len(s, msg, 0);
> +}
> +


If you change the zero argument into a strlen(msg) you can drop the check in monitor_output_len().


>  static void monitor_help(GDBState *s)
>  {
>      monitor_output(s, "gdbstub specific monitor commands:\n");
> @@ -258,6 +267,7 @@ static void monitor_help(GDBState *s)
>      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");
> +    monitor_output(s, "qemu monitor pass-through commands:\n");
>  }

You might want to consider not showing that last line for CONFIG_USER_ONLY.

Best regards
-- 
Edgar E. Iglesias
Axis Communications AB