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
