Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

We do not go through STOP_POST and for the situations where we do, we do not set the variables we say we will #4770

Closed
evverx opened this issue Nov 29, 2016 · 3 comments · Fixed by #4843
Labels
bug 🐛 Programming errors, that need preferential fixing pid1

Comments

@evverx
Copy link
Member

evverx commented Nov 29, 2016

Based on #4745 (comment)

So we do not go through STOP_POST on service_enter_signal(s, SERVICE_FINAL_SIGTERM, SERVICE_FAILURE_PROTOCOL) and for the situations where we do, we do not set the variables we say we will.
Indeed, this is a divergence from the documentation. However, this divergence was not introduced by the addition of the protocol error type and was already present with the resources error type. I think we should merge this PR as it currently is and start a new issue for the discrepancy between the manpage and the actual fail behavior. This new issue should also address the lack of a line for SERVICE_RESULT=protocol in the table at https://www.freedesktop.org/software/systemd/man/systemd.exec#$EXIT_CODE.

@evverx evverx added bug 🐛 Programming errors, that need preferential fixing pid1 labels Nov 29, 2016
@joukewitteveen
Copy link
Contributor

This problem is manifest in several places in src/core/service.c. We currently use service_enter_signal(s, SERVICE_FINAL_SIGTERM, f), where f != SERVICE_SUCCESS in case of failure, but this does not cause a pass through STOP_POST.

  • service_enter_running (since 3d474ef we fail services with RemainAfterExit=yes when their main process terminates unsuccessfully)
  • service_sigchld_event
  • service_notify_cgroup_empty_event
  • service_dispatch_timer (Warning: we call service_enter_signal(s, SERVICE_FINAL_SIGTERM, SERVICE_FAILURE_TIMEOUT) when stop-post times out, but not only in that case)
  • service_enter_stop_post itself
  • service_enter_start
  • service_run_next_control

Do we mean SERVICE_STOP_SIGTERM in some of these cases, or do we need SERVICE_FAIL_SIGTERM that acts like SERVICE_FINAL_SIGTERM, but passes through service_enter_stop_post?

As for the setting of $EXIT_STATUS and $EXIT_CODE, the documentation glosses over a corner case: it is possible that there has been no main process, so there is no meaningful value for these variables. We could make this more explicit by adding a new value for EXIT_STATUS, namely "n.a.". We could also just document that it can happen, and is meaningful, that these variables are not set.

Reminder:

  • Add the row for "protocol" errors when this is resolved (either "exited"/"0", or "n.a."/not set)

@evverx
Copy link
Member Author

evverx commented Dec 1, 2016

@joukewitteveen , oh, that's a great summary. Thanks!

I'll try to reread #3818

I'm not sure I remember what is the purpose of the $EXIT_CODE, $EXIT_STATUS and $SERVICE_RESULT

@joukewitteveen
Copy link
Contributor

And of course #1254, where @poettering implies that we might indeed want SERVICE_STOP_SIGTERM instead of SERVICE_FINAL_SIGTERM.

I am not entirely sure why we use service_enter_signal from service_notify_cgroup_empty_event, since there shouldn't be anything to kill anyway in that situation.

joukewitteveen added a commit to joukewitteveen/systemd that referenced this issue Dec 6, 2016
poettering added a commit that referenced this issue Dec 7, 2016
Go through stop_post on failure (#4770)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Programming errors, that need preferential fixing pid1
Development

Successfully merging a pull request may close this issue.

2 participants