On Fri, Aug 18, 2023 at 11:10 AM Peter Maydell <peter.mayd...@linaro.org> wrote:
>
> On Fri, 18 Aug 2023 at 10:03, Clément Chigot <chi...@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 <chi...@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
