Date: Tue, 15 Jul 2014 22:29:34 -0700 From: <dteske@FreeBSD.org> To: "'Bryan Drewery'" <bdrewery@FreeBSD.org>, <dteske@FreeBSD.org>, "'Mateusz Guzik'" <mjguzik@gmail.com> Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org Subject: RE: svn commit: r268641 - head/usr.sbin/service Message-ID: <01a401cfa0b6$eda1a700$c8e4f500$@FreeBSD.org> In-Reply-To: <53C5F501.1050304@FreeBSD.org> References: <201407150218.s6F2Itj8044531@svn.freebsd.org> <53C56BE9.9050304@FreeBSD.org> <20140715191553.GA31990@dft-labs.eu> <014901cfa0a0$773c3640$65b4a2c0$@FreeBSD.org> <53C5F501.1050304@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
> -----Original Message----- > From: Bryan Drewery [mailto:bdrewery@FreeBSD.org] > Sent: Tuesday, July 15, 2014 8:44 PM > To: dteske@FreeBSD.org; 'Mateusz Guzik' > Cc: src-committers@freebsd.org; svn-src-all@freebsd.org; svn-src- > head@freebsd.org > Subject: Re: svn commit: r268641 - head/usr.sbin/service > > On 7/15/14, 9:48 PM, dteske@FreeBSD.org wrote: > > > > > >> -----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 > >> > >> 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 > >>>> > >> > ========================================================== > >> ==================== > >>>> --- 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@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@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@scribe9.vicor.com ~ $ sr service foo start > > dteske@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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?01a401cfa0b6$eda1a700$c8e4f500$>