rc.subr uses sysrc(8) for the 'enable' and 'disable' commands, which
means the entire rc(8) stack depends on bsdconfig. instead, provide a
minimal amount of rc.conf-editing functionality in rc.subr and use it to
implement these commands.
Details
Details
- Reviewers
des kevans bapt - Group Reviewers
pkgbase rc - Commits
- rG7d88b0e3dbe4: rc.subr: remove the dependency on bsdconfig
rGf6328f052518: rc.subr: remove the dependency on bsdconfig
Diff Detail
Diff Detail
- Repository
- rG FreeBSD src repository
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
Comment Actions
although i don't think rc.subr should depend on bsdconfig in general, the real purpose of this change is to let us move bsdconfig out of the FreeBSD-utilities package in pkgbase.
| libexec/rc/rc.subr | ||
|---|---|---|
| 2743 | It bothers me that both /etc/rc.conf and /etc/defaults/rc.conf are hardcoded in several places in rc.subr. Perhaps it's time to define _rc_conf and _defaults_rc_conf at the top of the file. Maybe _rc_conf_d as well. | |
| 2745 | purely a matter of taste: this intermediate assignment is not really needed | |
| 2746 | Some rc scripts have multiple names; for instance, /etc/rc.d/netif calls load_rc_config network before load_rc_config netif. You might want to cache the names passed to load_rc_config somewhere so you can iterate over them here. | |
| 2749 | This entire loop can be rewritten as grep -rl "^${_var}=" "$_files", although you'll have to think about what to do if you get multiple hits. Replacing just the first one (as you currently do) may turn out to have no effect at all. (you may get false positives since e.g. /etc/rc.conf.d/${name}/foo/bar will be visible to grep but not to load_rc_config; whether or not that is a problem is up for debate) | |
| 2784 | Have you tested this with a value that contains a /? | |
| 2787 | I would argue that if /etc/rc.conf.d/${name} exists, then we should put the variable there rather than in /etc/rc.conf. | |
| libexec/rc/rc.subr | ||
|---|---|---|
| 2784 | right now the value can only be YES or NO, but i suppose it would be useful to support arbitrary values here since the function isn't documented to only work for _enable vars. | |
| 2787 | i'm not opposed to this, but it's not how it currently works - are we okay with changing the behaviour? th documentation in rc(8) just says "Enable the service in rc.conf(5)", so we haven't previously committed to editing a particular file here. | |
Comment Actions
review fixes
i haven't made any cleanup changes here (like adding more variables for
settings) because there's quite a few of those we could do and i think it makes
more sense to do that in a follow-up commit.
Comment Actions
Only nits left, up to you if you want to leave them as is or do a final round.
| libexec/rc/rc.subr | ||
|---|---|---|
| 2737 | nit: you can declare multiple local variables in a single statement. | |
| 2761 | nit: you can declare multiple local variables in a single statement. | |
| 2770 | this needs to be local | |
| 2778 | for bonus points, if $_file exists but is a directory, add the variable to a file named enable in that directory. | |
| 2797 | nit: you can declare multiple local variables in a single statement. | |
| libexec/rc/rc.subr | ||
|---|---|---|
| 2778 | i think i'll do this in a separate review, because potentially write_rcvar could be used for things which are not ${name}_enable and we probably need to decide when/how to write files to that directory. | |