From owner-freebsd-rc@FreeBSD.ORG Wed Jun 15 21:02:57 2011 Return-Path: Delivered-To: freebsd-rc@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 010261065672 for ; Wed, 15 Jun 2011 21:02:57 +0000 (UTC) (envelope-from jilles@stack.nl) Received: from mx1.stack.nl (relay02.stack.nl [IPv6:2001:610:1108:5010::104]) by mx1.freebsd.org (Postfix) with ESMTP id 96E528FC0A for ; Wed, 15 Jun 2011 21:02:56 +0000 (UTC) Received: from turtle.stack.nl (turtle.stack.nl [IPv6:2001:610:1108:5010::132]) by mx1.stack.nl (Postfix) with ESMTP id ECE153593E3; Wed, 15 Jun 2011 23:02:55 +0200 (CEST) Received: by turtle.stack.nl (Postfix, from userid 1677) id D922517399; Wed, 15 Jun 2011 23:02:55 +0200 (CEST) Date: Wed, 15 Jun 2011 23:02:55 +0200 From: Jilles Tjoelker To: jhell Message-ID: <20110615210255.GA44402@stack.nl> References: <20110615034526.GA12185@DataIX.net> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20110615034526.GA12185@DataIX.net> User-Agent: Mutt/1.5.21 (2010-09-15) Cc: freebsd-rc@freebsd.org Subject: Re: [PATCH] Knock out two if statements, one eval & IDCMD with builtin test. X-BeenThere: freebsd-rc@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: "Discussion related to /etc/rc.d design and implementation." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 15 Jun 2011 21:02:57 -0000 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? > 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. 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 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; } -- Jilles Tjoelker