| Message ID | 20230823070735.363197-1-chigot@adacore.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2,1/3] hw/misc/sifive_test.c: replace exit(0) with proper shutdown | expand |
On Wed, Aug 23, 2023 at 5:08 PM 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> Acked-by: Alistair Francis <alistair.francis@wdc.com> Alistair > --- > 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); > + } > } > > /* > 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) > -- > 2.25.1 > >
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
On Mon, Sep 4, 2023 at 11:23 AM Peter Maydell <peter.maydell@linaro.org> wrote: > > 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. I do agree but AFAIK, this isn't possible to pass the exit code using qemu_system_shutdown_request. Would it make more sense to trigger a SHUTDOWN_CAUSE_GUEST_PANIC instead ? This would result in a non-0 exit code and the already available gdb_trace would show the real exit code if needed. Clément
On Mon, 4 Sept 2023 at 10:36, Clément Chigot <chigot@adacore.com> wrote: > > On Mon, Sep 4, 2023 at 11:23 AM Peter Maydell <peter.maydell@linaro.org> wrote: > > > > 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> > > > + /* > > > + * 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. > > I do agree but AFAIK, this isn't possible to pass the exit code using > qemu_system_shutdown_request. That would mean that we should add a mechanism to do so. But my opinion is still what I said about the first version of this patchset: we should fix whatever the problem is that means that gdb_exit() is not correctly ensuring that gdb gets the packet response, not paper over it like this. thanks -- PMM
On Mon, Sep 4, 2023 at 11:42 AM Peter Maydell <peter.maydell@linaro.org> wrote: > > On Mon, 4 Sept 2023 at 10:36, Clément Chigot <chigot@adacore.com> wrote: > > > > On Mon, Sep 4, 2023 at 11:23 AM Peter Maydell <peter.maydell@linaro.org> wrote: > > > > > > 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> > > > > > + /* > > > > + * 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. > > > > I do agree but AFAIK, this isn't possible to pass the exit code using > > qemu_system_shutdown_request. > > That would mean that we should add a mechanism to do so. > > But my opinion is still what I said about the first version > of this patchset: we should fix whatever the problem is > that means that gdb_exit() is not correctly ensuring that > gdb gets the packet response, not paper over it like this. The main issue is that calling exit(0) bypasses the call of qemu_cleanup(). For the two other patches, the wrong behavior is obvious: qemu_cleanup being not called so is gdb_exit and then the gdb packet is never even created, let alone being sent. Replacing exit by a shutdown request ensures that the softmmu main loop terminates and that qemu_cleanup/gdb_exit is being called. For this one, I have to verify a bit further. Honestly, I did include it for the sake of coherence and because we used to need it. However, I've realized that this was earlier to commit b9e10c6c (which adds explicit calls to gdb_exit). It might not be mandatory after all, even if I still think that this is an improvement as bypassing qemu_cleanup could lead to many leaks if contributors expect it to be called once the softmmu main loop has started. Clément
Hi Peter, On Mon, Sep 4, 2023 at 2:46 PM Clément Chigot <chigot@adacore.com> wrote: > > On Mon, Sep 4, 2023 at 11:42 AM Peter Maydell <peter.maydell@linaro.org> wrote: > > > > On Mon, 4 Sept 2023 at 10:36, Clément Chigot <chigot@adacore.com> wrote: > > > > > > On Mon, Sep 4, 2023 at 11:23 AM Peter Maydell <peter.maydell@linaro.org> wrote: > > > > > > > > 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> > > > > > > > + /* > > > > > + * 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. > > > > > > I do agree but AFAIK, this isn't possible to pass the exit code using > > > qemu_system_shutdown_request. > > > > That would mean that we should add a mechanism to do so. > > > > But my opinion is still what I said about the first version > > of this patchset: we should fix whatever the problem is > > that means that gdb_exit() is not correctly ensuring that > > gdb gets the packet response, not paper over it like this. > > The main issue is that calling exit(0) bypasses the call of qemu_cleanup(). > For the two other patches, the wrong behavior is obvious: qemu_cleanup > being not called so is gdb_exit and then the gdb packet is never even > created, let alone being sent. Replacing exit by a shutdown request > ensures that the softmmu main loop terminates and that > qemu_cleanup/gdb_exit is being called. > > For this one, I have to verify a bit further. Honestly, I did include > it for the sake of coherence and because we used to need it. However, > I've realized that this was earlier to commit b9e10c6c (which adds > explicit calls to gdb_exit). It might not be mandatory after all, even > if I still think that this is an improvement as bypassing qemu_cleanup > could lead to many leaks if contributors expect it to be called once > the softmmu main loop has started. For the cases I've tried, there was indeed no leak if qemu_cleanup was not called. However, digging a bit in the qemu, I found that net/vhost-vdpa.c is mentioning that its cleanup function has to be called through qemu_cleanup in some cases to perform its final sanitation. Thus, I've improved and kept the commit dealing with gdbstub (in v3 just sent). If you really think it's safer to avoid that, I'm ok to drop it until I find a leak/issue (if any) caused by qemu_cleanup not being called here. 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..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); + } } /* 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 | 13 +++++++++++++ gdbstub/user.c | 2 ++ 3 files changed, 16 insertions(+), 2 deletions(-)