Hi Tony, Thanks for the review. Below is my response to your comments. Please let me know if you have any additional questions.
-- Quaker ===================================================== REVIEWER: Truong.Q.Nguyen at Sun.COM WEBREV: http://cr.grommit.com/~zf162725/cr_0308/ FILES: Packing/Build/SMF related NOTES: Description of feedbacks: ACCEPT Request accepted REJECT Request rejected EXPLAIN Explanation given DISCUSS Request requires further discussion to resolve DEFER Request deferred (e.g. because work is out-of-scope) ===================================================== usr/src/Makefile - OK usr/src/cmd/Makefile - OK /usr/src/cmd/cmd-inet/usr.lib/Makefile - OK usr/src/lib/libwladm/Makefile.com - OK For the packaging changes, I only look at the SMF side of things since we're not packaging experts :^) Basically, I look at the following - manifests type "f" and use the 'manifest' class - i.manifest and r.manifest included properly. If the service isn't in ON, the packages are constructed to use the bundled i.manifest and r.manifest, rather than creating private copies. - if new service should be enabled on fresh install(in a profile), an appropriate postinstall should be there. - if conversion of an existing service (inetd.conf or init.d script), a combination of preinstall and postinstall scripts may be necessary to ensure the service remains enabled after upgrade. | ACCEPT & EXPLAIN | The wpa service is disabled by default, and will be temporarily crated and | enabled by dladm when user connect to a wpa mode AP. usr/src/pkgdefs/Makefile - looks fine usr/src/pkgdefs/SUNWsupr/Makefile - looks fine usr/src/pkgdefs/SUNWsupr/depend - looks fine usr/src/pkgdefs/SUNWsupr/pkginfo.tmpl - looks fine usr/src/pkgdefs/SUNWsupr/prototype_com - looks fine usr/src/pkgdefs/SUNWsupr/prototype_i386 - looks fine usr/src/pkgdefs/SUNWsupu/Makefile - looks fine usr/src/pkgdefs/SUNWsupu/depend - looks fine usr/src/pkgdefs/SUNWsupu/pkginfo.tmpl - looks fine usr/src/pkgdefs/SUNWsupu/prototype_com - looks fine usr/src/pkgdefs/SUNWsupu/prototype_i386 - looks fine usr/src/cmd/cmd-inet/usr.lib/wpad/svc-wpa Don't you want to include the CDDL header in this file? | ACCEPT | The CDDL header is missed, will be added. - Can you give more explanations as to how both SMF methods and sys-unconfig/sysidconfig use this script? Probably expand on the different arguments? | EXPLAIN | I am sorry for the confusion. the comments of the sys-unconfig/sysidconfig is wrong, | since I write this script based on the copy of sshd. | The wrong comments will be removed, and the svc-wpa script only supports start and stop method | (refresh method is same as start). usr/src/cmd/cmd-inet/usr.lib/wpad/wpa.xml - OK usr/src/lib/libwladm/common/libwladm.c add_new_property - we're not consistent in calling scf_*_destroy functions to clean up here when there's a failure. Assuming the command will exit, you can choose to leave the code as it is or modify such that we do the appropriate cleaning up. | ACCEPT & EXPLAIN | Since the command will exit, we choose to leave the code as it is to make code simple. add_pg_method - calling scf_transaction_reset is redundant here since scf_transaction_destroy* functions effectively calls scf_transaction_reset. Also, looks like the "goto out2" statements in case rv is not 0 can be change to "break" and the out sections can probably be simplified to out: if (trans != NULL) { scf_transaction_destroy_children(tran); scf_transaction_destroy(tran); } if (pg != NULL) scf_pg_destroy(pg); return (status) | ACCEPT create_instance - nits line 2219 - calling "goto out" here would be more consistent. | ACCEPT create_service - this name is a bit misleading since we're not creating a new service but simply setting things up before creating an instance. Is there a reason why the code can't be a part of create_instance(similar to delete_instance)? | ACCEPT | rename create_instance() to do_create_instance() | rename create_service() to create_instance(). - Again, the multiple goto sections can be simplified if we check for the non-null value and call the appropriate destroy function. | ACCEPT - Line 2248 seems unnecessary since we can use SERVICE_NAME definition in scf_handle_decode_fmri call. | ACCEPT wpa_instance_create - Is username always "root"? | EXPLAIN | The wpa service is desinged running as root but with limited privileges(net_rawaccess and sys_net_config). wait_till_to - what happens when max is not assigned a valid value since one of the clauses in the if statement fails? | EXPLAIN | If one of the clauses in the if statement fails, max = DEFAULT_TIMEOUT; delete_instance - again the out sections can be simplified | ACCEPT line 2364 - SERVICE_NAME can be used as a parameter | ACCEPT line 2407 - I don't understand this comment | EXPLAIN | It's wrong comment, removed. The change webrev is at: http://cr.grommit.com/~zf162725/cr_0326/
