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/ai-webserver/ai_schema.rng ------------------------------------------------- Looks OK usr/src/cmd/ai-webserver/default.xml ------------------------------------------------- Looks OK usr/src/cmd/auto-install/ai_manifest.rng ------------------------------------------------- Looks OK usr/src/cmd/auto-install/ai_manifest.xml ------------------------------------------------- Looks OK 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() 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")); } } 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 */ usr/src/cmd/auto-install/auto_parse.c ------------------------------------------------- Looks OK 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 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 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 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
