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: [EMAIL PROTECTED]
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/
_______________________________________________
networking-discuss mailing list
[email protected]
