[OpenWrt-Devel] [PATCH procd 2/2] state: fix reboot causing shutdown inside LXC container

[Paul Oranje]

Op 23 jan. 2020, om 10:28 heeft Petr Štetiar <ynezz at true.cz> het volgende geschreven:
> 
> Paul Oranje <por at 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
_______________________________________________
openwrt-devel mailing list
openwrt-devel at lists.openwrt.org
https://lists.openwrt.org/mailman/listinfo/openwrt-devel