Jean, svc-install-server - these changes don't completely capture what I commented on. I looked, and we do deliver a skeleton AI_HTTPD_CONF file, so checking for its existence, though necessary for the invocation of apache to be sane, isn't enough. We need to maybe set a flag in the loop that processes install services, after line 59, to indicate that we've indeed started at least one install service. Only then should we run our apache process, right? Or is there any other reason to start the apache process when they system isn't serving any install services?
-ethan 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/ > > 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
