svn commit: r268641 - head/usr.sbin/service

Wed Jul 16 05:29:47 UTC 2014

> -----Original Message-----
> From: Bryan Drewery [mailto:bdrewery at FreeBSD.org]
> Sent: Tuesday, July 15, 2014 8:44 PM
> To: dteske at FreeBSD.org; 'Mateusz Guzik'
> Cc: src-committers at freebsd.org; svn-src-all at freebsd.org; svn-src-
> head at freebsd.org
> Subject: Re: svn commit: r268641 - head/usr.sbin/service
> 
> On 7/15/14, 9:48 PM, dteske at FreeBSD.org wrote:
> >
> >
> >> -----Original Message-----
> >> From: owner-src-committers at freebsd.org [mailto:owner-src-
> >> committers at freebsd.org] On Behalf Of Mateusz Guzik
> >> Sent: Tuesday, July 15, 2014 12:16 PM
> >> To: Bryan Drewery
> >> Cc: Devin Teske; src-committers at freebsd.org; svn-src-all at freebsd.org;
> svn-
> >> src-head at freebsd.org
> >> Subject: Re: svn commit: r268641 - head/usr.sbin/service
> >>
> >> On Tue, Jul 15, 2014 at 12:59:05PM -0500, Bryan Drewery wrote:
> >>> On 7/14/2014 9:18 PM, Devin Teske wrote:
> >>>> Author: dteske
> >>>> Date: Tue Jul 15 02:18:55 2014
> >>>> New Revision: 268641
> >>>> URL: http://svnweb.freebsd.org/changeset/base/268641
> >>>>
> >>>> Log:
> >>>>    Fix an issue with service(8) where utilities such as screen(1)
and
> tmux(1)
> >>>>    would behave differently when utilizing rc-script was invoked
> manually
> >> vs.
> >>>>    service(8). The issue being that these utilities require the TERM
> environ
> >>>>    variable to be set and service(8) was not passing it down.
> >>>>
> >>>>    Reported by:	Michael Dexter <editor at callfortesting.org>
> >>>>    PR:		bin/191869
> >>>>    Reviewed by:	allanjude
> >>>>    MFC after:	3 days
> >>>>    X-MFC-to:	stable/10, stable/9
> >>>>
> >>>> Modified:
> >>>>    head/usr.sbin/service/service.sh
> >>>>
> >>>> Modified: head/usr.sbin/service/service.sh
> >>>>
> >>
> ==========================================================
> >> ====================
> >>>> --- head/usr.sbin/service/service.sh	Tue Jul 15 01:03:29 2014
> >> 	(r268640)
> >>>> +++ head/usr.sbin/service/service.sh	Tue Jul 15 02:18:55 2014
> >> 	(r268641)
> >>>> @@ -139,7 +139,7 @@ cd /
> >>>>   for dir in /etc/rc.d $local_startup; do
> >>>>   	if [ -x "$dir/$script" ]; then
> >>>>   		[ -n "$VERBOSE" ] && echo "$script is located in
$dir"
> >>>> -		exec env -i HOME=/
PATH=/sbin:/bin:/usr/sbin:/usr/bin
> >> $dir/$script $*
> >>>> +		exec env -i HOME=/
PATH=/sbin:/bin:/usr/sbin:/usr/bin
> >> TERM="$TERM" $dir/$script $*
> >>>>   	fi
> >>>>   done
> >>>>
> >>>>
> >>>
> >>> Hm, I'm not sure about this. The "behaves differently" is exactly the
> >>> reason for service(8). It runs with a clean environment such as the
boot
> >>> does. Running an rc script without service(8) will give wrong
behavior
> >>> in many scripts that do not match boot-time behavior.
> >>>
> >>
> >> Indeed, the whole point is to NOT inherit anything from calling
process.
> >>
> >
> > If that were the point, then we have an issue. Because as it stands,
> > service(8) does _not_ accurately provide the same environment as boot.
> >
> > Take the following boot script:
> >
> > dteske at scribe9.vicor.com ~ $ cat /etc/rc.d/foo
> > #!/bin/sh
> > # PROVIDE: foo
> > set > /tmp/termtest
> > . /etc/rc.subr
> > name=foo
> > rcvar=foo_enable
> > command="/usr/local/bin/tmux"
> > foo_flags="new -s foo ls"
> > load_rc_config $name
> > run_rc_command "$1"
> >
> > Now reboot to find the following in /tmp/termtest:
> >
> > dteske at scribe9.vicor.com ~ $ cat /tmp/termtest
> > HOME=/
> > ID=/usr/bin/id
> > IDCMD='if [ -x /usr/bin/id ]; then /usr/bin/id -un; fi'
> > IFS='
> > '
> > JID=0
> > OPTIND=1
> > PATH=/sbin:/bin:/usr/sbin:/usr/bin
> > PPID=1
> > PS1='# '
> > PS2='> '
> > PS4='+ '
> > PS='/bin/ps -ww'
> > PWD=/
> > RC_PID=19
> > SYSCTL=/sbin/sysctl
> > SYSCTL_N='/sbin/sysctl -n'
> > SYSCTL_W=/sbin/sysctl
> > _arg=faststart
> > _boot=faststart
> > _file=/etc/rc.d/foo
> > _rc_conf_loaded=true
> > _rc_elem=/etc/rc.d/foo
[snip]
> >
> > Compare that with the following:
> >
> > dteske at scribe9.vicor.com ~ $ sr service foo start
> > dteske at scribe9.vicor.com ~ $ cat /tmp/termtest
> > HOME=/
> > IFS='
> > '
> > OPTIND=1
> > PATH=/sbin:/bin:/usr/sbin:/usr/bin
> > PPID=1285
> > PS1='# '
> > PS2='> '
> > PS4='+ '
> > PWD=/
> >
> > Looks to me like some things are being inherited.
> > Notably, it would appear that the rc.d scripts are
> > run within the parent namespace which has had
> > source_rc_confs() called (I'm leaving alone for the
> > moment that this indicates that each rc-script is
> > being less than efficient by re-sourcing rc.subr).
> >
> > Given the above (that we are farther from a "clean
> > environment" than one might expect), is it really
> > that big of a deal to expose $TERM? If so, what are
> > the serious issues that could arise?
> >
> > I personally cannot think of any shortcomings or
> > fallout from adding $TERM to be exposed.
> >
> 
> You said the shortcoming yourself - it doesn't fix the issue at boot.

Huh? The proposal is to keep the patch to service(8) and add
yet another patch to make $TERM accessible during boot.
Right now, with the patch to service(8) as it stands, of the three
scenarios where $TERM is accessible to rc.d scripts, only one remains:

1. In multi-user mode when executed directly
2. In multi-user mode when executed via service(8)
3. At boot-time (which does not use service(8))

My patch addressed #2 above, #1 already expands $TERM in an rc.d
script, and the proposal is to make a patch for #3 to expand $TERM
in rc.d scripts (it is this last one that [quote] "personally cannot think
of any shortcomings or fallout from"). With this trio complete (still
looking for the best possible way to integrate init(8) interpretation
of tty(5) into an `early' $TERM expansion for rc.d scripts), the
problem *would* be solved. Whereas backing out the patch for #2
takes us back to only #1 working.

> What exactly are you fixing? What screen/tmux rc scripts are you
> referring to? The ports don't have these.
> 

While it's constructive to ask what this is for, I do feel that at a higher
level we shouldn't restrict ports from implementing this.

Specifically this is for bhyve (or anything wanting to instantiate named
screen/tmux sessions at boot-time -- or since not all rc.d scripts are
built for boot utilization, via "service" level management interface).

> Also your change introduces bugs into the manpage which states exactly
> what environment is passed in.
> 

Ah, thanks. Didn't know that much was documented. I propose
that the man-page be changed in alignment if we decide to go
this route. I do think it is canonically correct to provide $TERM in
a boot context, service context, and multi-user invocation context.
Not all rc.d scripts are "boot" scripts. Especially considering the
level of flexibility we have with the "rcng" style rc-script (advanced
over the old *.sh-style which couldn't decline boot activity in the
same ways).

> What terminal I happen to restart a script from should not affect the
> process. This is a regression of one of the key *documented* points of
> service(8) that it will not pollute the rc script with the calling
> environment. TERM is a pretty big aspect of an environment.
> 

I must have missed that in the man-page. I did however find this which
you may have been misinterpreting:

[quote] When used for this purpose it will set the same restricted
environment that is in use at boot time (see below).[/quote]

The "(see below)" is taken as:

[quote]When used to run rc.d scripts the service command sets HOME to /
and PATH to /sbin:/bin:/usr/sbin:/usr/bin which is how they are set in
/etc/rc at boot time.[/quote]

I do not interpret that as you say [quote]service(8) ... will not pollute
the
rc script with the calling environment.[/quote] but rather, I interpret as
"it tries to replicate boot". So my proposition is...

1. We make /etc/rc set $TERM
2. We update the service(8) manual to say it sets $TERM in the same way

In that way service(8) remains true to its stated primary use,
[quote]to start and stop services provided by the rc.d scripts[/quote]
*while* still replicating the boot process.

> I run my systems 100% remotely. I don't want whatever console happens to
> be setup to be modifying my startup scripts with a TERM value.
> Especially if I go run service(8) from SSH and have it change behavior
> further.
> 

That's an edge-case that would see the other end of this dichotomy
do nothing but suffer. Let me explain. There's you arguing for a situation
that will never occur, and then there's the people wanting to use screen
and/or tmux in an rc.d script that simply can't because 2/3rds of the ways
an rc.d script can be invoked currently (pre SVN r268641; post r268641
only 1/3rd) end in failure.

I'm aware that we could just pass-the-buck and tell people trying to use
those applications via rc.d scripts to forcefully set a TERM variable, but
the fallbacks of using the wrong value for $TERM end in the same failure.

Maybe there's a compromise... we turn it on in HEAD and don't MFC any
of it (r268461 and proposed future patch to have /etc/rc set $TERM; with
man-page update, again, if we decided this was something we agree on).
If any of the rc.d scripts break (either in base or ports) we revert it.

But there's another choice as well... we could set TERM but not export it.
This would allow it to be expandable but would not leach to forked daemons
or utilities (remaining in the shell namespace). Then rc.d scripts wanting
to
export it need only say-so (export TERM).

Trying to open doors here. While I understand the hypothetical that some
program spawned by an rc.d script might behave differently if you're $TERM
changes, I think that's purely a hypothetical at this point (I can think of
no
ports off the top of my head that this would impact; and no base rc.d
scripts
should care).
-- 
Devin