Joseph J VLcek wrote: > 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 Done.
> > nit 2: > ------ > > @ line 850 add a comment to do_disable() to describe the action that > specifying the -t will have. > Done. > > usr/src/pkgdefs/SUNWinstalladm-tools/prototype_com > -------------------------------------------------- > > nit: > blank lines @ 113 & 120 not needed Done > > issue: > Don't need line 114. It's done at line 99 > 114 d none var 755 root sys > Gone. > usr/src/cmd/installadm/server.xml > ---------------------------------- > > Question: > --------- > > Do you want this enabled at install? > > 40 <create_default_instance enabled='false' /> I think not. > > Question: > --------- > > Why udp6? I don't know, is there udp4 tftp? > > 52 name='udp6' OpenSolaris appears to ship udp6 and not 4. > > Question: > --------- > > Do you want 0 for timeouts? Doesn't this mean it will wait forever? > > 63 timeout_seconds='0' /> > 69 timeout_seconds='0' /> Yes. The issue is that we loop through potentially hundreds of services on start and stop. That means doing an installadm enable of each service on start and a installadm disable -t on stop. This can take quite a long time. There isn't really a value I could pick that would not potentially cause a false error. > > > > > 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 Done > > 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=//'` Done. > > > Issue 4: > -------- > Maybe you don't need to echo success info. It could fill the log. > e.g.: > 54 echo "Starting $service_name" Removed those. Thanks for the review. Jean > > > > > > >
