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
