cmd: unify die() across C programs #7362
Conversation
When die() dies, it doesn't give us much choice as to how to go. While reviewing a pull request snapcore#7358 we realized that system-shutdown was defining die twice, once from libsnap-confine-private, using the regular die-and-exit code, and once internally with custom die-with-reboot. This isn't great. We should just have one function and allow a process to say how it wants to customize death. This is what sc_panic() is. It's a drop-in replacement to die() (in fact, die will be a thin wrapper around it). On top of die it offers a pair of customization points: one for how to exit (e.g. you can call reboot) and one for how to say something failed (you can print to whatever is appropriate, e.g. to a file). This patch contains just the new panic function along with documentation and tests. Signed-off-by: Zygmunt Krynicki <me@zygoon.pl>
The die() function is our common print-and-quit helper. With the introduction of sc_panic() we have a flexible way to achieve that. Since die() is so widespread it's easier to make it a wrapper for sc_panic(). This patch does just that. Signed-off-by: Zygmunt Krynicki <me@zygoon.pl>
Now that libsnap-confine-private offers a way to customize the behavior of die() we can drop or implementation and configure the behavior of sc_panic(), that backs the implementation of die() Signed-off-by: Zygmunt Krynicki <me@zygoon.pl>
| static sc_panic_exit_fn panic_exit_fn = NULL; | ||
| static sc_panic_msg_fn panic_msg_fn = NULL; | ||
|
|
||
| void sc_panic(const char *fmt, ...) { |
There was a problem hiding this comment.
Quick drive-by: maybe some else than "panic"? Panic smells a lot like a go panic() which will behave differently AFAICT (print stacktrace etc).
There was a problem hiding this comment.
Well, the fact that it prints a stack trace isn't the essential feature. We could also print one if that would make you feel better. I'm somewhat -1 on this precisely because of error + panic parity that C and Go can have.
There was a problem hiding this comment.
I'm not arguing in favour of adding a stacktrace to the C code. Just that its confusing to use the same name for something different without any advantage in doing so (at least I don't see one).
There was a problem hiding this comment.
I argue that the principal function of sc_panic is the same as panic, to abort execution without retaining regular control flow.
|
Thanks for the PR - the solution looks interesting. But I think its likely that the shutdown utility will go away with UC20 and its also really rarely touched (last real change ~1y ago). So I would prefer if we would go with the simpler fix in #7358. I think the use-case(s) we have does not justfiy the extra code/API. |
|
@mvo5 that fix is incorrect, we should reboot, not exit in |
|
One critical piece of information that I was missing when I suggested to go with 7358 is that the helpers in "libsnapconfine-private" may call "die()" which will cause a program exit. But when die() is called in the context of system-shutdown we need it to behave differently. So I need to recheck this with fresh eyes - sorry that I did not realize this before. |
cmd/system-shutdown: rename sync_and_reboot to sync_and_halt
cmd: unify die() across C programs
Historically we inherited die() from ubuntu-core-launcher. It underwent a few
small changes but otherwise retained its function for print-and-quit helper.
This was true until system-shutdown was first implemented. There die existed as
a function with the same purpose but different implementation. The changes were
motivated by desire to fit the environment in which system-shutdown executes:
it runs as the last PID of the system and should not just exit. Therefore the
implementation called sync and reboot. The output of the message was also
formatted differently to fit other messages printed by the system at the same
time.
At some point, system-shutdown started to use all of libsnap-confine-private,
not just a single module. This presented a problem because now die() was
defined twice. Oddly, this went unnoticed until
#7358 was opened, trying to resolve the
problem.
After some inspection I decided to implement the following solution. There is
only one die(), it always does what it says on the tin. Die is now implemented
with a new function, sc_panic(), which is just like die but can be tweaked in
two ways: the way the error is printed and the way the process is exited. This
is exactly what system-shutdown needed to change.
The panic module comes with simple tests and documentation.