I have another concern regarding location-based IPfilter configuration. The current implementation requires users to supply a customized ipf rules for every location if they want to configure IPfilter. This essentially undo the functionality of Host-based firewall which simplified IPfilter configuration. We ought to allow users to configure location with the option of using their existing IPFilter configuration, in fact, I'd argued that option should be the default.
-tony Tony Nguyen wrote: > Michael and team, > > The SMF related changes are good in general. I have just a few comments. > > General: > > The integration of 6855845, Allow Property Modification in SMF profiles, > into b119 should be sufficient for NWAM phase next to quickly update > location specific values and simplify logic in various services and > their inter-relationships. > > Is there a reason why /etc/svc/volatile is used instead of /var/run or a > non-persistent pg? > > File specific comments: > usr/src/cmd/svc/shell/ipf_include.sh - ok > usr/src/cmd/ipf/svc/ipfilter.xml - ok > usr/src/cmd/svc/milestone/net-svc - ok > usr/src/cmd/svc/milestone/network-netmask.xml - ok > usr/src/cmd/svc/milestone/network-netcfg.xml - ok > usr/src/cmd/svc/milestone/network-ipqos.xml - ok > usr/src/cmd/svc/milestone/net-netmask - ok > usr/src/cmd/svc/milestone/net-ipqos - ok > > usr/src/cmd/cmd-inet/lib/nwamd/ipf6.conf.dfl > usr/src/cmd/cmd-inet/lib/nwamd/ipf.conf.dfl > > Comments on line 3 and 16 suggest these files aren't the final version. > > usr/src/cmd/svc/milestone/network-service.xml - No update? However, are > dependencies (e.g. NIS dependencies) and comments still accurate if the > service is now only responsible for updating DNS information? > > usr/src/cmd/svc/milestone/network-physical.xml - > > 41 & 87 - It's not clear from the design spec that there will be > additional network/physical instances. Clarification please? > > line 74 - unnecessary diff? > > usr/src/cmd/svc/milestone/network-location.xml > > 100 -113 manifest-import is defined as a dependent but the comment > states it should be a dependency. Has testing been done to verify > correct behavior? > > usr/src/cmd/svc/milestone/net-loc > usr/src/cmd/svc/milestone/net-svc > usr/src/cmd/svc/milestone/net-physical > usr/src/cmd/svc/milestone/net-nwam > > These files have similar implementation to figure out whether a service > is enabled. It's probably better to have define a single > service_is_enabled() in net_include.sh that these three files can consume. > > create/revert_legacy_loc doesn't correctly save/restore custom file. > User may have a policy other than custom but has a valid configuration > file. The current code doesn't correctly save the configuration file if > policy is something other than 'custom' thus can't properly restore it. > > usr/src/cmd/svc/milestone/net-loc > > 501 - clearing custom policy_file is probably unnecessary since policy > is set to 'none'. > 502 - refresh_ipf also needs to be set here to make the changes effective. > > stop_svc() - in all uses of this function, no configuration change was > made to the target service so a refresh isn't necessary. > > refresh_svc() - The name seems incomplete as the function really is > refreshing/starting the service with the updated configuration. Perhaps > run_svc? > > -tony > >>> >>> Date: Mon, 06 Jul 2009 15:36:11 -0700 >>> From: Michael Hunter <Michael.Hunter at Sun.COM> >>> To: nwam-dev at opensolaris.org >>> Subject: [nwam-dev] NWAM Phase 1 Code Review request >>> >>> >>> This is a request for people to review the NWAM Phase 1 code. >>> >>> The code can be found at http://cr.opensolaris.org/~mph/nwam1_cr/ >>> >>> For those inside sun there is a cscope database >>> in /net/coupe.eng/builds/mph/nwam1_cr/usr/src (sorry to those external >>> to SWAN). >>> >>> Documentation can both be found in the source tree and on the project >>> web page at http://opensolaris.org/os/project/nwam/ >>> >>> We request code review be finished by Monday July 20th. >>> >>> Michael Hunter >>> _______________________________________________ >>> nwam-dev mailing list >>> nwam-dev at opensolaris.org >>> http://mail.opensolaris.org/mailman/listinfo/nwam-dev >
