Re: [Qemu-trivial] [Qemu-devel] [PATCH] gdbstub: avoid busy loop while waiting for gdb

[Peter Maydell]

On 3 May 2016 at 23:58, Peter Wu <address@hidden> wrote:
> While waiting for a gdb response, or while sending an acknowledgement
> there is not much to do, so just mark the socket as non-blocking to
> avoid a busy loop while paused at gdb. This only affects the user-mode
> emulation (qemu-arm -g 1234 ./a.out).
>
> Note that this issue was reported before at
> https://lists.nongnu.org/archive/html/qemu-devel/2013-02/msg02277.html.
>
> While at it, close the gdb client fd on EOF or error while reading.
>
> Signed-off-by: Peter Wu <address@hidden>
> ---
>  gdbstub.c | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
>
> diff --git a/gdbstub.c b/gdbstub.c
> index 0e431fd..b126bf5 100644
> --- a/gdbstub.c
> +++ b/gdbstub.c
> @@ -332,7 +332,7 @@ static int get_char(GDBState *s)
>          if (ret < 0) {
>              if (errno == ECONNRESET)
>                  s->fd = -1;
> -            if (errno != EINTR && errno != EAGAIN)
> +            if (errno != EINTR)
>                  return -1;
>          } else if (ret == 0) {
>              close(s->fd);
> @@ -393,7 +393,7 @@ static void put_buffer(GDBState *s, const uint8_t *buf, 
> int len)
>      while (len > 0) {
>          ret = send(s->fd, buf, len, 0);
>          if (ret < 0) {
> -            if (errno != EINTR && errno != EAGAIN)
> +            if (errno != EINTR)
>                  return;
>          } else {
>              buf += ret;
> @@ -1542,9 +1542,13 @@ gdb_handlesig(CPUState *cpu, int sig)
>              for (i = 0; i < n; i++) {
>                  gdb_read_byte(s, buf[i]);
>              }
> -        } else if (n == 0 || errno != EAGAIN) {
> +        } else {
>              /* XXX: Connection closed.  Should probably wait for another
>                 connection before continuing.  */
> +            if (n == 0) {
> +                close(s->fd);
> +            }
> +            s->fd = -1;
>              return sig;
>          }
>      }
> @@ -1599,8 +1603,6 @@ static void gdb_accept(void)
>      gdb_has_xml = false;
>
>      gdbserver_state = s;
> -
> -    fcntl(fd, F_SETFL, O_NONBLOCK);
>  }
>
>  static int gdbserver_open(int port)

It does look like all the places we use s->fd:
 * are only for user-mode
 * are currently coded as "loop round until we get something",
   so effectively blocking but doing it with a busy loop

I suspect the O_NONBLOCK here may be legacy from before the
system-emulation gdbstub was reworked to use a chr backend
(in system emulation there really are other things we need
to attend to besides gdb stub packets, like the monitor).

Other than the error in the commit message,
Reviewed-by: Peter Maydell <address@hidden>

thanks
-- PMM