conf/78762: [ipfw] [patch] /etc/rc.d/ipfw should excecute $firewall_script not read it

[Sean McNeil]

The following reply was made to PR conf/78762; it has been noted by GNATS.

From: Sean McNeil <sean at mcneil.com>
To: jonw at whoweb.com
Cc: bug-followup at freebsd.org
Subject: Re: conf/78762: [ipfw] [patch] /etc/rc.d/ipfw should excecute 
	$firewall_script not read it
Date: Mon, 18 Jun 2007 23:02:21 -0700

 On Tue, 2007-06-19 at 01:12 -0400, jonw at whoweb.com wrote:
 > Sourcing is intended to be used like "#include" for including libraries of
 > functions and variable assignments, not running "scripts" that are
 > intended to be executed. The fact that the shell executes code that is
 > sourced, doesn't make it correct policy to use it as such and is
 > indicative of someone finding a loophole for supporting /etc/rc.conf.d but
 > forgetting the basics of real programming.
 > 
 > Only the simplest of scripts will survive being sourced.  Anyone who tries
 > to build a complex script to support numerous conditions and branches is
 > going to assume they can use an exit statement if they require one.  I
 > did.  You can't call something a script and not support exiting, and
 > suggesting to simply not use exit is the reason that we are discussing
 > this now.  Not using exit suits your requirements for including options
 > from /etc/rc.conf.d fine, but doesn't suit my needs to actually execute a
 > script that has conditions and branches based upon various OS
 > configurations and from which I might need to exit immediately if certain
 > conditions are met.
 > 
 > It's wrong to call something a script (ie firewall_script) and treat it
 > like an include file, so reverting to the previous functionality is not
 > the correct solution.  I must be missing something regarding your
 > variables from rc.conf.d/ipfw not being included in the ipfw script.  The
 > load_rc_config routine looks for /etc/rc.conf.d/ipfw and sources that in
 > before executing the startup code.  Executing or sourcing firewall_script
 > shouldn't have any impact on the rc.conf.d/ipfw variables.
 > 
 > It sounds to me like the correct solution is to support both includes and
 > executables.  That can be done a couple of ways, maybe more.
 > 
 > 1) If firewall_script is defined, execute it.  If firewall_include is
 > defined, source it.
 > 
 > 2) Check the mode of firewall_script.  If it's executable, execute it.  If
 > it's not executable, source it.
 > 
 > Jon
 
 Thank you, Jon.  I like your suggestion.  Indeed, having something named
 _script and sourcing it would be misleading.  The problem is that when
 you execute the script you lose all assignments made in /etc/rc.d/ipfw
 and /etc/rc.firewall only sources /etc/rc.conf and /etc/rc.conf.local.
 
 Actually, I think your suggestion should have been applied and
 firewall_script should have been executed without forcing the shell with
 the /bin/sh or ".". That way you can direct which shell to use in the
 script (#!/bin/sh, #!/bin/csh) or in the assignment of $firewall_script
 and the default could be changed from "/etc/rc.firewall" to
 ". /etc/rc.firewall".  Except then something would have to be done with
 the -z and -r tests, so that wouldn't quite work as is.
 
 Funny how such a little thing can become so complicated.
 
 If I understand you correctly about read vs. execute, it should look
 something like
 
                 if [ -x "${firewall_script}" ]; then
                         "${firewall_script}"
                 else
                         . "${firewall_script}"
                 fi
 
 This would restore the rcNG /etc/rc.conf.d/ipfw setting ability and
 allow you to use the shell of your choosing.
 
 > 
 > > This is a bad idea and has broken the new feature of rcNG allowing us to
 > > place options into /etc/rc.conf.d/ipfw and /etc/rc.conf.d/ip6fw.  The
 > > commit to src/etc/rc.d/ipfw revision 1.15 and src/etc/rc.d/ip6fw 1.9
 > > have now broken this basic concept.
 > >
 > > IMHO, the correct thing is: Don't use exit in your firewall script.  I
 > > offer 3 solutions, however, below.
 > >
 > > What has been broken:
 > >
 > > /etc/rc.conf.d/ipfw
 > > 	firewall_enable="YES"
 > > 	firewall_type="/etc/fw/rc.firewall.rules"
 > >
 > > /etc/rc.conf.d/ip6fw
 > > 	ipv6_firewall_enable="YES"
 > > 	ipv6_firewall_type="/etc/fw/rc.firewall6.rules"
 > >
 > > Now, this no longer works and I must once again pollute and move more
 > > stuff back into /etc/rc.conf.  Namely,
 > >
 > > 	firewall_type="/etc/fw/rc.firewall.rules"
 > > 	ipv6_firewall_type="/etc/fw/rc.firewall6.rules"
 > >
 > > must now be in /etc/rc.conf or /etc/rc.conf.local.
 > >
 > > Solution:
 > >
 > > 1) revert to sourcing the rc.firewall script.
 > > 2) Fix rc.firewall and rc.firewall6 to somehow get stuff
 > > from /etc/rc.conf.d as it should (as ipfw and ip6fw?).
 > > 3) completely remove rc.conf.d support as more things fail to work with
 > > it.
 > >
 > >
 > >
 > 
 > 
 >