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

[Clément Chigot]

On Fri, Aug 18, 2023 at 11:10 AM Peter Maydell <peter.maydell@linaro.org> wrote:
>
> 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.

Indeed, I did miss that. I used to have it directly in
gdb_handle_packet and in handle_v_kill. But now that the support of
softmmu and user has been splitted, I thought putting it in gdb_exit
was a solution.
However, IIUC the code, the second request will simply be ignored, the
main loop (where the requests matter) have been already exited.
I see what I can do anyway to avoid this double request.

> 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().

Here, I'm seeing the symptoms with a gdb connection being closed too
sharply and a fd not being flush. But looking at the qemu_cleanup(),
many things could require a proper cleanup which will be skipped by a
simple exit(0). This could lead to many unexpected side effects.

Thanks,
Clément