Re: [PATCH 3/3] gdbstub: replace exit(0) with proper shutdown

[Peter Maydell]

On Fri, 18 Aug 2023 at 10:03, Clément Chigot <chigot@adacore.com> wrote:
>
> This replaces the exit(0) call by a shutdown request, ensuring a proper
> cleanup of Qemu. Otherwise, some connections could be broken without
> being correctly flushed.
>
> Signed-off-by: Clément Chigot <chigot@adacore.com>
> ---
>  gdbstub/gdbstub.c |  3 +--
>  gdbstub/softmmu.c | 11 +++++++++++
>  gdbstub/user.c    |  2 ++
>  3 files changed, 14 insertions(+), 2 deletions(-)
>
> diff --git a/gdbstub/gdbstub.c b/gdbstub/gdbstub.c
> index 5f28d5cf57..358eed1935 100644
> --- a/gdbstub/gdbstub.c
> +++ b/gdbstub/gdbstub.c
> @@ -1298,7 +1298,6 @@ static void handle_v_kill(GArray *params, void 
> *user_ctx)
>      gdb_put_packet("OK");
>      error_report("QEMU: Terminated via GDBstub");
>      gdb_exit(0);
> -    exit(0);
>  }
>
>  static const GdbCmdParseEntry gdb_v_commands_table[] = {
> @@ -1818,7 +1817,7 @@ static int gdb_handle_packet(const char *line_buf)
>          /* Kill the target */
>          error_report("QEMU: Terminated via GDBstub");
>          gdb_exit(0);
> -        exit(0);
> +        break;
>      case 'D':
>          {
>              static const GdbCmdParseEntry detach_cmd_desc = {
> diff --git a/gdbstub/softmmu.c b/gdbstub/softmmu.c
> index f509b7285d..9ca7ae10bc 100644
> --- a/gdbstub/softmmu.c
> +++ b/gdbstub/softmmu.c
> @@ -434,6 +434,17 @@ void gdb_exit(int code)
>      }
>
>      qemu_chr_fe_deinit(&gdbserver_system_state.chr, true);
> +
> +    /*
> +     * Shutdown request is a clean way to stop the QEMU, compared
> +     * to a direct call to exit(). But we can't pass the exit code
> +     * through it so avoid doing that when it can matter.
> +     */
> +    if (code) {
> +        exit(code);
> +    } else {
> +        qemu_system_shutdown_request(SHUTDOWN_CAUSE_GUEST_SHUTDOWN);
> +    }
>  }
>
>  /*
> diff --git a/gdbstub/user.c b/gdbstub/user.c
> index 5b375be1d9..f3d97d621f 100644
> --- a/gdbstub/user.c
> +++ b/gdbstub/user.c
> @@ -113,6 +113,8 @@ void gdb_exit(int code)
>          gdb_put_packet(buf);
>          gdbserver_state.allow_stop_reply = false;
>      }
> +
> +    exit(code);
>  }

These are not the only places that call gdb_exit().
Notably, qemu_cleanup() calls it, and I'm pretty sure
it does not expect that gdb_exit() will either call
exit() or qemu_system_shutdown_request(), because it's
already in the process of cleaning up and stopping
the system.

If we send the "we're exiting" report to the gdb process,
that ought to be sufficient. If we're not actually managing
to send that last packet to gdb because we don't flush
the data out to the file descriptor, then it seems to me
that the fix ought to be to ensure we do do that flush
as part of gdb_exit() not to rearrange or try to avoid
all the subsequent calls to exit().

-- PMM