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

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

[Ross Lagerwall]

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. 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@citrix.com>
---
 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[] = {
-- 
2.31.1

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

[Juergen Gross]

[-- Attachment #1.1.1: Type: text/plain, Size: 1633 bytes --]

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.

 > 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@citrix.com>
> ---
>   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.


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3149 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

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

[Juergen Gross]

[-- Attachment #1.1.1: Type: text/plain, Size: 2518 bytes --]

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@lists.xenproject.org
>> 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@suse.com>

> 
>>
>>> 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@citrix.com>
>>> ---
>>>    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 #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3149 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

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

[Ross Lagerwall]

> From: Juergen Gross
> Sent: Monday, June 27, 2022 3:49 PM
> To: Ross Lagerwall; xen-devel@lists.xenproject.org
> 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.

> 
> > 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@citrix.com>
> > ---
> >   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.

Thanks,
Ross