Hi Dave, Thanks for the comments. I'm wrapping up some other stuff and will get back to this as soon as I'm done.
-tony 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/ > > 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 >
