Page MenuHomeFreeBSD

rc: Honor ${name}_env when a custom *_cmd is defined (e.g., start_cmd)
ClosedPublic

Authored by 0mp on Aug 12 2019, 3:26 PM.

Details

Summary
rc: Honor ${name}_env when a custom *_cmd is defined (e.g., start_cmd)

A user may set ${name}_env variable in rc.conf(5) in order to set additional
environment variables for a service command. Unfortunately, at the moment this
variable is only honored when the command is specified via the command
variable. Those additional environment variables are never set if the service
is started via the ${rc_arg}_cmd variable (for example start_cmd).

PR:		239692
Test Plan
  1. Save the following script as /usr/local/etc/rc.d/teststartcmd and make it executable:
#!/bin/sh
# PROVIDE: teststartcmd
. /etc/rc.subr
name=teststartcmd
load_rc_config $name
start_cmd="teststartcmd_start"
teststartcmd_start() {
	env
}
run_rc_command "$1"
  1. Run sysrc teststartcmd_env="TESTVARIABLE=foo" to modify rc.conf.
  1. Run service teststartcmd onestart and observe that TESTVARIABLE is not in the output.
  1. Save the following script as /usr/local/etc/rc.d/testcommand and make it executable:
#!/bin/sh
# PROVIDE: testcommand
. /etc/rc.subr
name=testcommand
load_rc_config $name
command="/usr/bin/env"
run_rc_command "$1"
  1. Run sysrc testcommand_env="TESTVARIABLE=foo" to modify rc.conf.
  1. Run service testcommand onestart and observe that TESTVARIABLE is in the output this time.

With the patch, both outputs should contain TESTVARIABLE=foo.

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

0mp created this revision.Aug 12 2019, 3:26 PM
0mp edited the summary of this revision. (Show Details)Aug 12 2019, 3:29 PM
0mp added subscribers: kevans, dteske, jilles, cem.
bcr added a subscriber: bcr.Aug 12 2019, 3:51 PM

Minor man page fix.

share/man/man8/rc.subr.8
573 ↗(On Diff #60675)

s/the/then/
(You can also leave out the "the" completely)

0mp updated this revision to Diff 60677.Aug 12 2019, 3:53 PM

Fix a typo (thanks @bcr)

0mp marked an inline comment as done.Aug 12 2019, 3:53 PM
bcr accepted this revision.Aug 12 2019, 4:33 PM

OK from manpages.
Nice feature, BTW.

This revision is now accepted and ready to land.Aug 12 2019, 4:33 PM
0mp added a comment.Aug 13 2019, 12:50 PM

Otherwise, we may just document that ${name}_env only works if the ${rc_arg}_cmd is not defined.

0mp retitled this revision from rc: Honor ${name}_env when a custom *_cmd is define (e.g., start_cmd) to rc: Honor ${name}_env when a custom *_cmd is defined (e.g., start_cmd).Aug 28 2019, 8:19 AM
0mp edited the summary of this revision. (Show Details)
jilles requested changes to this revision.Aug 28 2019, 1:04 PM

This change looks reasonable, although the exact logic can be improved as noted. It may have been the original intent that $start_cmd use $_env by itself but that would involve replicating error-prone code.

libexec/rc/rc.subr
1039–1041 ↗(On Diff #60677)

This should have an effect as close as possible to the effect without a _cmd. In that case, env plus the value of _env is prepended to the command and this is passed to eval. The code above first applies word splitting and pathname generation to the value of _env, concatenates it with spaces and evals it as shell variable assignments.

So I suggest instead:

if [ -n "$_env" ]; then
    eval "export -- $_env"
fi

This applies exactly one expansion pass, a full shell parse and expansion.

The -- will cause any attempt to use options to fail. There are no options that agree between export and env.

Differently from env, export will not set environment variables that do not have a proper name (consisting solely of alphabetics, numerics, and underscores, and starting with an alphabetic or an underscore). There is no way the shell allows this, so this difference cannot be fixed.

This revision now requires changes to proceed.Aug 28 2019, 1:04 PM
cem removed a subscriber: cem.Aug 28 2019, 3:39 PM
0mp updated this revision to Diff 61448.Aug 29 2019, 8:16 AM
  • Apply changes to rc.subr suggested by jilles
  • Update the manpage patch to reflect latest changes
0mp updated this revision to Diff 61450.Aug 29 2019, 12:12 PM
  • Refactor manpage changes a little bit
jilles accepted this revision.Aug 29 2019, 10:21 PM
This revision is now accepted and ready to land.Aug 29 2019, 10:21 PM
0mp added a comment.Aug 30 2019, 8:11 AM

Just one last question: do we want to MFC this change?