Tony Nguyen wrote: > > Roland Mainz wrote: > > Casper.Dik at Sun.COM wrote: > >>> Tony Nguyen wrote: > >>>> The updated and incremental webrevs are at: > >>>> > >>>> http://cr.opensolaris.org/~tonyn/firewall29Oct2008/ > >>>> > >>>> http://cr.opensolaris.org/~tonyn/firewall-inc/ > >>>> > >>>> The original webrev is still available at: > >>>> > >>>> http://cr.opensolaris.org/~tonyn/firewall/ > >>> Before starting to comment on the scripts: Is it mandatory for the > >>> firewall SMF scripts to use /sbin/sh or is /usr/bin/ksh acceptable, too > >>> ? > >> Possibly if you make sure that /usr is mounted first. > > > > The scripts already use stuff like "sed" ([1]) and AFAIK that means they > > depend on /usr being mounted. > > > > [1]=my main concern is that this script could be slighty reworked to > > avoid depending on "sed"&co. just to dismantle one single string - > > builtin shell operators can do the same in a much faster manner (and if > > we're allowed to use ksh93 we may be able to fold various calls to > > "svcprop" to just one in a similar manner how > > http://svn.genunix.org/repos/on/branches/ksh93/gisburn/scripts/svcproptree1.sh > > works). > > > > It's a good point. network/ipfilter has a dependency on filesystem/usr > so that isn't an issue. If we use ksh, it'd have to be ksh93 since > that's the only version shipped with OpenSolaris. However, making this > script ksh93 requires method scripts delivered by other teams and > consolidations to be ksh93 which I'm not ready to mandate and perhaps > requiring ARC commitment. I'd like to defer the suggested change until > the follow up IPv6 support work at which I can make that proposal.
AFAIK we do not need ARC since we only affect the base scripts and "ipf_include.sh" which is consumed by those scripts. BTW: Some comments about "ipf_include.sh": - AFAIK it may be better to change "ipf_get_lock" to an implementation which uses "mkdir" for locks (e.g. basically ([1]) $ mkdir "mylock" && printf "lock obtained\n" || printf "could not obtain lock\n" # to obtain the lock and $ rmdir "mylock" # to release it) since this is the only operation which is guranteed to be atomic and avoids the creation of duplicates (if you need an implementation for a lock scheme with multiple concurrent readers see http://svn.genunix.org/repos/on/branches/ksh93/gisburn/scripts/filemutexdemo1.sh (it uses ksh93 classes but I can provide a plain Bourne version on demand)). [1]=Untested implementation: -- snip -- try_lock { lockname="$1" mkdir "/tmp/myapplocks.${lockname}" 2>/dev/null && return 0 return 1 } obtain_lock() { lockname="$1" wait_interval=1 # we should really start with 0.1 but # "expr" does not support floating-point values while true ; do if try_lock "${lockname}" ; then return 0 fi sleep ${wait_interval} # double sleep interval to avoid hogging too much # resources when busy-waiting wait_interval=`expr ${wait_interval} \* 2` done # not reached } release_lock() { lockname="$1" rmdir "/tmp/myapplocks.${lockname}" return 0 } -- snip -- - AFAIK these (and likely some other variables) should be lowercase since they are _not_ exported into the environment unless they are _readonly_ constants (see http://www.opensolaris.org/os/project/shell/shellstyle/#names_should_be_lowercase): -- snip -- 27 ETC_IPF_DIR=/etc/ipf 28 VAR_IPF_DIR=/var/tmp/ipf 29 IPFILCONF=$ETC_IPF_DIR/ipf.conf 30 IPFILOVRCONF=$ETC_IPF_DIR/ipf_ovr.conf 31 IP6FILCONF=$ETC_IPF_DIR/ipf6.conf 32 IPFILSVCSCONF=$ETC_IPF_DIR/ipf_services.conf 33 IPNATCONF=$ETC_IPF_DIR/ipnat.conf 34 IPPOOLCONF=$ETC_IPF_DIR/ippool.conf 35 IPF_LOCK=/var/run/ipfilter.pid 36 CONF_FILES="" 37 NAT_FILES="" -- snip -- - Please avoid using "echo", if possible use "printf" (see http://www.opensolaris.org/os/project/shell/shellstyle/#avoid_echo) For example: -- snip -- 696 if [ $? -eq 0 -a -n "$addr" ]; then 697 echo "pass in log quick proto ${proto} from ${addr}" \ 698 "to ${ip} port = ${port} ${tcp_opts}" >>${out} 699 fi -- snip -- ... could be changed to... -- snip -- 696 if [ $? -eq 0 -a -n "$addr" ]; then 697 printf \ "pass in log quick proto %s from %s to %s port = %s %s\n" \ "${proto}" "${addr}" "${ip}" "${port}" "${tcp_opts}" >>${out} 699 fi -- snip -- - It may be nice to put the script through the "ksh93 -n" wringer (ignore the '`...` obsolete, use $(...)' warning since the Bourne shell does not implement $(..)-style command substitutions). For example for "ipf_include.sh" I am getting: -- snip -- $ ksh93 -n ipf_include.sh 2>&1 | fgrep -v '`...` obsolete, use $(...)' ipf_include.sh: warning: line 167: Invariant test ipf_include.sh: warning: line 193: Invariant test ipf_include.sh: warning: line 443: Invariant test -- snip -- - "true" / "false" don't need extra calls of the test operator, e.g. ... -- snip -- 593 if [ "$isrpc" = "true" ]; then -- snip -- ... can be changed to ... -- snip -- 593 if "$isrpc" ; then -- snip -- (assuming "isrpc" only contains the values "true" and "false". POSIX shell (unfortunately not the original Bourne shell) allows the use of '!' to negate the return code, e.g. $ /usr/xpg4/bin/sh -c '( ! false ) && echo "hello"' # will return "hello"). - Function "generate_rules": You're several times redirecting to file "out" (e.g. >>${out}) - it may be better to open it only once using $ exec 8>"${out}" # and then use $ printf "foo\n" >&8 # to write such a file (this will avoid a |open()|/|write()|/|close()| sequence for each ">>"/">"-style redirection). For ksh93 see http://www.opensolaris.org/os/project/shell/shellstyle/#use_redirect_not_exec_to_open_files and http://www.opensolaris.org/os/project/shell/shellstyle/#use_dynamic_file_descriptors - The following "mktemp" call should use the PID as part of the pattern, e.g. change: -- snip -- 868 TEMP=`mktemp /var/run/ipf.conf.XXXXXX` 869 process_nonsvc_progs $TEMP -- snip -- ... to ... -- snip -- 868 TEMP=`mktemp /var/run/ipf.conf.pid$$.XXXXXX` 869 process_nonsvc_progs $TEMP -- snip -- This helps to track-down files without owners. - Function "create_global_rules", lines -- snip -- 873 if [ "$policy" != "none" ]; then 877 if [ "$policy" = "deny" ]; then 899 if [ "$policy" = "allow" ]; then -- snip -- AFAIK either "case" or nested "if" may be better (or POSIX shell "elif" for ksh88/ksh93/bash) - Please make sure all functions have explicit "return" statements at the end - You're using tools from /usr/xpg4/bin/ - does this require any updates of the package dependicies, e.g. make your package depend on "SUNWxcu4" ? ---- Bye, Roland -- __ . . __ (o.\ \/ /.o) roland.mainz at nrubsig.org \__\/\/__/ MPEG specialist, C&&JAVA&&Sun&&Unix programmer /O /==\ O\ TEL +49 641 3992797 (;O/ \/ \O;)
