[prev in list] [next in list] [prev in thread] [next in thread]
List: opensolaris-smf-discuss
Subject: Re: [smf-discuss] [networking-discuss] Code review request -
From: Tony Nguyen <Truong.Q.Nguyen () Sun ! COM>
Date: 2008-11-07 0:14:54
Message-ID: 4913887E.4040407 () sun ! com
[Download RAW message or body]
Roland Mainz wrote:
> Tony Nguyen wrote:
>> Roland Mainz wrote:
>>> Casper.Dik@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
>
_______________________________________________
smf-discuss mailing list
smf-discuss@opensolaris.org
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic