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/
Thank you, thank you, thank you for creating scf_instance_delete_prop.
I'm still working through ipf_include.sh.
nfs-server:
What's up with waiting for mountd for a second? Is that even
effective?
milestone/global.xml:
exceptions description:
Use the same grammar used in the "apply_to" description. i.e.
The host and network IPs, network interfaces, and ippools to
exempt from the set policy. That is, those to accept if the
policy is set to deny, or to deny if the policy is set to
accept.
svcadm.c:
In some places you uu_die if my_ctname fails, in others you uu_warn.
The error handling should be consistent.
Though it is unlikely that your error messages will ever be seen by
the user, if they're worth localizing they're also worth making more
meaningful to a non-developer. e.g. instead of
"Failed to process %s: restarter_setup() failed"
from which, nonintuitively from the reader's perspective, you
continue, something like this would be better:
"Unable to record FMRI with request. svcs -l output may be
incomplete."
yp.sh:
Should there be an exit after the call to create_client_ipf_rules?
create_client_ipf_rules appears to mishandle multiple IP address for
a single host name.
If the host name is a substring of another, you'll get erroneous
results.
If IPv6 addresses are included in your hosts file, I think your
implementation will malfunction.
rfc1179.xml:
ipp-listener.xml:
ipf_method should refer to a full FMRI, not just "print/server".
ipfilter.xml:
Now that you are starting a persistent daemon, this shouldn't be a
transient service. (Arguably this is an existing bug given ipmon).
svc/ipfilter:
When you start, there shouldn't be a svc.ipfd to kill.
Instead of pgrepping, use smf_kill_contract in the stop method.
What does the 'reload' subcommand do, and where is it used and
documented?
It may seem hacky, but I think I'd prefer having the default policy
be empty, and using that default state as an indicator that our
initial boot hasn't yet occurred. An explicit "upgraded" property
that gets set once and sticks around forever is unappealing.
Additionally, it's one more thing that a profile creator will need to
know to set to avoid their settings being overwritten on first boot
(e.g. in a jumpstart-like environment).
ipfd.c:
repository_event_wait:
Is the variable 'scratch' still needed?
is_correct_event:
The seven nearly identical blocks after determining you're dealing
with SCF_PG_RESTARTER_ACTIONS could be reduced to a loop through an
array of property types to test (you would need two arrays: one for
when you're not in_maintenance, and one for all the time).
Dave
