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

[dteske at FreeBSD.org]

> -----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.
> 

I would argue that not all programs are going to like having
a nearly empty environment. Things like TERM and SHLVL
at the very least should be passed (after-all, the boot process
takes place on [a] a terminal and [b] in a shell).

Blatting out the environment feels rude and non-UNIXy.
UNIX programs can, and should expect at a minimum to
be able to interrogate the environment they are running
within -- destroying all vestiges of that environment purely
for the sake of saying "it's clean" seems counter to the
UNIX pathos.


> > As far as I can tell, TERM is not set on boot. So the rc script would
> > also not work on boot. So this change is wrong.
> >
> 
> Yes, but maybe there is a sensible default that could be used?
> 

I argue that the sensible default should be what was inherited.
Otherwise, if you want to (for example) change said default from
cons25 to xterm (just as a well-known example) then you risk
having to make the change in more than one place (rather than
letting it trickle down from ttys(5)).

Furthermore, while I can believe that TERM is not set for rc.d
scripts (because I tested it), I see no reason why this has to be.
By the time the boot process has started, we have invoked init(8)
and it has processed ttys(5) and we should know what type of
terminal we are on. Since we ought to know this information, why
_not_ pass it along to the boot scripts so that, god forbid, a boot
script might be able to handle different terminal types appropriately
(read: serial console situation). The alternative would be that the
startup script would have to invoke some C code utilizing isatty(3)
etc. on stdin which seems like more of a kludge than just revealing
$TERM.

> Alternatively, one would have to fixup rc scripts using tmux to do
> what's best for them.
> 

The only thing that will work as a patch to an rc-script to allow said rc-
script to utilize tmux or screen would be to manually set the TERM
variable to some guessed value. Considering the less-than-desired
outcome from using the wrong TERM variable, it seems obvious to me
that this is a less-than-desired solution (versus patching service(8)).

> Either, the change as it is looks wrong. Please revert for the time being.
> 

Reverting doesn't help anyone. I'm in-favor of modifying the boot
process to reveal $TERM -- thus allowing scripts that rely on $TERM
to be able (as is the intention) to function as-desired in all situations.

I will agree that the patch is not holistic in-that there still exists the
edge-case that boot-scripts utilizing screen and/or tmux will still fail
because the boot-process does not reveal $TERM, but I think reverting
the progress made so far ignores the larger picture (that honestly... not
providing $TERM is wrong).
-- 
Devin