Date: Tue, 15 Jul 2014 19:13:44 -0700 From: <dteske@FreeBSD.org> To: "'Mateusz Guzik'" <mjguzik@gmail.com>, "'Bryan Drewery'" <bdrewery@FreeBSD.org> Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, 'Devin Teske' <dteske@FreeBSD.org>, src-committers@freebsd.org Subject: RE: svn commit: r268641 - head/usr.sbin/service Message-ID: <011a01cfa09b$928b4710$b7a1d530$@FreeBSD.org> In-Reply-To: <20140715191553.GA31990@dft-labs.eu> References: <201407150218.s6F2Itj8044531@svn.freebsd.org> <53C56BE9.9050304@FreeBSD.org> <20140715191553.GA31990@dft-labs.eu>
next in thread | previous in thread | raw e-mail | index | archive | help
> -----Original Message----- > From: owner-src-committers@freebsd.org [mailto:owner-src- > committers@freebsd.org] On Behalf Of Mateusz Guzik > Sent: Tuesday, July 15, 2014 12:16 PM > To: Bryan Drewery > Cc: Devin Teske; src-committers@freebsd.org; svn-src-all@freebsd.org; = svn- > src-head@freebsd.org > Subject: Re: svn commit: r268641 - head/usr.sbin/service >=20 > 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@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 > > > > = =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D > > > --- 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=3D/ PATH=3D/sbin:/bin:/usr/sbin:/usr/bin > $dir/$script $* > > > + exec env -i HOME=3D/ PATH=3D/sbin:/bin:/usr/sbin:/usr/bin > TERM=3D"$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. > > >=20 > Indeed, the whole point is to NOT inherit anything from calling = process. >=20 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. > > >=20 > Yes, but maybe there is a sensible default that could be used? >=20 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. >=20 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. >=20 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). --=20 Devin
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?011a01cfa09b$928b4710$b7a1d530$>