rc.d/sendmail: Return non-zero if the daemon fails to start or is not running

[0mp]

Summary

If you have a mail server that is running sendmail daemon
(sendmail_enable=YES) and sendmail queue runner (sendmail_msp_queue=YES)
and the sendmai daemon dies, /etc/rc.d/sendmail status does see the
daemon is not running but returns 0 as the exit code. This prevents
other programs (like puppet) from restarting sendmail to fix the issue.
If the sendmail_msp_queue is not running, it will return non zero as the
exit code.

PR: 223132

---

https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=223132

[0mp]

This is a stopgap. A proper fix would require a rewrite of this service script and possibly a split of the different functionalities of this script into separate files.

[0mp]

I think this patch is a fair workaround for the issues reported in https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=223132.

What is unclear to me is whether we should expand this patch to preserve the exit code of sendmail_submit and sendmail_outbound.

[christos]

LGTM. Only a minor nit: in the commit message there is a typo: "sendmai daemon ...".

[markj]

libexec/rc/rc.d/sendmail

235

Should these be $_ret?

This should have some comment explaining what is going on.

[christos]

libexec/rc/rc.d/sendmail

235

I think you only specify the variable name without $ if it's inside $((...)).

[christos]

libexec/rc/rc.d/sendmail

235

I see that we use both styles in the tree.

[0mp]

libexec/rc/rc.d/sendmail

235

It is recommended by shellcheck to let $(()) expand variables on its own. $? has to be used as a variable in this case, however, because just ? has a different meaning in this context.

[0mp]

libexec/rc/rc.d/sendmail

235

Both $(($foobar)) and $((foobar)) is valid sh.

[gshapiro]

This is an improvement over the current functionality, but I think we would want all invocations to be checked. I wonder if it would be better to separate these into separate rc scripts instead of using one for four possible different daemons. That shouldn't prevent this change from going forward while alternatives are formulated.

[0mp]

In D46862#1076197, @gshapiro wrote:

This is an improvement over the current functionality, but I think we would want all invocations to be checked. I wonder if it would be better to separate these into separate rc scripts instead of using one for four possible different daemons. That shouldn't prevent this change from going forward while alternatives are formulated.

I absolutely agree that this patch is imperfect.

I am happy that you don't see an problem with splitting the script into a few independent scripts. I'll make sure to ping you if I end up rewriting rc.d/sendmail.

Thanks!

[0mp]

Check all invocations.

[markj]

libexec/rc/rc.d/sendmail

236

Are you sure that rc scripts are always run in a separate shell? I have some vague notion that it's possible to source them, and maybe there are some modes where rc or service do that. Maybe I'm just completely wrong?

[0mp]

libexec/rc/rc.d/sendmail

236

This is a good question.

Here's a fragment of run_rc_script():

			if [ -n "$rc_boottrace" ]; then
				boottrace_fn "$_file" "$_arg"
			elif [ -n "$rc_fast_and_loose" ]; then
				set $_arg; . $_file
			else
				( trap "echo Script $_file interrupted >&2 ; kill -QUIT $$" 3
				  trap "echo Script $_file interrupted >&2 ; exit 1" 2
				  trap "echo Script $_file running >&2" 29
				  set $_arg; . $_file )
			fi

rc_fast_and_loose seems to just source everything into the current shell. It might be that other programs like service(8) do something similar. I've not checked.

However, a quick grep over rc.d for exit reveals that rc scripts use exit a lot. My first thought is that rc_fast_and_loose and similar use cases are just not really working at the moment.

[0mp]

libexec/rc/rc.d/sendmail

236

For reference, service(8) only handles one service script at a time so it does not care about exits in those scripts.

[markj]

libexec/rc/rc.d/sendmail

236

Perhaps we should just leave the exit status set. Then, if the script is run in its own shell, it'll exit with the correct status, and if it is sourced into an existing shell, it will not trigger an exit.

Alternately, perhaps rc_fast_and_loose is broken and should be removed. I'm not sure.

[0mp]

libexec/rc/rc.d/sendmail

236

If I understand correctly, setting the exit status without calling exiting the whole script could be done as follows:

(exit "$_ret")

instead of

exit "$_ret"

This way we exit the subshell but still set the exit status as long as (exit ...) is the last command in the script.

As a side note: I do believe that rc_fast_and_loose is broken currently.

[markj]

libexec/rc/rc.d/sendmail

236

I think that should work, yes.

I don't insist on doing that. As you say, there are other scripts that invoke exit already.

I would just suggest giving this change a long MFC timeout, so that there's time to catch bugs before the change appears on a stable branch (if at all).

[0mp]

libexec/rc/rc.d/sendmail

236

Here's a related thread from 2012 which suggests that rc_fast_and_loose is broken: https://lists.freebsd.org/pipermail/freebsd-current/2012-January/031167.html

Also, I think we can use return 1 instead of (exit 1) actually. It's just that calling return outside of a function is not supported by POSIX I think. At the same time, rc scripts are very FreeBSD-specific and our sh(1) supports calling return like that.

[markj]

libexec/rc/rc.d/sendmail

236

I'd rather not rely on non-POSIX sh behaviour here, unless there's already a lot of precedent for that in existing rc scripts.

[0mp]

I opened a PR for investigating the rc_fast_and_loose support: https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=282255

[0mp]

Set exit status by calling exit in a subshell.

[0mp]

I'll set MFC to 2 weeks so that it does not land in 14.2.

[markj]

Approved.

[0mp]

Thanks!