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

[Peter Maydell]

On Wed, 23 Aug 2023 at 08:07, 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 | 13 +++++++++++++
>  gdbstub/user.c    |  2 ++
>  3 files changed, 16 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..fa9b09537d 100644
> --- a/gdbstub/softmmu.c
> +++ b/gdbstub/softmmu.c
> @@ -434,6 +434,19 @@ 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.
> +     * As this function is also called during the cleanup process,
> +     * avoid sending the request if one is already set.
> +     */
> +    if (code) {
> +        exit(code);
> +    } else if (!qemu_shutdown_requested_get()) {
> +        qemu_system_shutdown_request(SHUTDOWN_CAUSE_GUEST_SHUTDOWN);
> +    }
>  }

This definitely doesn't look right. Either exit() is OK
to call, or it is not. We shouldn't be exiting one way
if the exit status is 0 and another way if it is non-0.

thanks
-- PMM