[Qemu-devel] Re: [PATCH] Gdbstub user mode -gdb dev option : add unix sockets

[Jan Kiszka <jan.kiszka@siemens.com>]

Philippe Waille wrote:
>   This patch removes the -g portnumber option in user mode.
>   Adds a -gdb device option as in the system mode.
> 
>   -gdb tcp:host:port              or   -gdb tcp::port
>   -gdb unix:socketname[,unlink]
> 
>   Missing tests :
>   + system mode regression test (I don't have a "hello world" example
> for system mode)
>   + _WIN32 mode
> 
> 
> Philippe Waille
> -------------------------------------------------------------------------
> 
> Index: linux-user/main.c
> ===================================================================
> --- linux-user/main.c	(révision 7105)
> +++ linux-user/main.c	(copie de travail)
> @@ -2202,40 +2202,37 @@
>  
>  static void usage(void)
>  {
> -    printf("qemu-" TARGET_ARCH " version " QEMU_VERSION QEMU_PKGVERSION ", Copyright (c) 2003-2008 Fabrice Bellard\n"
> -           "usage: qemu-" TARGET_ARCH " [options] program [arguments...]\n"
> -           "Linux CPU emulator (compiled for %s emulation)\n"
> -           "\n"
> -           "Standard options:\n"
> -           "-h                print this help\n"
> -           "-g port           wait gdb connection to port\n"
> -           "-L path           set the elf interpreter prefix (default=%s)\n"
> -           "-s size           set the stack size in bytes (default=%ld)\n"
> -           "-cpu model        select CPU (-cpu ? for list)\n"
> -           "-drop-ld-preload  drop LD_PRELOAD for target process\n"
> -           "-E var=value      sets/modifies targets environment variable(s)\n"
> -           "-U var            unsets targets environment variable(s)\n"
> -           "\n"
> -           "Debug options:\n"
> -           "-d options   activate log (logfile=%s)\n"
> -           "-p pagesize  set the host page size to 'pagesize'\n"
> -           "-singlestep  always run in singlestep mode\n"
> -           "-strace      log system calls\n"
> -           "\n"
> -           "Environment variables:\n"
> -           "QEMU_STRACE       Print system calls and arguments similar to the\n"
> -           "                  'strace' program.  Enable by setting to any value.\n"
> -           "You can use -E and -U options to set/unset environment variables\n"
> -           "for target process.  It is possible to provide several variables\n"
> -           "by repeating the option.  For example:\n"
> -           "    -E var1=val2 -E var2=val2 -U LD_PRELOAD -U LD_DEBUG\n"
> -           "Note that if you provide several changes to single variable\n"
> -           "last change will stay in effect.\n"
> -           ,
> -           TARGET_ARCH,
> -           interp_prefix,
> -           x86_stack_size,
> -           DEBUG_LOGFILE);
> +    printf("qemu-" TARGET_ARCH " version " QEMU_VERSION ", Copyright (c) 2003-2008 Fabrice Bellard\n");
> +    printf("usage: qemu-" TARGET_ARCH " [options] program [arguments...]\n");
> +    printf("Linux CPU emulator (compiled for %s emulation)\n",
> +           TARGET_ARCH);
> +    printf("\n");
> +    printf("Standard options:\n");
> +    printf("-h                print this help\n");
> +    gdbstub_usage();
> +    printf("-L path           set the elf interpreter prefix (default=%s)\n",
> +           interp_prefix);
> +    printf("-s size           set the stack size in bytes (default=%ld)\n",
> +           x86_stack_size);
> +    printf("-cpu model        select CPU (-cpu ? for list)\n");
> +    printf("-drop-ld-preload  drop LD_PRELOAD for target process\n");
> +    printf("-E var=value      sets/modifies targets environment variable(s)\n");
> +    printf("-U var            unsets targets environment variable(s)\n");
> +    printf("\n");
> +    printf("Debug options:\n");
> +    printf("-d options   activate log (logfile=%s)\n", DEBUG_LOGFILE);
> +    printf("-p pagesize  set the host page size to 'pagesize'\n");
> +    printf("-strace      log system calls\n");
> +    printf("\n");
> +    printf("Environment variables:\n");
> +    printf("QEMU_STRACE       Print system calls and arguments similar to the\n");
> +    printf("                  'strace' program.  Enable by setting to any value.\n");
> +    printf("You can use -E and -U options to set/unset environment variables\n");
> +    printf("for target process.  It is possible to provide several variables\n");
> +    printf("by repeating the option.  For example:\n");
> +    printf("    -E var1=val2 -E var2=val2 -U LD_PRELOAD -U LD_DEBUG\n");
> +    printf("Note that if you provide several changes to single variable\n");
> +    printf("last change will stay in effect.\n");

Please drop this conversion single -> multiple printf, just change the
string related to this patch. Also, I think factoring out
gdbstub_usage() is a bit overkill.

>      exit(1);
>  }
>  
> @@ -2264,7 +2261,6 @@
>      CPUState *env;
>      int optind;
>      const char *r;
> -    int gdbstub_port = 0;
>      char **target_environ, **wrk;
>      envlist_t *envlist = NULL;
>  
> @@ -2345,10 +2341,15 @@
>                  fprintf(stderr, "page size must be a power of two\n");
>                  exit(1);
>              }
> -        } else if (!strcmp(r, "g")) {
> -            if (optind >= argc)
> +        } else if (!strcmp(r, "gdb")) {
> +            if (optind >= argc) {
>                  break;
> -            gdbstub_port = atoi(argv[optind++]);
> +            }
> +            if (gdbserver_start(argv[optind++]) < 0) {
> +                /* stop command line analysis */
> +                optind = argc;
> +                break;

Shouldn't you rather fail with some error message and exit()?

> +            }
>  	} else if (!strcmp(r, "r")) {
>  	    qemu_uname_release = argv[optind++];
>          } else if (!strcmp(r, "cpu")) {
> @@ -2706,8 +2707,10 @@
>      ts->heap_limit = 0;
>  #endif
>  
> -    if (gdbstub_port) {
> -        gdbserver_start (gdbstub_port);
> +    if (gdbstub_accept != NULL) {
> +        if ((*gdbstub_accept)() < 0) {
> +            return -1;
> +        }

Rather introduce gdbstub_accept() and keep the function point private
(static) to gdbstub.c. gdbstub_accept() could then also check for the
pointer being non-NULL and do the inital handlesig.

>          gdb_handlesig(env, 0);
>      }
>      cpu_loop(env);
> Index: gdbstub.c
> ===================================================================
> --- gdbstub.c	(révision 7105)
> +++ gdbstub.c	(copie de travail)
> @@ -19,23 +19,19 @@
>   */
>  #include "config.h"
>  #include "qemu-common.h"
> +
>  #ifdef CONFIG_USER_ONLY
> -#include <stdlib.h>
> -#include <stdio.h>
> -#include <stdarg.h>
> -#include <string.h>
> -#include <errno.h>
> -#include <unistd.h>
> -#include <fcntl.h>
> +#include "qemu.h"
> +#include "gdbstub.h"
>  
> -#include "qemu.h"
> -#else
> +#else /* CONFIG_USER_ONLY */

Minor remark: I prefer comment on this with "!CONFIG_USER_ONLY" as the
following code handles the !defined case.

>  #include "monitor.h"
>  #include "qemu-char.h"
>  #include "sysemu.h"
>  #include "gdbstub.h"
> -#endif
> +#endif /* CONFIG_USER_ONLY */

same here

>  
> +
>  #define MAX_PACKET_LENGTH 4096
>  
>  #include "qemu_socket.h"
> @@ -215,7 +211,7 @@
>      -1
>  #endif
>  };
> -#else
> +#else  /* CONFIG_USER_ONLY */

and here

>  /* In system mode we only need SIGINT and SIGTRAP; other signals
>     are not yet supported.  */
>  
> @@ -232,7 +228,7 @@
>      -1,
>      TARGET_SIGTRAP
>  };
> -#endif
> +#endif  /* CONFIG_USER_ONLY */

and here.

>  
>  #ifdef CONFIG_USER_ONLY
>  static int target_signal_to_gdb (int sig)
> @@ -243,7 +239,7 @@
>              return i;
>      return GDB_SIGNAL_UNKNOWN;
>  }
> -#endif
> +#endif  /* CONFIG_USER_ONLY */
>  
>  static int gdb_signal_to_target (int sig)
>  {
> @@ -307,31 +303,13 @@
>  #ifdef CONFIG_USER_ONLY
>  /* XXX: This is not thread safe.  Do we care?  */
>  static int gdbserver_fd = -1;
> +gdbstub_func gdbstub_accept = NULL;
>  
> -static int get_char(GDBState *s)
> -{
> -    uint8_t ch;
> -    int ret;
> +static int gdb_disconnected (GDBState *);
> +static int get_char(GDBState *s);
> +#endif /* CONFIG_USER_ONLY */
>  
> -    for(;;) {
> -        ret = recv(s->fd, &ch, 1, 0);
> -        if (ret < 0) {
> -            if (errno == ECONNRESET)
> -                s->fd = -1;
> -            if (errno != EINTR && errno != EAGAIN)
> -                return -1;
> -        } else if (ret == 0) {
> -            close(s->fd);
> -            s->fd = -1;
> -            return -1;
> -        } else {
> -            break;
> -        }
> -    }
> -    return ch;
> -}
> -#endif
> -
> +static void put_buffer(GDBState *s, const uint8_t *buf, int len);
>  static gdb_syscall_complete_cb gdb_current_syscall_cb;
>  
>  enum {
> @@ -361,26 +339,6 @@
>  #endif
>  }
>  
> -static void put_buffer(GDBState *s, const uint8_t *buf, int len)
> -{
> -#ifdef CONFIG_USER_ONLY
> -    int ret;
> -
> -    while (len > 0) {
> -        ret = send(s->fd, buf, len, 0);
> -        if (ret < 0) {
> -            if (errno != EINTR && errno != EAGAIN)
> -                return;
> -        } else {
> -            buf += ret;
> -            len -= ret;
> -        }
> -    }
> -#else
> -    qemu_chr_write(s->chr, buf, len);
> -#endif
> -}
> -
>  static inline int fromhex(int v)
>  {
>      if (v >= '0' && v <= '9')
> @@ -1280,7 +1238,7 @@
>      return 0;
>  }
>  
> -#endif
> +#endif /* TARGET */

That comment is misleading. Just leave it alone for this patch.

>  
>  static int num_g_regs = NUM_CORE_REGS;
>  
> @@ -2091,8 +2049,7 @@
>  }
>  
>  #ifdef CONFIG_USER_ONLY
> -int
> -gdb_queuesig (void)
> +int gdb_queuesig (void)

If you are at it already: no space between function name and parameter
list. Applies to many other functions in this patch, too. Please adjust.

>  {
>      GDBState *s;
>  
> @@ -2104,16 +2061,41 @@
>          return 1;
>  }
>  
> -int
> -gdb_handlesig (CPUState *env, int sig)
> +/* Tell the remote gdb that the process has exited.  */
> +void gdb_exit(CPUState *env, int code)
>  {
>    GDBState *s;
> +  char buf[4];
> +
> +  s = gdbserver_state;
> +  if (gdbserver_fd < 0 || s->fd < 0)
> +    return;
> +
> +  snprintf(buf, sizeof(buf), "W%02x", code);
> +  put_packet(s, buf);
> +}
> +
> +void gdb_signalled(CPUState *env, int sig)
> +{
> +  GDBState *s;
> +  char buf[4];
> +
> +  s = gdbserver_state;
> +  if (gdbserver_fd < 0 || s->fd < 0)
> +    return;
> +
> +  snprintf(buf, sizeof(buf), "X%02x", target_signal_to_gdb (sig));
> +  put_packet(s, buf);
> +}
> +
> +int gdb_handlesig (CPUState *env, int sig)
> +{
> +  GDBState *s;
>    char buf[256];
>    int n;
>  
>    s = gdbserver_state;
> -  if (gdbserver_fd < 0 || s->fd < 0)
> -    return sig;
> +  if (gdb_disconnected (s)) return sig;

Separate line (with brackets), please.

>  
>    /* disable single step if it was enabled */
>    cpu_single_step(env, 0);
> @@ -2126,9 +2108,9 @@
>      }
>    /* put_packet() might have detected that the peer terminated the 
>       connection.  */
> -  if (s->fd < 0)
> -      return sig;
>  
> +  if (gdb_disconnected (s)) return sig;
> +

Same here.

>    sig = 0;
>    s->state = RS_IDLE;
>    s->running_state = 0;
> @@ -2138,10 +2120,11 @@
>          {
>            int i;
>  
> -          for (i = 0; i < n; i++)
> +          for (i = 0; i < n; i++) {
>              gdb_read_byte (s, buf[i]);
> +          }
>          }
> -      else if (n == 0 || errno != EAGAIN)
> +      else if (n == 0 || errno != EAGAIN) 

Trailing whitespace.

>          {
>            /* XXX: Connection closed.  Should probably wait for annother
>               connection before continuing.  */
> @@ -2153,108 +2136,294 @@
>    return sig;
>  }
>  
> -/* Tell the remote gdb that the process has exited.  */
> -void gdb_exit(CPUState *env, int code)
> +static int get_char(GDBState *s)
>  {
> -  GDBState *s;
> -  char buf[4];
> +    uint8_t ch;
> +    int ret;
>  
> -  s = gdbserver_state;
> -  if (gdbserver_fd < 0 || s->fd < 0)
> -    return;
> +    for(;;) {
> +        ret = recv(s->fd, &ch, 1, 0);
> +        if (ret < 0) {
> +            if (errno == ECONNRESET)
> +                s->fd = -1;
> +            if (errno != EINTR && errno != EAGAIN)
> +                return -1;
> +        } else if (ret == 0) {
> +            close(s->fd);
> +            s->fd = -1;
> +            return -1;
> +        } else {
> +            break;
> +        }
> +    }
> +    return ch;
> +}
>  
> -  snprintf(buf, sizeof(buf), "W%02x", code);
> -  put_packet(s, buf);
> +
> +static void put_buffer(GDBState *s, const uint8_t *buf, int len)
> +{
> +    int ret;
> +    while (len > 0) {
> +        ret = send(s->fd, buf, len, 0);
> +        if (ret < 0) {
> +            if (errno != EINTR && errno != EAGAIN)
> +                return;
> +        } else {
> +            buf += ret;
> +            len -= ret;
> +        }
> +    }
>  }
>  
> -/* Tell the remote gdb that the process has exited due to SIG.  */
> -void gdb_signalled(CPUState *env, int sig)
> +static int gdb_disconnected (GDBState *s) 
>  {
> -  GDBState *s;
> -  char buf[4];
> +    return  (gdbserver_fd < 0) || (s->fd < 0);
> +}
>  
> -  s = gdbserver_state;
> -  if (gdbserver_fd < 0 || s->fd < 0)
> -    return;
> +static GDBState *gdbserver_create_gdbstate (void)
> +{
> +    GDBState *s;
>  
> -  snprintf(buf, sizeof(buf), "X%02x", target_signal_to_gdb (sig));
> -  put_packet(s, buf);
> +    s = qemu_mallocz(sizeof(GDBState));
> +    s->c_cpu = first_cpu;
> +    s->g_cpu = first_cpu;
> +    gdb_has_xml = 0;
> +    gdbserver_state = s;
> +    return s;
>  }
>  
> -static void gdb_accept(void)
> +static int gdbserver_tcp_accept (void)
>  {
> +    int fd,res;
>      GDBState *s;
> -    struct sockaddr_in sockaddr;
> -    socklen_t len;
> -    int val, fd;
>  
> -    for(;;) {
> -        len = sizeof(sockaddr);
> -        fd = accept(gdbserver_fd, (struct sockaddr *)&sockaddr, &len);
> -        if (fd < 0 && errno != EINTR) {
> -            perror("accept");
> -            return;
> -        } else if (fd >= 0) {
> -            break;
> +    for (;;) {
> +        fd = accept (gdbserver_fd, 
> +                     (struct sockaddr *) NULL, (socklen_t *) NULL);
> +        if (fd < 0) {
> +           if (errno != EINTR) {
> +              perror("accept");
> +              return -1;
> +           }
> +        } else {
> +           break;
>          }
>      }
> -
>      /* set short latency */
> -    val = 1;
> -    setsockopt(fd, IPPROTO_TCP, TCP_NODELAY, (char *)&val, sizeof(val));
> +    res = 1;
> +    res = setsockopt(fd, IPPROTO_TCP, TCP_NODELAY, (char *)&res, sizeof (res));
> +    if (res < 0) {
> +       fprintf (stderr, "gdbserver ERROR : set nodelay\n");
> +       return -1;
> +    } 
> +    res = fcntl(fd, F_SETFL, O_NONBLOCK);
> +    if (res < 0) {
> +       fprintf (stderr, "gdbserver ERROR : set nonblock\n");
> +       return -1;
> +    }
> +    s = gdbserver_create_gdbstate ();
> +    s -> fd = fd;
> +    return 0;
> +}
>  
> -    s = qemu_mallocz(sizeof(GDBState));
> -    s->c_cpu = first_cpu;
> -    s->g_cpu = first_cpu;
> -    s->fd = fd;
> -    gdb_has_xml = 0;
>  
> -    gdbserver_state = s;
> +static int gdbserver_tcp_start (const char *host, const char *port,
> +                                gdbstub_func faccept)
> +{
> +    struct addrinfo info_in, *info_out;
> +    struct sockaddr_in addrin, *addr;
> +    int res;
>  
> -    fcntl(fd, F_SETFL, O_NONBLOCK);
> +    memset (&info_in, 0, sizeof(struct addrinfo));
> +    info_in.ai_flags=AI_PASSIVE;  /* force INADDR_ANY if host == NULL */
> +    info_in.ai_family= AF_INET;
> +    info_in.ai_socktype=SOCK_STREAM;
> +    info_in.ai_protocol=IPPROTO_TCP;
> +
> +    if (getaddrinfo(host, port, &info_in, &info_out) < 0) {
> +        perror ("hostname/portname name conversion error");
> +        return -1;
> +    }
> +    addr = (struct sockaddr_in *) info_out -> ai_addr;
> +    gdbserver_fd = socket (info_in.ai_family, info_in.ai_socktype,
> +                           info_in.ai_protocol);
> +    if (info_in.ai_protocol < 0) {
> +        perror ("gdb socket");
> +        return -1;
> +    }
> +    /* allow fast reuse */ 
> +    res = 1;
> +    res = setsockopt(gdbserver_fd, SOL_SOCKET, SO_REUSEADDR,
> +                     (char *) &res, sizeof(res));
> +    if (res <0) {
> +        perror("gdb socket : set  fast reuse\n");
> +        return -1;
> +    }
> +    res = bind (gdbserver_fd, (struct sockaddr *) addr, sizeof (addrin));
> +    if (res <0) {
> +        perror("gdb socket bind\n");
> +        return -1;
> +    }
> +    res = listen (gdbserver_fd,0);
> +    if (res <0) {
> +        perror("listen\n");
> +        return -1;
> +    }
> +    gdbstub_accept = faccept;
> +    return 0;
> +} 
> +
> +#ifndef     _WIN32
> +static int gdbserver_unix_unlink (void)
> +{
> +    struct sockaddr_un sock;
> +    socklen_t len;
> +    int res;
> +
> +    len = sizeof (sock.sun_path);
> +    res = getsockname (gdbserver_fd, &sock, &len);
> +    if (res < 0 ) {
> +       fprintf (stderr,"gdb accept/unlink : cannot get socket name\n");
> +       return -1;
> +    }
> +    res = unlink (sock.sun_path);
> +    if (res < 0) {
> +       fprintf (stderr,"gdb accept/unlink : cannot unlink %s\n",sock.sun_path);
> +       return -1;
> +    } 
> +    return 0;
>  }
>  
> -static int gdbserver_open(int port)
> +static int gdbserver_unix_accept (void)
>  {
> -    struct sockaddr_in sockaddr;
> -    int fd, val, ret;
> +    int fd,res;
> +    GDBState *s;
> +    for (;;) {
> +        fd = accept (gdbserver_fd, 
> +                     (struct sockaddr *) NULL, ( socklen_t *) NULL);
> +        if (fd < 0) {
> +           if (errno != EINTR) {
> +              perror("accept");
> +              return -1;
> +           }
> +        } else {
> +           break;
> +        }
> +    }
>  
> -    fd = socket(PF_INET, SOCK_STREAM, 0);
> -    if (fd < 0) {
> -        perror("socket");
> -        return -1;
> +    res = fcntl(fd, F_SETFL, O_NONBLOCK);
> +    if (res < 0) {
> +       fprintf (stderr, "gdbserver ERROR : set nonblock\n");
> +       return -1;
>      }
> +    s = gdbserver_create_gdbstate ();
> +    s -> fd = fd;
> +    return 0;
> +}
>  
> -    /* allow fast reuse */
> -    val = 1;
> -    setsockopt(fd, SOL_SOCKET, SO_REUSEADDR, (char *)&val, sizeof(val));
> +static int gdbserver_unix_accept_unlink (void)
> +{
> +    return (gdbserver_unix_accept () + gdbserver_unix_unlink ());
> +}
>  
> -    sockaddr.sin_family = AF_INET;
> -    sockaddr.sin_port = htons(port);
> -    sockaddr.sin_addr.s_addr = 0;
> -    ret = bind(fd, (struct sockaddr *)&sockaddr, sizeof(sockaddr));
> -    if (ret < 0) {
> -        perror("bind");
> +static int gdbserver_unix_start (const char *filename, 
> +                                 gdbstub_func faccept)
> +{
> +    struct sockaddr_un addr;
> +    int res;
> +
> +    if (strlen(filename) == 0) {
> +       fprintf (stderr, "gdbserver ERROR : missing unix socket name\n");
> +       return -1;
> +    }
> +
> +    memset (&addr, 0, sizeof(struct sockaddr_un));
> +    addr.sun_family = AF_UNIX;
> +    strcpy (addr.sun_path, filename);
> +
> +    gdbserver_fd = socket (AF_UNIX,SOCK_STREAM,0);
> +    if (gdbserver_fd < 0) {
> +        perror ("gsbserver socket");
>          return -1;
>      }
> -    ret = listen(fd, 0);
> -    if (ret < 0) {
> -        perror("listen");
> +    res = bind (gdbserver_fd, (struct sockaddr *) &addr, 
> +                sizeof(struct sockaddr_un));
> +    if (res < 0) {
> +        perror ("gdbserver bind\n");
>          return -1;
>      }
> -    return fd;
> -}
> +    res = listen (gdbserver_fd,0);
> +    if (res <0) {
> +        perror("listen\n");
> +        return -1;
> +    }
> +    gdbstub_accept = faccept;
> +    return 0;
> +} 
>  
> -int gdbserver_start(int port)
> +#endif      /* _WIN32 */    
> +
> +void gdbstub_usage (void) 
>  {
> -    gdbserver_fd = gdbserver_open(port);
> -    if (gdbserver_fd < 0)
> -        return -1;
> -    /* accept connections */
> -    gdb_accept();
> +     printf("-gdb device       /* use gdb> target command */\n");
> +     printf("     tcp:host:port    target remote host:port  \n");
> +#ifndef _WIN32
> +     printf("     unix:name        target remote | socat stdio unix:name\n");
> +     printf("     unix:name,unlink : unlink remove name from filesystem after connection\n");
> +#endif /* _WIN32 */
> +}     
> +
> +int gdbserver_start (const char *device)
> +{
> +    const char *p;
> +    char *name, *devicename,*opt;
> +
> +    name = malloc (strlen(device)+1);
> +    /* Light memory leak : will never be freed */
> +
> +    if (strstart (device, "tcp:",&p)) {
> +        if ((p == NULL) || (strlen(p) == 0)) {
> +            perror ("tcp socket syntax error");
> +            return -1;
> +        }
> +        strcpy (name,p);
> +        devicename = strrchr (name,':');
> +        if (devicename == NULL) {
> +            return -1;
> +        }
> +        *devicename++ = 0;
> +        if (*devicename == 0) {
> +            return -1;
> +        }
> +        if (*name == 0) {
> +            name = NULL;
> +        }
> +        return gdbserver_tcp_start (name,devicename,
> +                                    gdbserver_tcp_accept);
> +#ifndef _WIN32
> +    } else if (strstart (device, "unix:",&p)) {
> +        strcpy (name,p);
> +        opt = strrchr(name,',');
> +        if (opt == NULL) {
> +           return gdbserver_unix_start(name,gdbserver_unix_accept);
> +        } else {
> +           *opt++ = 0;
> +           if (!strcmp(opt,"unlink")) {
> +              return gdbserver_unix_start(name,
> +                                          gdbserver_unix_accept_unlink);
> +           } else {
> +              fprintf (stderr,"gdb unix socket : unknown %s option\n",opt);
> +              return -1;
> +           }
> +        } 
> +#endif /* _WIN32 */
> +    } else {
> +       return -1;
> +    }
>      return 0;
>  }
>  
> +/* Tell the remote gdb that the process has exited due to SIG.  */
>  /* Disable gdb stub for child processes.  */
>  void gdbserver_fork(CPUState *env)
>  {

Does that new comment really belong here?

> @@ -2266,7 +2435,14 @@
>      cpu_breakpoint_remove_all(env, BP_GDB);
>      cpu_watchpoint_remove_all(env, BP_GDB);
>  }
> -#else
> +
> +#else /* CONFIG_USER_ONLY */
> +
> +static void put_buffer(GDBState *s, const uint8_t *buf, int len)
> +{
> +    qemu_chr_write(s->chr, buf, len);
> +}
> +
>  static int gdb_chr_can_receive(void *opaque)
>  {
>    /* We can handle an arbitrarily large amount of data.
> @@ -2330,7 +2506,7 @@
>      if (vm_running)
>          vm_stop(EXCP_INTERRUPT);
>  }
> -#endif
> +#endif /* _WIN32 */

Skip such not directly related changes.

>  
>  int gdbserver_start(const char *device)
>  {
> @@ -2356,7 +2532,7 @@
>              act.sa_handler = gdb_sigterm_handler;
>              sigaction(SIGINT, &act, NULL);
>          }
> -#endif
> +#endif /* _WIN32 */
>          chr = qemu_chr_open("gdb", device, NULL);
>          if (!chr)
>              return -1;
> @@ -2390,4 +2566,4 @@
>  
>      return 0;
>  }
> -#endif
> +#endif /* CONFIG_USER_ONLY */
> Index: gdbstub.h
> ===================================================================
> --- gdbstub.h	(révision 7105)
> +++ gdbstub.h	(copie de travail)
> @@ -16,20 +16,25 @@
>  void gdb_do_syscall(gdb_syscall_complete_cb cb, const char *fmt, ...);
>  int use_gdb_syscalls(void);
>  void gdb_set_stop_cpu(CPUState *env);
> +
>  #ifdef CONFIG_USER_ONLY
> +typedef int (*gdbstub_func) (void);
> +extern gdbstub_func gdbstub_accept;
> +
>  int gdb_queuesig (void);
>  int gdb_handlesig (CPUState *, int);
>  void gdb_exit(CPUState *, int);
>  void gdb_signalled(CPUState *, int);
> -int gdbserver_start(int);
>  void gdbserver_fork(CPUState *);
> -#else
> -int gdbserver_start(const char *port);
> -#endif
> +void gdbstub_usage (void);
> +#endif /* CONFIG_USER_ONLY */
> +
> +int gdbserver_start(const char *device);
> +
>  /* Get or set a register.  Returns the size of the register.  */
>  typedef int (*gdb_reg_cb)(CPUState *env, uint8_t *buf, int reg);
>  void gdb_register_coprocessor(CPUState *env,
>                                gdb_reg_cb get_reg, gdb_reg_cb set_reg,
>                                int num_regs, const char *xml, int g_pos);
>  
> -#endif
> +#endif /* GDBSTUB_H */

I still think you are on the right track, just minor issues, some coding
style cleanups and the gdbstub_accept refactoring.

Thanks so far!
Jan

-- 
Siemens AG, Corporate Technology, CT SE 2
Corporate Competence Center Embedded Linux