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

[Clément Chigot]

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