Add new rc keywords: enable, disable, delete

[dteske]

Summary

This adds new keywords to rc / service to enable or disable a services's rc.conf(5) variable and "delete" to remove the variable.

When the "service_delete_empty" variable in rc.conf is set to "YES" (default is "NO") an empty rc.conf.d file (in /etc/ or /usr/local/etc) is deleted if modified after issuing "service $foo delete".

Test Plan

service sshd enable

sshd enabled in /etc/rc.conf

service sshd disable

sshd disabled in /etc/rc.conf

service sshd delete

sshd_enable deleted in /etc/rc.conf

1. sysrc service_delete_empty=YES
2. /etc/rc.d/moused enable

moused enabled in /etc/rc.conf.d/moused

/etc/rc.d/moused disable

moused disabled in /etc/rc.conf.d/moused

service moused delete

moused_enable deleted in /etc/rc.conf.d/moused
Empty file /etc/rc.conf.d/moused removed

[bjk]

The man page should get a bunch more markup; off the top of my head,
.Xr for man page references,
.Pa for filesystem paths,
Li for literal variables and values...

[lme]

FYI, this is a rewrite of https://reviews.freebsd.org/D451 which lost its attachment some time ago.

[lme]

Implement bjk's suggestions.

[dteske]

sbin/init/rc.conf

709–710 ↗

(On Diff #47896)

This shouldn't be optional as its use at boot is not optional. sysrc does the right thing if you use the -s name option to tell it which service you are talking about. If no rc.conf.d files exist, none are touched -- but an admin creates an rc.conf.d file for the service, boot will adhere to it and thus service should let you operate on what boot will adhere-to.

Having this as optional and having the default set to no, it would cause boot and sysrc (which agree with each other) to disagree with service and that violates POLA.

sbin/init/rc.subr

925 ↗

(On Diff #47896)

I would think "rcdelete" should just be "delete".

The action is being passed to the "service" utility and it should be obvious that you're not deleting software off disk (which would be the job of "pkg", not "service").

Drawing a comparison to Linux, admins are taught to use "chkconfig" to manage the runlevels. Well FreeBSD has neither runlevels nor chkconfig (they simply don't apply), and instead we have here a patch that makes FreeBSD "service" like a Linux "service" and "chkconfig" combined. I like this idea. It's a good idea. However, for consistency, I would think "delete" would be fine and you can remove the "rc" prefix as it looks out of place.

1009–1014 ↗

(On Diff #47896)

This code is incorrect. It neglects /etc/rc.conf.local, /usr/local/etc/rc.conf, and /usr/local/etc/rc.conf.d. It also ignores the -s name argument to sysrc to enable *proper* access to rc.conf.d.

This code segment is a blocker.

1043–1044 ↗

(On Diff #47896)

Combine these two lines. For example, the current stanza:

/usr/sbin/sysrc "$rcvar=YES" > /dev/null
[ $? -eq 0 ] &&

Becomes:

/usr/sbin/sysrc "$rcvar=YES" > /dev/null &&

1045 ↗

(On Diff #47896)

Don't hard-code /etc/rc.conf. If you use the -v flag to sysrc it tells you which file it made the change in. You would have to capture the output and parse the value. For example, say sshd_enable=YES is in /etc/rc.conf.local:

dteske@vm11 ~ $ cat /etc/rc.conf.local
foo="1"
dteske@vm11 ~ $ sr sysrc -v foo=2
/etc/rc.conf.local: foo: 1 -> 2
dteske@vm11 ~ $ cat /etc/rc.conf.local
foo="2"

An example of capturing the output would be:

output=$( /usr/sbin/sysrc "$rcvar=NO" ) &&
        echo "$name disabled in ${output%%:*}"

Though, while we're here, it's worth noting that since the first argument starts with a variable, you should add "--" as an option terminator to make it clear, even if the supplied $rcvar value starts with "-", that the argument is an assignment operation and not an option flag. For example:

output=$( /usr/sbin/sysrc -- "$rcvar=NO" ) &&
        echo "$name disabled in ${output%%:*}"

1049–1051 ↗

(On Diff #47896)

See notes on lines 1043-1045

1055–1060 ↗

(On Diff #47896)

See notes on lines 1043-1045

1056–1058 ↗

(On Diff #47896)

I will fix sysrc after head-thaw (Sep 23, 2018). You can remove this comment. PR welcome if we need a reminder to revisit it after commits are open on head again.

1064–1066 ↗

(On Diff #47896)

See notes on lines 1043-1045

1070–1072 ↗

(On Diff #47896)

See notes on lines 1043-1045

[dteske]

This patch is a good effort. Let's keep building upon it and get it ready for when head is thawed on Sep 23. Thank you for resurrecting this topic.

[0mp]

share/man/man8/rc.8

34 ↗

(On Diff #47896)

Remember to bump the date.

329 ↗

(On Diff #47896)

I think that .Li is rarely used like this. Usually, it is used together with Dq like this:

.Dq Li content

331 ↗

(On Diff #47896)

I think that .Li is rarely used like this. Usually, it is used together with Dq like this:

.Dq Li content

335 ↗

(On Diff #47896)

Missing dot at the end of the sentence

[dteske]

sbin/init/rc.subr

1042–1075 ↗

(On Diff #47896)

I just noticed that enable, disable, and rcdelete are duplicated in the same case statement. The first matching pattern in a case statement is the one executed and thus the later [duplicate] patterns will not fire. For example:

case foo in
foo) echo 1;;
foo) echo 2
esac

Results in "1",

[dteske]

sbin/init/rc.subr

1056–1058 ↗

(On Diff #47896)

Already fixed under 11.2.

dteske@vm11 init $ uname -spr
FreeBSD 11.2-STABLE amd64
dteske@vm11 init $ sudo sysrc -x foogribble
[sudo] Password:
sysrc: unknown variable 'foogribble'
dteske@vm11 init $ echo $?
1

In the code (usr.sbin/bsdconfig/share/sysrc.subr):

...
# This function is a two-parter. Below is the awk(1) portion of the function,
# afterward is the sh(1) function which utilizes the below awk script.
#
f_sysrc_delete_awk='
# Variables that should be defined on the invocation line:
#       -v varname="varname"
#
BEGIN {
        regex = "^[[:space:]]*"varname"="
        found = 0
}
{
        if ( $0 ~ regex )
                found = 1
        else
                print
}
END { exit ! found }
'
...

Emphasis on the exit !found

[dteske]

Take

[dteske]

Provide a new offering

[lme]

I am Fine with your version, Devin. Thanks!

[dteske]

I just thought of something ...

Should we do a check of the entire ports tree to determine if there is some percentage of existing rc.d scripts that already make use of "enable", "disable" or "delete" in the form of $extra_commands handling? In other words, what's the likelihood we might conflict with existing adhoc implementations of these new features?

Might not be hard to do a quick scan of ports containing rc.d scripts (I think we would only need to check two places, the Makefile and packing list) and amongst those containing rc.d scripts which ones match a grep for '\<\(enable\|delete\|disable\)\>' and then checking each one for conflict.

Is this worth pursuing? What happens if we find a conflict?

[lme]

I ran this in /usr/ports and it didn't give me any output, so I think we're safe:

find . -name "*.in" -type f -exec egrep -H 'extra_commands' {} +|egrep 'enable|disable|delete'

[0mp]

The rc.8 manpage changes look fine.

sbin/init/rc.conf

619 ↗

(On Diff #48197)

Also, shouldn't service_delete_empty be documented in rc.conf(5)?

[dteske]

Thanks @lme for checking for potential conflict.

Good catch @0mp, I'll update rc.conf and then update this diff.

[dteske]

Rebase following r339413 and add patch for rc.conf(5) manual

[lme]

Any chance to get this into HEAD soon?

[bcr]

Still OK from manpages.