Date: Tue, 27 Dec 2011 18:20:12 GMT From: Devin Teske <devin.teske@fisglobal.com> To: freebsd-rc@FreeBSD.org Subject: Re: conf/163508: [rc.subr] [patch] Add " enable" and " disable" commands to rc.subr Message-ID: <201112271820.pBRIKCKB084959@freefall.freebsd.org>
next in thread | raw e-mail | index | archive | help
The following reply was made to PR conf/163508; it has been noted by GNATS. From: Devin Teske <devin.teske@fisglobal.com> To: <bug-followup@FreeBSD.org>, <gelraen.ua@gmail.com> Cc: <devin.teske@fisglobal.com> Subject: Re: conf/163508: [rc.subr] [patch] Add "enable" and "disable" commands to rc.subr Date: Tue, 27 Dec 2011 10:17:42 -0800 ------=_NextPart_000_0285_01CCC480.C5B42FC0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit I would like to submit for review a modified version of the original submitter's patch. I feel that the original patch takes a too-simplistic view of the problem at-hand and am offering a much more robust solution. The replacement patch uses my "sysrc" utility which -- if you haven't discovered it yet -- is a peer-reviewed "nuclear reactor" approach opposed to a "bike shed" approach. sysrc takes everything into consideration, including (but not limited to): (listed in no particular order) 1. Environment Variable Taint It is not possible to "confuse" or "break" the code by exporting strange things into the environment. 2. Shell Taint Checking If rc.conf(5) has invalid syntax prior to editing, it will refuse to edit and an error is produced. Similarly, if editing rc.conf(5) introduces a syntax error, the original rc.conf(5) is restored and an error is produced. This prevents producing a situation where rc.conf itself prevents you from booting into multi-user mode. If, for any reason at all, rc.conf(5) causes you to drop to single-user mode, it's assuredly is NOT because of sysrc, as it taint-checks both before and after. 3. Safety First Use mktemp to prevent race-conditions. Use atomic actions where necessary. 4. Minimal changes The original patch submitted with conf/163508 does more work than is necessary w/respect to items-changed within a single-file. Case-in-point, the "replace_var" function simply fires a sed global-replace to replace all instances of "thing=blah" on multiple lines. That's not cool. Sysrc will ONLY change the last [valid] assignment to the variable. Sysrc wants to keep your rc.conf exactly the way you like it as best it can and change as little as possible (and when it does make changes, it wants to do it in a fashion that's going to preserve as much structure as possible; see next item). 5. Quotations, whitespace, compound statements and in-line comments The sed command in the original patch's "replace_var" function (a) WILL preserve leading whitespace but (b) WON'T preserve in-line comments that appear after the assignment, (c) NOR preserve the type of quotation that was used in the assignment(s), (d) NOR preserve compound statements. When sysrc replaces the last [valid] assignment to a given variable, it will actually preserve the type of quotation used (whether it be single, double or no-quotation). It will also preserve both leading whitespace and trailing whitespace. It will also retain in-line comments. It will also preserve trailing commands in a compound statement (multi-variable-assignment compound separated by whitespace or multi-command compound separated by semi-colon). 6. Leave certain things alone! Sysrc will refuse to modify things that it knows that it couldn't have possibly done. For example, it will refuse to touch something like this: foogribble_enable=`echo YES` and instead opt to add an overriding new entry. 7. Performance Sysrc has been rewritten several times to improve performance. It heavily leverages awk to improve performance by several orders of magnitude compared to using straight bourne-shell built-in internals. ( end itemized list ; continue discussion ) The above list calls out major flaws in the currently-submitted patch and highlights the fact that sysrc has no such short-comings. NOTE: sysrc would have to be taken into the base to support this patch. (aside) MidnightBSD has taken it into its base already (and there's another distro that escapes my mind at the moment, which has similarly taken it into its base) NOTE: In my submitted patch, there are two open-ended questions to reviewers: (a) ought we to use a full pathname to sysrc and if-so (b) where might sysrc be placed? (/bin would be nice so that it's available in single-user mode). -- Devin P.S. sysrc is available here: http://druidbsd.sourceforge.net/download/sysrc.txt _____________ The information contained in this message is proprietary and/or confidential. If you are not the intended recipient, please: (i) delete the message and all copies; (ii) do not disclose, distribute or use the message in any manner; and (iii) notify the sender immediately. In addition, please be aware that any message addressed to our domain is subject to archiving and review by persons other than the intended recipient. Thank you. ------=_NextPart_000_0285_01CCC480.C5B42FC0 Content-Type: text/plain; name="conf_163508_patch.new.txt" Content-Transfer-Encoding: quoted-printable Content-Disposition: attachment; filename="conf_163508_patch.new.txt" --- /usr/src.8/etc/rc.subr 2011-09-25 10:25:06.000000000 +0300=0A= +++ /etc/rc.subr 2011-12-27 08:33:50.000000000 -0800=0A= @@ -443,7 +443,7 @@=0A= #=0A= # run_rc_command argument=0A= # Search for argument in the list of supported commands, which is:=0A= -# "start stop restart rcvar status poll ${extra_commands}"=0A= +# "start stop restart rcvar status poll enable disable = ${extra_commands}"=0A= # If there's a match, run ${argument}_cmd or the default method=0A= # (see below).=0A= #=0A= @@ -579,6 +579,10 @@=0A= #=0A= # rcvar Display what rc.conf variable is used (if any).=0A= #=0A= +# enable Set ${rcvar} to YES=0A= +#=0A= +# disable Set ${rcvar} to NO=0A= +#=0A= # Variables available to methods, and after run_rc_command() has=0A= # completed:=0A= #=0A= @@ -647,7 +651,7 @@=0A= eval _override_command=3D\$${name}_program=0A= command=3D${_override_command:-$command}=0A= =0A= - _keywords=3D"start stop restart rcvar $extra_commands"=0A= + _keywords=3D"start stop restart rcvar enable disable $extra_commands"=0A= rc_pid=3D=0A= _pidcmd=3D=0A= _procname=3D${procname:-${command}}=0A= @@ -689,12 +693,26 @@=0A= if [ "$_elem" !=3D "$rc_arg" ]; then=0A= continue=0A= fi=0A= +=0A= + if [ -n "${rcvar}" -a "${rc_arg}" =3D=3D "enable" ]; then=0A= + if checkyesno ${rcvar}; then=0A= + echo "Service ${name} already enabled."=0A= + return 0=0A= + fi=0A= + fi=0A= + if [ -n "${rcvar}" -a "${rc_arg}" =3D=3D "disable" ]; then=0A= + if ! checkyesno ${rcvar}; then=0A= + echo "Service ${name} not enabled."=0A= + return 0=0A= + fi=0A= + fi=0A= +=0A= # if ${rcvar} is set, $1 is not "rcvar"=0A= # and ${rc_pid} is not set, then run=0A= # checkyesno ${rcvar}=0A= # and return if that failed=0A= #=0A= - if [ -n "${rcvar}" -a "$rc_arg" !=3D "rcvar" -a "$rc_arg" !=3D "stop" = ] ||=0A= + if [ -n "${rcvar}" -a "$rc_arg" !=3D "rcvar" -a "$rc_arg" !=3D "stop" = -a "$rc_arg" !=3D "enable" ] ||=0A= [ -n "${rcvar}" -a "$rc_arg" =3D "stop" -a -z "${rc_pid}" ]; then=0A= if ! checkyesno ${rcvar}; then=0A= if [ -n "${rc_quiet}" ]; then=0A= @@ -895,6 +913,16 @@=0A= echo ""=0A= ;;=0A= =0A= + enable|disable)=0A= + local val=0A= + if [ "$rc_arg" =3D "enable" ]; then=0A= + val=3D"YES"=0A= + else=0A= + val=3D"NO"=0A= + fi=0A= + sysrc "$rcvar=3D$val" && sysrc -v "$rcvar"=0A= + ;;=0A= +=0A= *)=0A= rc_usage $_keywords=0A= ;;=0A= ------=_NextPart_000_0285_01CCC480.C5B42FC0--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201112271820.pBRIKCKB084959>