Re: [PATCH] xen/manage: Use orderly_reboot() to reboot

[Juergen Gross <jgross@xxxxxxxx>]

On 27.06.22 17:29, Ross Lagerwall wrote:
> > From: Juergen Gross
> > Sent: Monday, June 27, 2022 3:49 PM
> > To: Ross Lagerwall; xen-devel@xxxxxxxxxxxxxxxxxxxx
> > Cc: Stefano Stabellini; Oleksandr Tyshchenko; Dongli Zhang; Boris Ostrovsky
> > Subject: Re: [PATCH] xen/manage: Use orderly_reboot() to reboot
> >              
> >             
> > On 27.06.22 16:28, Ross Lagerwall wrote:
> > > Currently when the toolstack issues a reboot, it gets translated into a
> > > call to ctrl_alt_del(). But tying reboot to ctrl-alt-del means rebooting
> > > may fail if e.g. the user has masked the ctrl-alt-del.target under
> > > systemd.
> > >
> > > A previous attempt to fix this set the flag to force rebooting when
> > > ctrl_alt_del() is called.
> > Sorry, I have problems parsing this sentence.
> Probably because it is poorly worded... How about this?
>
> A previous attempt to fix this issue made a change that sets the
> kernel.ctrl-alt-del sysctl to 1 before ctrl_alt_del() is called.
Yes, much better.

With that (can be done while committing):

Reviewed-by: Juergen Gross <jgross@xxxxxxxx>

> > > However, this doesn't give userspace the
> > > opportunity to block rebooting or even do any cleanup or syncing.
> > >
> > > Instead, call orderly_reboot() which will call the "reboot" command,
> > > giving userspace the opportunity to block it or perform the usual reboot
> > > process while being independent of the ctrl-alt-del behaviour. It also
> > > matches what happens in the shutdown case.
> > >
> > > Signed-off-by: Ross Lagerwall <ross.lagerwall@xxxxxxxxxx>
> > > ---
> > >    drivers/xen/manage.c | 2 +-
> > >    1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/xen/manage.c b/drivers/xen/manage.c
> > > index 3d5a384d65f7..c16df629907e 100644
> > > --- a/drivers/xen/manage.c
> > > +++ b/drivers/xen/manage.c
> > > @@ -205,7 +205,7 @@ static void do_poweroff(void)
> > >    static void do_reboot(void)
> > >    {
> > >         shutting_down = SHUTDOWN_POWEROFF; /* ? */
> > > -     ctrl_alt_del();
> > > +     orderly_reboot();
> > >    }
> > >    
> > >    static struct shutdown_handler shutdown_handlers[] = {
> > The code seems to be fine.
> >
> > Albeit I wonder whether we shouldn't turn shutting_down into a bool,
> > as all that seems to be needed is "shutting_down != SHUTDOWN_INVALID"
> > today. But this could be part of another patch.
> Agreed that shutting_down could be a bool but better to change it
> in a separate patch.
Yes.


Juergen

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature
Description: OpenPGP digital signature