Looks good Jan. From the comments on each file in the webrev it looks like you may need to do a hg recommit.
I think you can push and file a different bug/RFE for the overall shell issues. I am sorry I didn't make it clear in my initial post that this should not be a blocking issue for this fix. My comments below. Joe jan damborsky wrote: > Hi Joe, > > thank you very much for your comments ! > Please see my response in-line. > > Jan > > > On 03/08/09 16:05, Joseph J. VLcek wrote: >> Looks good Jan. I have a couple of comments below and a couple of nits. >> >> Hope this helps! >> >> Joe >> >> jan damborsky wrote: >>> On 03/03/09 19:12, jan damborsky wrote: >>>> Hi, >>>> >>>> could I please ask for reviewing changes for following bug ? >>>> >>>> 6556 AI should provide an option for automatic reboot after an install >>>> http://defect.opensolaris.org/bz/show_bug.cgi?id=6556 >>>> >>>> webrev: >>>> http://cr.opensolaris.org/~dambi/bug-6556/ >>> >>> >>> Sue pointed out that links are mangled - here are >>> the correct ones (hopefully :-) >>> >>> webrev: >>> http://cr.opensolaris.org/~dambi/bug-6556/ >>> >>> bug: >>> http://defect.opensolaris.org/bz/show_bug.cgi?id=6556 >>> >>> My apologies for the inconvenience, >>> Jan >>> >>> _______________________________________________ >>> caiman-discuss mailing list >>> caiman-discuss at opensolaris.org >>> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss > > [...] > >> >> >> usr/src/cmd/auto-install/auto_install.c >> ------------------------------------------------- >> Issue 1: >> -------- >> 1212 strcmp(ai_auto_reboot, "true") == 0) { >> >> Should this be case insensitive? Perhaps user strcase() >> or maybe strncasecmp() > > Thanks for catching this - I have changed it to strcasecmp(), > since we compare with constant string. > >> >> Issue 2: >> -------- >> >> I think a log message should only be displayed if >> the ai_manifest/ai_auto_reboot is in the manifest. >> >> If the user does not specify a value for ai_auto_reboot in the manifest >> it might be confusing to them to see a log message stating no reboot >> will take place... >> >> The log message generated at lines 1239/1240 should be sufficient in >> this case. >> >> Suggested change from: >> >> 1211 if ((ai_auto_reboot != NULL) && >> 1212 strcmp(ai_auto_reboot, "true") == 0) { >> 1213 auto_log_print(gettext("Auto reboot enabled\n")); >> 1214 >> 1215 auto_reboot_enabled = B_TRUE; >> 1216 } else { >> 1217 auto_log_print(gettext("Auto reboot disabled\n")); >> 1218 } >> >> >> To: >> >> if ((ai_auto_reboot != NULL) { >> if ( strcase(ai_auto_reboot, "true") == 0) { >> auto_log_print(gettext("Auto reboot enabled\n")); >> >> auto_reboot_enabled = B_TRUE; >> } else { >> auto_log_print(gettext("Auto reboot disabled\n")); >> } >> } > > Done. > >> >> usr/src/cmd/auto-install/auto_install.h >> ------------------------------------------------- >> >> Total nit and not necessary. >> >> Moving comment to end of line looks clearer to "me" and it fits on the >> 80 char line. You decided. >> >> Suggested change from: >> 40 /* AI engine exit codes */ >> 41 /* success - control passed to user */ >> 42 #define AI_EXIT_SUCCESS 0 >> 43 /* success - auto reboot enabled */ >> 44 #define AI_EXIT_AUTO_REBOOT 64 >> 45 /* failure */ >> 46 #define AI_EXIT_FAILURE 1 >> >> To: >> /* AI engine exit codes */ >> #define AI_EXIT_SUCCESS 0 /* success - control passed to >> user */ >> #define AI_EXIT_AUTO_REBOOT 64 /* success - auto reboot enabled */ >> #define AI_EXIT_FAILURE 1 /* failure */ > > Done. > >> >> usr/src/cmd/auto-install/svc/auto-installer >> ------------------------------------------------- >> >> I realize this is not ksh93 but in general the guidelines found under >> could be useful: >> http://installzone-wiki.central.sun.com/wiki/index.php/Ksh93_Tips > > Thanks for pointing this out - I have realized there are > likely couple of things which might be changed in auto-installer > script in order to comply with those recommendations. > Are you fine with filing separate bug for this ? Yes. I am sorry I didn't make it clear in my initial post that this should not be a blocking issue for this fix. > >> >> >> Issue 1: >> -------- >> >> Lines: 110/111, 126/127, 149/150 >> >> could benefit from the command "tee" for posting the same message to >> the terminal and a file. >> >> >> e.g. >> sh-3.2$ echo "hi" | tee message.txt >> hi >> sh-3.2$ cat !$ >> cat message.txt >> hi > > Good point - I have changed this to take advantage of tee(1). > >> >> Issue 2: >> -------- >> Lines: 202/203, 205/206, 217/218, 231/232. >> >> You can avoid having the text start in col 0 by quoting >> the end of the first line prior to the continuation char. >> >> e.g.: >> Change from: >> 202 echo "Please refer to /tmp/install_log file \ >> 203 for details" > /dev/msglog >> >> To: >> echo "Please refer to /tmp/install_log file" \ >> "for details" > /dev/msglog > > Done. > >> >> Issue 3: >> -------- >> >> Nit/wording (and same issue as above) >> >> Change from: >> "Automated reboot enabled, the system will be rebooted now" >> >> To: >> "Automated reboot enabled. The system will be rebooted now. >> >> >> e.g.: >> >> 217 echo "Automated reboot enabled. The system will be" \ >> 218 "rebooted now." > /dev/msglog > > Done. > > Could you please take a look at updated webrev ? > http://cr.opensolaris.org/~dambi/bug-6556-joe/ > > > Thanks again ! > Jan >
