David Powell wrote:
> Tony Nguyen wrote:
> > The incremental and revised full webrev are respectively at:
> >
> > http://cr.opensolaris.org/~tonyn/firewallDec112008-inc/
> > http://cr.opensolaris.org/~tonyn/firewallDec112008/
> >
> > Some more closures to prior discussion below.
> >
> > The only portion of the code left to be reviewed are the ipf rule
> > generation.
[snip]
> ipf_include.sh:
>
> Instead of:
>
> command
> [ $? -ne 0 ] && return 1
>
> (and similar constructs), you could just do:
>
> command || return 1
>
> Instead of elaborate counting logic to limit the number of
> iterations of your while loops, just use 'for i in `seq 1 3`;
> do...'.
"seq" comes from the "SUNWgnu-coreutils" and requires a package
dependicy in this case. Additionally it's an extra (and unneccesary)
|fork()|+|exec()|.
Either normal "for i in 1 2 3" syntax is usefull (or the faster ksh93
version "for (( i=1 ; i <=3 ; i++ )) ; do ... done" works, too) but
using "seq" would be my last choice.
[snip]
> get_IP could just do something like:
>
> echo "$1" | sed -n -e 's,^pool:\(.*\),pool/\1,p' \
> -e 's,^host:\(.*\),\1,p' \
> -e 's,^network:\(.*\),\1,p'
At this point I am wondering whether it mayx be better to use ksh or
ksh93 syntax (e.g. $ x="foobarbaz"; print
"${x/~(E)(foo)(bar)(baz)/\1BAR\3}" # to replace "baz" with "BAZ" in this
case (~(E)pattern selecting extended regular expressions for the pattern
("E" may be replaced with perl pattern, shell, python etc. pattern,
too))) in this case since this will trigger the feared
"one-fork()-per-string"-syndrome which turns this kind of startup script
into a real fork bomb.
BTW: I strongly suggest to replace "echo" with "printf" in all cases of
this script, independetly of the interpreter being used (otherwise you
may have problems with "echo"'s interpretation of backslashes).
[snip]
> While the abstractions for check_ipf_syntax and check_nat_syntax
> are nice, there's no need to explicitly return 0 if the return
> value is 0. Just call the program you're calling and let your
> caller get the exit code of what you're calling.
In geneal it is strongly recommended to use explicitly "return"
statements to return a propper exit code. Otherwise you'll run into
problems if other shell flags like "errexit" or tracing mode are active
and at the end you don't know what caused the function to terminate. See
David Korn's recommendations in shell-discuss at opensolaris.org about this
topic.
> In general, boolean functions should communicate their result
> though their exit code, not through echoing 'true' or 'false'.
Agreed.
> tuple_get_port does the work of splitting a port range into pieces,
> but then throws the result away. If it is doing so to test the
> format of the input, then you shouldn't be doing it again later.
>
> port_get_lower/power_get_higher could be simpler:
>
> All callers call both, so you're unnecessarily performing all
> your sanity testing twice and processing twice. Make a single
> function that sets/returns both lport and hport.
>
> Perform a single test for well-formedness (assuming
> tuple_get_port hasn't done so already):
>
> echo $1 | grep '^[0-9]{1,5}-[0-9]{1,5}$'
>
> Then just emit them in the correct order:
>
> echo $1 | ( IFS=- read a b ; [ $a -lt $b ] && echo $a $b || echo $b
> $a )
>
> ipf_get_lock is racy. A simpler and more robust lock file can be
> created by creating a lockfile.$newpid that contains $newpid and
> attempting to ln $IPF_LOCK to your lockfile.
Erm... AFAIK the only portable way (portable across all types of
filesystems, including NFS and DFS) is to use the atomic nature of
"mkdir", e.g. use "mkdir" to create the lock and "rmdir" to release it,
if "mkdir" fails then spin at this lock in incremetally larger time
intervals (e.g 0.1, 0.2, 0.4, 0.8, 1.6 seconds etc.).
[snip]
> process_service:
>
> `something > /dev/null` is a strange construct; I'm not sure what
> you're trying to do, but I know that it will cause some versions of
> ksh93 to crash.
This has been fixed with ksh93-integration update1 which is now being
putback into OS/Net build 106 (April is running final tests with SFWNV
but this should work, after that RTI and putback follows).
----
Bye,
Roland
--
__ . . __
(o.\ \/ /.o) roland.mainz at nrubsig.org
\__\/\/__/ MPEG specialist, C&&JAVA&&Sun&&Unix programmer
/O /==\ O\ TEL +49 641 3992797
(;O/ \/ \O;)
