David Powell wrote:
> Tony Nguyen wrote:
>> Dave,
>>
>> See my responses inline and the updated webrev at:
>>
>> http://cr.opensolaris.org/~tonyn/firewall13Jan2009-inc/
>> http://cr.opensolaris.org/~tonyn/firewall13Jan2009/
>
> I'm still working through ipf_include.sh.
... and servinfo.c. Here's the rest:
servinfo.c:
uaddr2port:
If there's only one dot in the provided address, you'll scan past
the beginning of your buffer (and could end up writing past the end
of 'port_str'). You need to ensure 'p' doesn't advance past
'addr'.
Since the string pointed to by 'p' is 'l' bytes long, after the
strncpy 'port_str' won't neccesarily be NUL terminated, which could
cause atol to fail.
svc_getrpcinfo:
I'm still wondering where the "12" in "buf[12]" comes from. It
should either be documented, or a constant (one hopefully derived
from whatever determines the length of the buffer).
You now silently exclude protocols or ports that don't fit on the
line. I suppose it is unlikely, but I'm wondering if that was a
concious choice.
printf("%s\n", str) is more simply written as puts(str);
ipf_include.sh:
Don't use &&/|| with { }; use if/then instead.
Instead of
command && return 0 || return 1
when command returns 0 or 1, do
command
return $?
inst_ipf_file:
inst_nat_file:
Instead of echoing the result of fmri_to_file to concatenate a
suffix, pass the suffix as an argument to fmri_to_file to append
for you. Then just call fmri_to_file.
(On further inspection, inst_nat_file doesn't appear to be used,
and other uses of inst_ipf_file have been replaced per my previous
comments. They should just be removed.)
service_check_state:
Instead of
while true
condition && break
sleep
do
while !condition
sleep
file_get_ports:
get_active_ports:
'/to.*port =/ s/\(.*to.* port =\) \([a-z0-9]*\).*/\2/p'
can be simplified to
's/.*to.* port = \([a-z0-9]*\).*/\1/p'
since 'p' will only print if a substitution is made, and the
pattern is more restrictive than the address.
prepend_new_rules:
I don't think it's necessary to escape '@'.
tuple_get_port:
There's no need to escape ':' in 's/.*\://''
tuple_get_proto:
There's no need to escape ':' in 's/\:.*//''
Use if/then/else here; it will be clearer.
ipf_remove_lock:
Why do you remove the lock in two steps instead of just 'rm -r' as
you do in ipf_get_lock?
Instead of 'while [ true ]' use 'while :'.
replace_file:
Why not always move?
process_server_svc:
All callers test for IPF_FMRI before calling this function. You
don't need to test it again.
All callers test service_is_enabled before calling this function.
You don't need to test it again.
process_nonsvc_progs:
Instead of using port_is_range, just 'set -- $port' and test $#.
create_services_rules:
Instead of scraping the output of svcs, it is probably more stable
and more efficient to get the list of services using:
svcprop -cf -p general/enabled -p general_ovr/enabled '*' 2>/dev/null |
sed -n 's,^\(svc:.*\)/:properties/.* true$,\1,p' | sort -u
Since your 'service_is_enabled' predicate determines if the service
is enabled *or* temporarily enabled, with the above you can get rid
of that test from your loop.
service_update_rules:
I missed that you don't install rules for services that are in
maintenance. If this is the case, the decision to install rules
for services that are temporarily disabled doesn't really make
sense. (I'd still replace the use of svcs, though.)
service_update:
create_services_rules:
Both test the result of ipf_get_lock, but afaikt it will always
succeed.
Dave
