Roland Mainz wrote: > 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.
Roland, Thanks you for the comments. The change to existing base scripts is really the concern, your suggestion to make ipf_include.sh ksh93 was never controversial. Please see Nico's response. > > BTW: Some comments about "ipf_include.sh": I'm wrapping up other changes and will respond to your comments as soon as I'm done. cheers, -tony > > - 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 >
