Jean McCormack wrote: > > Can I please get a code review for 4488 > > CR: > http://defect.opensolaris.org/bz/show_bug.cgi?id=4488 > > Webrev: > http://cr.opensolaris.org/~jeanm/slim_4488/ > > Jean > _______________________________________________ > caiman-discuss mailing list > caiman-discuss at opensolaris.org > http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
usr/src/cmd/installadm/installadm.c ----------------------------------- nit 1: ------ 783 * do_enable will restart the specified service Should that read "enbale" instead of "restart"? e.g.: * do_enable will enable the specified service nit 2: ------ @ line 850 add a comment to do_disable() to describe the action that specifying the -t will have. usr/src/pkgdefs/SUNWinstalladm-tools/prototype_com -------------------------------------------------- nit: blank lines @ 113 & 120 not needed issue: Don't need line 114. It's done at line 99 114 d none var 755 root sys usr/src/cmd/installadm/server.xml ---------------------------------- Question: --------- Do you want this enabled at install? 40 <create_default_instance enabled='false' /> Question: --------- Why udp6? I don't know, is there udp4 tftp? 52 name='udp6' Question: --------- Do you want 0 for timeouts? Doesn't this mean it will wait forever? 63 timeout_seconds='0' /> 69 timeout_seconds='0' /> usr/src/cmd/installadm/svc-install-server ----------------------------------------- Issue 1: -------- Style thing. In looking at other files in /lib/svc/method it seems different conventions are used. But here is ksh93 how to: http://installzone-wiki.central.sun.com/wiki/index.php/Ksh93_Tips I wouldn't hold this up until it conforms but just so you know in the future. Issue 2: -------- If you are going to define the commands in the style: COMMAND=<path> Add echo to @ line 35 e.g. ECHO=/usr/bin/echo Then user ${ECHO} instead of echo. Issue 3: -------- Using {} around env variables is safer. You can do it for all if you like... See the Ksh93_Tips for guidelines. but for example: 81 status=`$GREP "status=" $i | $SED 's/status=//'` To: tatus=`${GREP} "status=" ${i} | ${SED} 's/status=//'` Issue 4: -------- Maybe you don't need to echo success info. It could fill the log. e.g.: 54 echo "Starting $service_name"
