On 03/09/09 18:54, Jean McCormack wrote: > Webrev is now updated. Sorry it took so long. I wanted to get everyone's > requested changes in, have dinner, > and do some testing. > > Here's the link: > > http://cr.opensolaris.org/~jeanm/slim_4488_2/
>> In installadm.c:do_disable, you need to add a check to make sure the >> user has entered a service name. > Done. When you print the usage, to be consistent with other subcommands, you can just print the usage for disable. See how it's done in the enable command. Sue > Jean > > Joseph J. VLcek wrote: >> 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 > > _______________________________________________ > caiman-discuss mailing list > caiman-discuss at opensolaris.org > http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
