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

[Clément Chigot]

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