| Message ID | 20230818090224.409192-4-chigot@adacore.com |
|---|---|
| State | New |
| Headers | show |
| Series | Risc-V/gdb: replace exit(0) with proper shutdown | expand |
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
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
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); } int gdb_handlesig(CPUState *cpu, int sig)
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(-)