Bug 216115 - service -e showing sendmail enabled when it is not
Summary: service -e showing sendmail enabled when it is not
Status: Open
Alias: None
Product: Base System
Classification: Unclassified
Component: conf (show other bugs)
Version: 11.0-RELEASE
Hardware: amd64 Any
: --- Affects Only Me
Assignee: freebsd-rc (Nobody)
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-01-15 15:41 UTC by Jason Mader
Modified: 2023-01-19 07:14 UTC (History)
4 users (show)

See Also:


Attachments
Fix service -e (488 bytes, text/plain)
2017-10-04 04:12 UTC, Alex Kozlov
no flags Details
Fix startup scripts (5.81 KB, text/plain)
2017-10-04 04:13 UTC, Alex Kozlov
no flags Details
rc-scripts checker (2.33 KB, text/plain)
2018-10-09 16:11 UTC, Alex Kozlov
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Jason Mader 2017-01-15 15:41:38 UTC
The output of `service -e` shows `/etc/rc.d/sendmail` enabled even when /etc/rc.conf has `sendmail_enable="NONE"`
Comment 1 Jason Mader 2017-01-15 17:54:34 UTC
root: /usr/sbin/service: WARNING: $growfs_enable is not set properly - see rc.conf(5).
root: /usr/sbin/service: WARNING: $ is not set properly - see rc.conf(5).
root: /usr/sbin/service: WARNING: $rsyncd_enable is not set properly - see rc.conf(5).
Comment 2 Jonas Palm 2017-05-30 13:39:12 UTC
I have the same problem on multiple servers. This seems to be some regression bug. Those servers that are still running 11.0-p3 don't show sendmail while 11.0-p8 and above do show it. Sendmail is disabled on all our servers via sendmail_enable="NONE" in /etc/defaults/vendor.conf
Comment 3 Jonas Palm 2017-05-30 13:51:55 UTC
(In reply to Jonas Palm from comment #2)
Sorry, the regression part was nonsense. Theses servers that don't show sendmail are rather old and also got an old config with all the sendmail parts disabled individually with *_enable="NO".
Comment 4 Chris Rees freebsd_committer freebsd_triage 2017-08-30 13:12:38 UTC
I can't reproduce, I'm sorry.

[crees@pegasus]~/ntpd_flags% service -e | grep sendmail
[crees@pegasus]~/ntpd_flags% grep ^sendmail /etc/rc.conf
sendmail_enable="NONE"
[crees@pegasus]~/ntpd_flags%


Please would you try this to help debug?  Open a Bourne shell (sh) somewhere and run these commands, and give the output.

$ . /etc/rc.subr
$ load_rc_config sendmail
$ echo $sendmail_enable
Comment 5 Jason Mader 2017-08-30 14:59:21 UTC
(In reply to Chris Rees from comment #4)

# sh
# . /etc/rc.subr
# load_rc_config sendmail
# echo $sendmail_enable
NONE
# grep sendmail /etc/rc.conf
sendmail_enable="NONE"
# service -e
/etc/rc.d/hostid
/etc/rc.d/zvol
/etc/rc.d/hostid_save
/etc/rc.d/zfs
/etc/rc.d/cleanvar
/etc/rc.d/ip6addrctl
/etc/rc.d/netif
/etc/rc.d/ipsec
/etc/rc.d/ipfw
/etc/rc.d/newsyslog
/etc/rc.d/syslogd
/etc/rc.d/savecore
/etc/rc.d/dmesg
/etc/rc.d/nfsuserd
/etc/rc.d/nfsd
/etc/rc.d/virecover
/etc/rc.d/motd
/etc/rc.d/ntpd
/etc/rc.d/rctl
/etc/rc.d/sshd
/etc/rc.d/sendmail
/etc/rc.d/cron
/etc/rc.d/mixer
/etc/rc.d/gptboot
/etc/rc.d/bgfsck

11.0-RELEASE-p12
Comment 6 Alex Kozlov freebsd_committer freebsd_triage 2017-10-01 06:13:29 UTC
/etc/rc.subr checks:
933 if [ -n "$_pidcmd" ]; then
934    _keywords="${_keywords} status poll"
935 fi

but a few lines above $_pidcmd set to:
928 if [ -n "$pidfile" ]; then
929    _pidcmd='rc_pid=$(check_pidfile '"$pidfile $_procname $command_interpreter"')'
930 else
931    _pidcmd='rc_pid=$(check_process '"$_procname $command_interpreter"')'
fi

so line 933 condition is always true:
[ -n 'rc_pid=$(check_pidfile /var/run/sendmail.pid /usr/sbin/sendmail )' ]

and status and poll commands are always added to keywords.

This should help:
Index: /etc/rc.subr

@@ -930,7 +930,7 @@
                else
                        _pidcmd='rc_pid=$(check_process '"$_procname $command_interpreter"')'
                fi
-               if [ -n "$_pidcmd" ]; then
+               if [ -n "$(eval "$_pidcmd")" ]; then
                        _keywords="${_keywords} status poll"
                fi
        fi
Comment 7 Alex Kozlov freebsd_committer freebsd_triage 2017-10-01 06:30:50 UTC
Proper, actually tested patch:

Index: /etc/rc.subr
@@ -930,7 +930,8 @@
                else
                        _pidcmd='rc_pid=$(check_process '"$_procname $command_interpreter"')'
                fi
-               if [ -n "$_pidcmd" ]; then
+               eval "$_pidcmd"
+               if [ -n "$rc_pid" ]; then
                        _keywords="${_keywords} status poll"
                fi
        fi
Comment 8 Chris Rees freebsd_committer freebsd_triage 2017-10-01 06:44:56 UTC
Great, I think that's correct!

One thing, no need for the quotes in the eval statement (in fact they're incorrect).

Hopefully someone will give you approval to commit that.
Comment 9 Jilles Tjoelker freebsd_committer freebsd_triage 2017-10-03 22:13:37 UTC
The proposed patch basically executes status when executing any subcommand, and then adds status and poll to the available subcommands when the daemon is running. This seems wrong.

Although the if [ -n "$_pidcmd" ]; then part is definitely redundant, I think this condition should just be removed instead of changed. The idea is that _pidcmd is defined and status and poll are available iff a command or process name is defined (which may be done in various ways).

What happens with /etc/rc.d/sendmail is that service(8)'s naive greps catch the lines

name="sendmail_msp_queue"
rcvar="sendmail_msp_queue_enable"

in addition to the intended lines

name="sendmail"
rcvar="sendmail_enable"

and the msp_queue ones override the intended lines because they are last.

Indenting the two msp_queue lines in /etc/rc.d/sendmail fixes the problem but it is a rather ugly and obscure solution.
Comment 10 Chris Rees freebsd_committer freebsd_triage 2017-10-03 22:22:35 UTC
Oh, yuck, you're indeed correct about the multiple definitions.

Could we pipe the evil grep to head -n 1?

Chris
Comment 11 Chris Rees freebsd_committer freebsd_triage 2017-10-03 22:22:56 UTC
Obviously meant eval, not evil :)
Comment 12 Alex Kozlov freebsd_committer freebsd_triage 2017-10-03 22:29:14 UTC
Yes, it's leftover of (mis)merge from NetBSD in r98186, instead of 

else 
   _pidcmd='rc_pid=$(check_process '"$_procname $command_interpreter"')'

previously it was

elif [ -n "$command" ]; then
   _pidcmd='_pid=`check_process '$command'`'

so
if [ -n "$_pidcmd" ]; then

kinda makes sense.

And you're right, sendmail rc script setting up pidfile vatiable, so it should get status and poll commands, but because it setting more than one rcvar, service is confused.

It's not that easy to properly fix service, unfortunately.
Comment 13 Chris Rees freebsd_committer freebsd_triage 2017-10-03 22:46:11 UTC
(In reply to Alex Kozlov from comment #12)

Sure it is!  Just take the first result from grep ^name= using head...

Oh yeah, don't forget rcvar too.

Chris
Comment 14 Alex Kozlov freebsd_committer freebsd_triage 2017-10-04 04:12:48 UTC
Created attachment 186895 [details]
Fix service -e
Comment 15 Alex Kozlov freebsd_committer freebsd_triage 2017-10-04 04:13:33 UTC
Created attachment 186896 [details]
Fix startup scripts
Comment 16 Alex Kozlov freebsd_committer freebsd_triage 2017-10-04 04:14:20 UTC
I've used enabled command, it's more robust and faster (see attach). But before it would work properly a few startup scripts needs to be fixed (see second attach). Fix is a pretty trivial, just need to wrap startup code in the start_cmd function. What I'm really not sure about is what's to do with rc.d/
serial, it's more like example or do it yourself template than startup script.
Perhaps it's better to move it to share/examples or just delete.
Comment 17 Jilles Tjoelker freebsd_committer freebsd_triage 2017-10-22 22:13:53 UTC
I suppose using 'enabled' is better in the end, but how about old scripts that cause 'service -e' to do strange things?

I think /etc/rc.d/serial can be fixed like other scripts. Perhaps leave the indentation wrong to make merging easier.

Using the first result from 'grep ^name=' seems rather arbitrary; it does not seem definitely better than using the last result, except for how it works on /etc/rc.d/sendmail.
Comment 18 Eitan Adler freebsd_committer freebsd_triage 2018-05-28 19:41:28 UTC
batch change:

For bugs that match the following
-  Status Is In progress 
AND
- Untouched since 2018-01-01.
AND
- Affects Base System OR Documentation

DO:

Reset to open status.


Note:
I did a quick pass but if you are getting this email it might be worthwhile to double check to see if this bug ought to be closed.
Comment 19 Alex Kozlov freebsd_committer freebsd_triage 2018-10-07 12:12:04 UTC
Apology for a very late response. It seems that I've somehow missed last message.
I think rc scripts in the base can be fixed immediately and fixes to service should be postponed until (most) of the ports' scripts would be checked/fixed. Good tool for this seems to be devel/rclint.
Comment 20 Alex Kozlov freebsd_committer freebsd_triage 2018-10-09 16:11:26 UTC
Created attachment 197968 [details]
rc-scripts checker
Comment 21 Alex Kozlov freebsd_committer freebsd_triage 2018-10-09 16:13:17 UTC
Alright, I'm too lazy to grok devel/rclint, so I hacked together crude checker in shell.
There're a few false positives, but even so from 1341 rc-scripts more than 200 have code outside of command functions :-(

But only a few broken with service enabled command:
* audio/madfufw - exit(0) if /var/run/maudio.pid exist or creates that file if it's not
* net/frr3, net/frr4, net/frr5 - preprocesses rc-commands, 'enabled' always return no (rc=1)
* net/kafka - unconditionally creates ${kafka_log_dir}/${kafka_out_file} if its doesn't exist
* sysutils/ipad_charge - exit if less than two command line arguments given
* sysutils/logstash5 - unconditionally creates piddir if it's exist (inverted condition?)
* www/gitlab-ce - tries to recreate Gemfile.lock in certain conditions
* sysutils/p5-Tail-Stat, www/nginx, www/nginx-devel, www/uwsgi - uses hack to run multiple instances from on rc-script, always retun rc=0, so 'enabled' always return yes