Page MenuHomeFreeBSD

Add new rc keywords: enable, disable, delete
ClosedPublic

Authored by dteske on Sep 10 2018, 7:46 PM.

Details

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

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

lme created this revision.Sep 10 2018, 7:46 PM
bjk added a subscriber: bjk.Sep 10 2018, 8:15 PM

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 added a comment.Sep 10 2018, 8:27 PM

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

lme updated this revision to Diff 47896.Sep 10 2018, 8:37 PM

Implement bjk's suggestions.

dteske requested changes to this revision.Sep 11 2018, 9:58 PM
dteske added inline comments.
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

This revision now requires changes to proceed.Sep 11 2018, 9:58 PM

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 requested changes to this revision.Sep 11 2018, 10:15 PM
0mp added a subscriber: 0mp.
0mp added inline comments.
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 added inline comments.Sep 18 2018, 11:13 PM
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 added inline comments.Sep 18 2018, 11:29 PM
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 commandeered this revision.Sep 19 2018, 12:34 AM
dteske edited reviewers, added: lme; removed: dteske.

Take

dteske updated this revision to Diff 48197.Sep 19 2018, 12:35 AM

Provide a new offering

dteske edited the summary of this revision. (Show Details)Sep 19 2018, 12:39 AM
dteske edited the test plan for this revision. (Show Details)
dteske retitled this revision from Add new rc keywords: enable, disable, rcdelete to Add new rc keywords: enable, disable, delete.Sep 19 2018, 1:38 AM
lme accepted this revision.Sep 24 2018, 4:12 PM

I am Fine with your version, Devin. Thanks!

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 added a comment.Sep 25 2018, 9:19 AM

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 requested changes to this revision.Sep 25 2018, 9:30 AM

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)?

This revision now requires changes to proceed.Sep 25 2018, 9:30 AM

Thanks @lme for checking for potential conflict.

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

dteske updated this revision to Diff 49572.Oct 24 2018, 5:56 PM

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

lme accepted this revision.Oct 31 2018, 6:46 PM

Any chance to get this into HEAD soon?

bcr accepted this revision.Oct 31 2018, 6:59 PM
bcr added a subscriber: bcr.

Still OK from manpages.

This revision was not accepted when it landed; it landed in state Needs Review.Oct 31 2018, 8:37 PM
This revision was automatically updated to reflect the committed changes.