Ethan Quach wrote: > 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? My thinking was that this was similar to tftp running and dns running in that it's a process we will need and starting it up is part of initializing the system for us. i.e. the things we need will be up and running.
Jean > > > -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
