| Message ID | 20200121083509.24067-2-ynezz@true.cz |
|---|---|
| State | Accepted |
| Delegated to: | Petr Štetiar |
| Headers | show |
| Series | [OpenWrt-Devel,procd,1/2] instance: provide error feedback if ujail binary is missing | expand |
A few small remarks, see in-line, regards, Paul > Op 21 jan. 2020, om 09:35 heeft Petr Štetiar <ynezz@true.cz> het volgende geschreven: > > Executing `reboot` command in OpenWrt system runing inside LXC container > results in a shutdown of the container instead of rebooting the > container. > > This appears to have been caused by commit 832369078d81 ("state: fix > shutdown when running in a container (FS#2425)"), which exits the pid > einz instead of the reboot(). > > While at it, refactor the halting code into separate function to shorten > the switch/case block and make it clearer, decrease the indentation > level by reversing the container if condition, replace magic 0 with > EXIT_SUCCESS constant in exit() and make it wait 1s for reboot message > delivery in both container/host cases as well. > > Ref: FS#2666 > Cc: Paul Spooren <mail@aparcar.org> > Fixes: 832369078d81 ("state: fix shutdown when running in a container (FS#2425)") > Tested-by: Baptiste Jonglez <lede@bitsofnetworks.org> > Signed-off-by: Petr Štetiar <ynezz@true.cz> > --- > state.c | 52 ++++++++++++++++++++++++++++++---------------------- > 1 file changed, 30 insertions(+), 22 deletions(-) > > diff --git a/state.c b/state.c > index 4737d0121ad0..e117ea302f93 100644 > --- a/state.c > +++ b/state.c > @@ -94,6 +94,34 @@ static void set_console(void) > set_stdio(tty); > } > > +static void perform_halt() > +{ > + if (reboot_event == RB_POWER_OFF) > + LOG("- power down -\n"); > + else > + LOG("- reboot -\n"); > + > + /* Allow time for last message to reach serial console, etc */ > + sleep(1); > + > + if (is_container()) { > + reboot(reboot_event); When reboot returns, hasn't something gone wrong then ? > + exit(EXIT_SUCCESS); The return below after exit() can never be reached. > + return; > + } > + > + /* We have to fork here, since the kernel calls do_exit(EXIT_SUCCESS) > + * in linux/kernel/sys.c, which can cause the machine to panic when > + * the init process exits... */ > + if (!vfork()) { /* child */ > + reboot(reboot_event); When reboot returns, hasn't something gone wrong then ? > + _exit(EXIT_SUCCESS); > + } > + > + while (1) > + sleep(1); > +} > + > static void state_enter(void) > { > char ubus_cmd[] = "/sbin/ubusd"; > @@ -153,29 +181,9 @@ static void state_enter(void) > sync(); > sleep(1); > #ifndef DISABLE_INIT > - if (reboot_event == RB_POWER_OFF) > - LOG("- power down -\n"); > - else > - LOG("- reboot -\n"); > - > - if (!is_container()) { > - /* Allow time for last message to reach serial console, etc */ > - sleep(1); > - > - /* We have to fork here, since the kernel calls do_exit(EXIT_SUCCESS) > - * in linux/kernel/sys.c, which can cause the machine to panic when > - * the init process exits... */ > - if (!vfork( )) { /* child */ > - reboot(reboot_event); > - _exit(EXIT_SUCCESS); > - } > - > - while (1) > - sleep(1); > - } else > - exit(0); > + perform_halt(); > #else > - exit(0); > + exit(EXIT_SUCCESS); > #endif > break; > > > _______________________________________________ > openwrt-devel mailing list > openwrt-devel@lists.openwrt.org > https://lists.openwrt.org/mailman/listinfo/openwrt-devel
Paul Oranje <por@oranjevos.nl> [2020-01-22 11:09:22]: Hi, thanks for review. > > + if (is_container()) { > > + reboot(reboot_event); > When reboot returns, hasn't something gone wrong then ? What do you suggest? I dont know how that behaves in all environments in order to answer that question and I've following "However, in containers reboot() call is ignored"[1] in my head since I've discovered it. > > + exit(EXIT_SUCCESS); > The return below after exit() can never be reached. What do you suggest? Does that additional return hurts that much? I mean, if we keep it, it's clear, that the code bellow the return cant be ever reached, omitting it leaves some possibility. Debugging this stuff is PITA. 1. https://git.busybox.net/busybox/tree/init/init.c#n750 -- ynezz
Op 23 jan. 2020, om 10:28 heeft Petr Štetiar <ynezz@true.cz> het volgende geschreven: > > Paul Oranje <por@oranjevos.nl> [2020-01-22 11:09:22]: > > Hi, > > thanks for review. > >>> + if (is_container()) { >>> + reboot(reboot_event); >> When reboot returns, hasn't something gone wrong then ? > > What do you suggest? Falling through to exit() seems arbitrary, the return statement after that is a double whammy. Suggestion: log of this unexpected condition or even stronger, an assert(). > I dont know how that behaves in all environments in order to answer that > question and I've following "However, in containers reboot() call is > ignored"[1] in my head since I've discovered it. Ah, so the code builds on that busybox implementation. An assert() would kill >>> + exit(EXIT_SUCCESS); >> The return below after exit() can never be reached. > > What do you suggest? Suggestion: remove the superfluous return statement or replace it with a comment like /* NEVER REACHED */. Decorating perform_halt() with the _Noreturn function specifier might be an idea as it allows the compiler to warn when the function would possibly return (as of "c11 6.7.4 Function specifiers", or is an older version of the C standard to be used ?). > Does that additional return hurts that much? I mean, if we keep it, it's > clear, that the code bellow the return cant be ever reached, omitting it > leaves some possibility. Debugging this stuff is PITA. Of coarse it does not really hurt, it's a bit like an if that always returns with an else clause, in that it is meaningless. Besides, when code coverage analysis is used, these won't dirty the analysis. > > 1. https://git.busybox.net/busybox/tree/init/init.c#n750 Bye, Paul
On January 23, 2020 1:04:16 PM UTC, Paul Oranje <por@oranjevos.nl> wrote: >Suggestion: log of this unexpected condition or even stronger, an >assert(). All logging is killed already, logging just to serial console won't cut it, assert is overkill, which needs compiler flags/ifdef fiddling and who knows how that would behave in different environments. I could split it into separate functions if that implicit return would be more acceptable for you. It just seems overkill to me, just to make both of us satisfied :-) >Decorating perform_halt() with the _Noreturn function specifier might Ok, I like this one.
Op 23 jan. 2020, om 15:58 heeft Petr Štetiar <ynezz@true.cz> het volgende geschreven: > > On January 23, 2020 1:04:16 PM UTC, Paul Oranje <por@oranjevos.nl> wrote: > >> Suggestion: log of this unexpected condition or even stronger, an >> assert(). > > All logging is killed already, logging just to serial console won't cut it, assert is overkill, which needs compiler flags/ifdef fiddling and who knows how that would behave in different environments. Okay, did not think about the impact of this stdclib call. > I could split it into separate functions if that implicit return would be more acceptable for you. It just seems overkill to me, just to make both of us satisfied :-) Right, it's not really worth the trouble, it's just kind of aesthetic thing. > >> Decorating perform_halt() with the _Noreturn function specifier might > > Ok, I like this one. >
diff --git a/state.c b/state.c index 4737d0121ad0..e117ea302f93 100644 --- a/state.c +++ b/state.c @@ -94,6 +94,34 @@ static void set_console(void) set_stdio(tty); } +static void perform_halt() +{ + if (reboot_event == RB_POWER_OFF) + LOG("- power down -\n"); + else + LOG("- reboot -\n"); + + /* Allow time for last message to reach serial console, etc */ + sleep(1); + + if (is_container()) { + reboot(reboot_event); + exit(EXIT_SUCCESS); + return; + } + + /* We have to fork here, since the kernel calls do_exit(EXIT_SUCCESS) + * in linux/kernel/sys.c, which can cause the machine to panic when + * the init process exits... */ + if (!vfork()) { /* child */ + reboot(reboot_event); + _exit(EXIT_SUCCESS); + } + + while (1) + sleep(1); +} + static void state_enter(void) { char ubus_cmd[] = "/sbin/ubusd"; @@ -153,29 +181,9 @@ static void state_enter(void) sync(); sleep(1); #ifndef DISABLE_INIT - if (reboot_event == RB_POWER_OFF) - LOG("- power down -\n"); - else - LOG("- reboot -\n"); - - if (!is_container()) { - /* Allow time for last message to reach serial console, etc */ - sleep(1); - - /* We have to fork here, since the kernel calls do_exit(EXIT_SUCCESS) - * in linux/kernel/sys.c, which can cause the machine to panic when - * the init process exits... */ - if (!vfork( )) { /* child */ - reboot(reboot_event); - _exit(EXIT_SUCCESS); - } - - while (1) - sleep(1); - } else - exit(0); + perform_halt(); #else - exit(0); + exit(EXIT_SUCCESS); #endif break;
Executing `reboot` command in OpenWrt system runing inside LXC container results in a shutdown of the container instead of rebooting the container. This appears to have been caused by commit 832369078d81 ("state: fix shutdown when running in a container (FS#2425)"), which exits the pid einz instead of the reboot(). While at it, refactor the halting code into separate function to shorten the switch/case block and make it clearer, decrease the indentation level by reversing the container if condition, replace magic 0 with EXIT_SUCCESS constant in exit() and make it wait 1s for reboot message delivery in both container/host cases as well. Ref: FS#2666 Cc: Paul Spooren <mail@aparcar.org> Fixes: 832369078d81 ("state: fix shutdown when running in a container (FS#2425)") Tested-by: Baptiste Jonglez <lede@bitsofnetworks.org> Signed-off-by: Petr Štetiar <ynezz@true.cz> --- state.c | 52 ++++++++++++++++++++++++++++++---------------------- 1 file changed, 30 insertions(+), 22 deletions(-)