I don't see why not. Sue
On 03/10/09 06:22, Jean McCormack wrote: > Done. > > Do you know if I'm allowed to push? > > Jean > > Susan Sohn wrote: >> 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 >> >
