Roland Mainz wrote: > Tony Nguyen wrote: > >> Roland Mainz wrote: >> >>> [EMAIL PROTECTED] 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": > Hi Roland,
Apologies for a late response. I'm responding to non-ksh93 specific comments as the team decided to defer making the these ksh93 scripts. Thanks again, tony > - 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 -- > These are globals constants with the exception of CONF_FILES and NAT_FILES which I can change to lowercase. > - 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. > good point. I'll make the change. > - 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) > agreed, a case statement is more appropriate. > - Please make sure all functions have explicit "return" statements at > the end > will do. > - 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" > ? > I'm not sure and will look into that. _______________________________________________ networking-discuss mailing list [email protected]
