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 ? > > > 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
