Date: Thu, 16 Jun 2011 09:40:32 -0400 From: jhell <jhell@DataIX.net> To: Jilles Tjoelker <jilles@stack.nl> Cc: freebsd-rc@freebsd.org Subject: Re: [PATCH] Knock out two if statements, one eval & IDCMD with builtin test. Message-ID: <20110616134032.GA17614@DataIX.net> In-Reply-To: <20110615210255.GA44402@stack.nl> References: <20110615034526.GA12185@DataIX.net> <20110615210255.GA44402@stack.nl>
next in thread | previous in thread | raw e-mail | index | archive | help
On Wed, Jun 15, 2011 at 11:02:55PM +0200, Jilles Tjoelker wrote:
> On Tue, Jun 14, 2011 at 11:45:26PM -0400, jhell wrote:
> > After looking over Jilles patch on this same list it made ID & IDCMD
> > catch my eye when I seen the $(eval $IDCMD) where it was the only place
> > it was used throughout the whole system in which it calls another if
> > statement from IDCMD to check the presence of /usr/bin/id.
>
> > This is not bad at all, don't get me wrong but this could be done from
> > one location to knock out the eval and two if statements with one
> > builtin test right from the ID variable itself and get rid of the need
> > for the IDCMD.
>
> > Slight speed improvement ? maybe... cleaner yes.
>
> There is no patch included. Maybe you forgot it or the mailing list
> software ate it?
Hmmm thats pretty silly ;) it was attached at some point as it shows in
my sent box ;). somthing got hungry.
>
> > As for functionality can anyone think of a need to wait for processing
> > this till run_rc_command is thrown ? if so should it be escaped and
> > re-eval'd as $(eval \$ID) or something similiar later ?
>
> It is useful to postpone the check because it usually is not necessary
> at all. The id command only needs to be executed if ${name}_user is set.
>
Agree'd I kept it as close to the original as possible without trying to
do so much like the if statement in the IDCMD and the second if
statement thats nested below in run_rc_command. I believe at least this
is close or doing the same thing but with less checking.
> Also note that when /etc/rc.subr is sourced for the first time, /usr may
> not be mounted. Therefore the unavailability of /usr/bin/id must not be
> cached.
As for this caching this may actually need to be changed in what I have.
I might need to change it to ID='$(...)' so it could be eval'd at a
later point instead if that sounds good to you. I just don't see the
need for using two variables whereas we can use one with single quotes
and less if statements but like I said its only minor but something that
stood out at me when I seen it and made me question it.
Patch sourced in:
--- rc.subr.orig 2011-06-14 22:46:26.000000000 -0400
+++ rc.subr 2011-06-14 23:25:19.000000000 -0400
@@ -46,8 +46,7 @@
SYSCTL="/sbin/sysctl"
SYSCTL_N="${SYSCTL} -n"
SYSCTL_W="${SYSCTL}"
-ID="/usr/bin/id"
-IDCMD="if [ -x $ID ]; then $ID -un; fi"
+ID=$(test -x /usr/bin/ids && /usr/bin/id -un || false)
PS="/bin/ps -ww"
JID=`$PS -p $$ -o jid=`
@@ -677,10 +676,9 @@
_nice=\$${name}_nice _user=\$${name}_user \
_group=\$${name}_group _groups=\$${name}_groups
- if [ -n "$_user" ]; then # unset $_user if running as that user
- if [ "$_user" = "$(eval $IDCMD)" ]; then
- unset _user
- fi
+ # unset $_user if running as that user
+ if [ -n "$_user" -a "$_user" = "$ID" ]; then
+ unset _user
fi
eval $_pidcmd # determine the pid if necessary
>
> As for performance, expanding "$(eval $IDCMD)" currently takes two forks
> (assuming /usr/bin/id is executable), which can be reduced to one with a
> simple tweak to sh (only for 9.x sh though, not 8.x). The second fork
> can also be avoided by rearranging the script, just using "$($ID)"
> and checking its existence outside command substitution.
>
> Index: bin/sh/eval.c
> ===================================================================
> --- bin/sh/eval.c (revision 223024)
> +++ bin/sh/eval.c (working copy)
> @@ -140,7 +140,7 @@
> STPUTC('\0', concat);
> p = grabstackstr(concat);
> }
> - evalstring(p, builtin_flags & EV_TESTED);
> + evalstring(p, builtin_flags);
> } else
> exitstatus = 0;
> return exitstatus;
> @@ -908,6 +908,7 @@
> dup2(pip[1], 1);
> close(pip[1]);
> }
> + flags &= ~EV_BACKCMD;
> }
> flags |= EV_EXIT;
> }
>
I like this! seems like a good idea. agree'd for 9.X only.
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20110616134032.GA17614>