Hi Joe,
On 03/09/09 14:19, Joseph J VLcek wrote: > Looks good Jan. Thanks a lot for review ! > > From the comments on each file in the webrev it looks like you may > need to do a hg recommit. Yeah - I did 'recommit' right before the final push. > > 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. No problem at all :-) I have filed following bug for tracking this: 7214 desire for auto-installer SMF script following ksh93 programming recommendations When filing the bug, I have realized that those recommendations are available only internally - I am thinking if we might consider to make them publicly available. Thanks again ! Jan > > 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 >> >
