Jean McCormack wrote: > 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 >> >> >> >> >> >> >> >
Jean, Did you update the webrev for me to confirm? It doesn't seem to be updated. Am I looking at the wrong URL? You should commit before making the webrev so the commit comment is in the webrev. always used to forget to do this. ;) I find it does help when one is looking at more than one webrev. ;) usr/src/cmd/installadm/svc-install-server ----------------------------------------------------------------- You don't need the PATH env in if you define the COMMAND=<path> env vars. Joe
