[v3,5/8] package/petitboot: fix shutdown

There are a few ways to set HOST_PROG_SHUTDOWN that enable us to
successfully kexec the new kernel chosen from the petitboot menu.

1. HOST_PROG_SHUTDOWN=/usr/libexec/petitboot/bb-kexec-reboot
    `- bb-kexec-reboot does 'exec kill -QUIT 1'
        |- init sends SIGTERM to pb-console
        |   `- pb-console does 'trap 'reset; echo "SIGTERM received, booting..."; sleep 2' SIGTERM'
        `- init runs 'null::restart:/usr/sbin/kexec-restart'
            `- kexec-restart does '/usr/sbin/kexec -f -e'

2. HOST_PROG_SHUTDOWN=/usr/sbin/kexec-restart
    `- kexec-restart does '/usr/sbin/kexec -f -e'

3. unset HOST_PROG_SHUTDOWN (default is /sbin/shutdown)
    `- petitboot tries, in order:
        |- 1. '/sbin/shutdown -r now'
        |      `- file not found
        |- 2. '/usr/sbin/kexec -e'
        `- 3. '/usr/sbin/kexec -e -f'

If we're using busybox init, do (1). This performs a proper shutdown
before the kexec, giving us nice console output with a terminal reset
and the "booting..." notification. Otherwise, fall back to (2).

Signed-off-by: Reza Arbab <arbab@linux.ibm.com>
---
 package/petitboot/kexec-restart |  8 ++++++++
 package/petitboot/petitboot.mk  | 16 ++++++++++++++--
 2 files changed, 22 insertions(+), 2 deletions(-)
 create mode 100755 package/petitboot/kexec-restart

Hi Reza,

On 09/10/2023 17:17, Reza Arbab wrote:
> There are a few ways to set HOST_PROG_SHUTDOWN that enable us to
> successfully kexec the new kernel chosen from the petitboot menu.
> 
> 1. HOST_PROG_SHUTDOWN=/usr/libexec/petitboot/bb-kexec-reboot
>      `- bb-kexec-reboot does 'exec kill -QUIT 1'
>          |- init sends SIGTERM to pb-console
>          |   `- pb-console does 'trap 'reset; echo "SIGTERM received, booting..."; sleep 2' SIGTERM'
>          `- init runs 'null::restart:/usr/sbin/kexec-restart'
>              `- kexec-restart does '/usr/sbin/kexec -f -e'
> 
> 2. HOST_PROG_SHUTDOWN=/usr/sbin/kexec-restart
>      `- kexec-restart does '/usr/sbin/kexec -f -e'
> 
> 3. unset HOST_PROG_SHUTDOWN (default is /sbin/shutdown)
>      `- petitboot tries, in order:
>          |- 1. '/sbin/shutdown -r now'
>          |      `- file not found
>          |- 2. '/usr/sbin/kexec -e'
>          `- 3. '/usr/sbin/kexec -e -f'
> 
> If we're using busybox init, do (1). This performs a proper shutdown
> before the kexec, giving us nice console output with a terminal reset
> and the "booting..." notification. Otherwise, fall back to (2).

  This doesn't explain _why_ this is the proper approach, or why the current one 
needs to be fixed.

  It seems to me that (1) would be the proper thing to do for sysvinit, openrc 
and systemd based init as well (not that anyone would be crazy enough to put 
systemd in a petitboot image, I hope...). So, if I understand correctly, what 
needs to be fixed is when init is not an actual init, but something trivial like 
a single shell script. So at the very least, the condition should change I think.

  But then, BR2_INIT_NONE is not a reliable indication that PID 1 is not a real 
init. So I think it would be better to instead decide at runtime which approach 
to take. That could be done for instance by looking at /proc/1/exe and comparing 
it with /sbin/init - if they are the same, we have a real init; if not, we 
probably have a shell or some such.

  But maybe I misunderstand the reasoning completely.

  Also, why don't we simply always do (2)? The only difference is the message on 
pb-console, and normally it would anyway be pb-console that triggers the boot so 
it would already have shown something, I expect?

> 
> Signed-off-by: Reza Arbab <arbab@linux.ibm.com>
> ---
>   package/petitboot/kexec-restart |  8 ++++++++
>   package/petitboot/petitboot.mk  | 16 ++++++++++++++--
>   2 files changed, 22 insertions(+), 2 deletions(-)
>   create mode 100755 package/petitboot/kexec-restart
> 
> diff --git a/package/petitboot/kexec-restart b/package/petitboot/kexec-restart
> new file mode 100755
> index 000000000000..62fbea114513
> --- /dev/null
> +++ b/package/petitboot/kexec-restart
> @@ -0,0 +1,8 @@
> +#!/bin/sh
> +
> +/usr/sbin/kexec -f -e
> +
> +while :
> +do
> +	sleep 1
> +done
> diff --git a/package/petitboot/petitboot.mk b/package/petitboot/petitboot.mk
> index 0992111cb1ec..5c7913b560b1 100644
> --- a/package/petitboot/petitboot.mk
> +++ b/package/petitboot/petitboot.mk
> @@ -21,7 +21,6 @@ PETITBOOT_CONF_OPTS = \
>   	--without-twin-x11 \
>   	$(if $(BR2_PACKAGE_BUSYBOX),--enable-busybox,--disable-busybox) \
>   	HOST_PROG_KEXEC=/usr/sbin/kexec \

  This leaves a trailing backslash here, as reported by check-package.

> -	HOST_PROG_SHUTDOWN=/usr/libexec/petitboot/bb-kexec-reboot
>   
>   # HPA and Busybox tftp are supported. HPA tftp is part of Buildroot's tftpd
>   # package.
> @@ -46,6 +45,17 @@ else
>   PETITBOOT_CONF_OPTS += --without-fdt
>   endif
>   
> +ifeq ($(BR2_INIT_BUSYBOX),y)
> +PETITBOOT_CONF_OPTS += HOST_PROG_SHUTDOWN=/usr/libexec/petitboot/bb-kexec-reboot
> +define PETITBOOT_INITTAB
> +	grep -q kexec-restart $(TARGET_DIR)/etc/inittab || \
> +		printf "\nnull::restart:/usr/sbin/kexec-restart\n" >> $(TARGET_DIR)/etc/inittab

  So, if I understand correctly, previously this wasn't booting _at all_? I 
mean, it would just reboot instead of kexec'ing like it should?


  I've marked the patch as Changes Requested.

  Regards,
  Arnout


> +endef
> +PETITBOOT_TARGET_FINALIZE_HOOKS += PETITBOOT_INITTAB
> +else
> +PETITBOOT_CONF_OPTS += HOST_PROG_SHUTDOWN=/usr/sbin/kexec-restart
> +endif
> +
>   define PETITBOOT_POST_INSTALL
>   	$(INSTALL) -D -m 0755 $(@D)/utils/bb-kexec-reboot \
>   		$(TARGET_DIR)/usr/libexec/petitboot/bb-kexec-reboot
> @@ -53,8 +63,10 @@ define PETITBOOT_POST_INSTALL
>   		$(TARGET_DIR)/etc/petitboot/boot.d/01-create-default-dtb
>   	$(INSTALL) -D -m 0755 $(@D)/utils/hooks/90-sort-dtb \
>   		$(TARGET_DIR)/etc/petitboot/boot.d/90-sort-dtb
> -	$(INSTALL) -m 0755 -D $(PETITBOOT_PKGDIR)/S15pb-discover \
> +	$(INSTALL) -D -m 0755 $(PETITBOOT_PKGDIR)/S15pb-discover \
>   		$(TARGET_DIR)/etc/init.d/S15pb-discover
> +	$(INSTALL) -D -m 0755 $(PETITBOOT_PKGDIR)/kexec-restart \
> +		$(TARGET_DIR)/usr/sbin/kexec-restart
>   	mkdir -p $(TARGET_DIR)/usr/share/udhcpc/default.script.d/
>   	ln -sf /usr/sbin/pb-udhcpc \
>   		$(TARGET_DIR)/usr/share/udhcpc/default.script.d/

On Sun, Nov 05, 2023 at 06:57:47PM +0100, Arnout Vandecappelle wrote:
>On 09/10/2023 17:17, Reza Arbab wrote:
>>There are a few ways to set HOST_PROG_SHUTDOWN that enable us to
>>successfully kexec the new kernel chosen from the petitboot menu.
>>
>>1. HOST_PROG_SHUTDOWN=/usr/libexec/petitboot/bb-kexec-reboot
>>     `- bb-kexec-reboot does 'exec kill -QUIT 1'
>>         |- init sends SIGTERM to pb-console
>>         |   `- pb-console does 'trap 'reset; echo "SIGTERM received, booting..."; sleep 2' SIGTERM'
>>         `- init runs 'null::restart:/usr/sbin/kexec-restart'
>>             `- kexec-restart does '/usr/sbin/kexec -f -e'
[...]
> It seems to me that (1) would be the proper thing to do for sysvinit, 
>openrc and systemd based init as well (not that anyone would be crazy 
>enough to put systemd in a petitboot image, I hope...). So, if I 
>understand correctly, what needs to be fixed is when init is not an 
>actual init, but something trivial like a single shell script. So at 
>the very least, the condition should change I think.
>
> But then, BR2_INIT_NONE is not a reliable indication that PID 1 is 
>not a real init. So I think it would be better to instead decide at 
>runtime which approach to take. That could be done for instance by 
>looking at /proc/1/exe and comparing it with /sbin/init - if they are 
>the same, we have a real init; if not, we probably have a shell or 
>some such.

Good idea! I'll set HOST_PROG_SHUTDOWN=kexec-restart unconditionally, 
and make that script smart enough to accomodate all the init options.

> Also, why don't we simply always do (2)? The only difference is the 
>message on pb-console, and normally it would anyway be pb-console that 
>triggers the boot so it would already have shown something, I expect?

Well, just in general it's better to terminate things properly before 
kexec. But also, pb-console doesn't really trigger the boot per se--it's 
pb-discover doing that in the background, so the SIGTERM pb-console 
receives is considered the final notice that we're going to boot.

What's nicer than the "booting..." notification is actually the 
preceding terminal reset, so you exit the ncurses visual mode,  
scrolling is raw rather than being confined to the window ncurses set 
up, etc.

>>@@ -46,6 +45,17 @@ else
>>  PETITBOOT_CONF_OPTS += --without-fdt
>>  endif
>>+ifeq ($(BR2_INIT_BUSYBOX),y)
>>+PETITBOOT_CONF_OPTS += HOST_PROG_SHUTDOWN=/usr/libexec/petitboot/bb-kexec-reboot
>>+define PETITBOOT_INITTAB
>>+	grep -q kexec-restart $(TARGET_DIR)/etc/inittab || \
>>+		printf "\nnull::restart:/usr/sbin/kexec-restart\n" >> $(TARGET_DIR)/etc/inittab
>
> So, if I understand correctly, previously this wasn't booting _at 
>all_? I mean, it would just reboot instead of kexec'ing like it 
>should?

Yes, at the moment we are doing `kill -QUIT 1` but without the above 
"restart" entry to do the kexec.