Hi Alok, I have reviewed all the file. I only list the files that I have comments.
** usr/src/cmd/auto-install/config/get_manifest: - line 77: Please check the return value for the "mkdir" call. - line 69: I think an error message about failing to download the given file should be printed. Otherwise, people would be wondering what happened. - lines 81-85: what happened to the indentation? ** usr/src/cmd/auto-install/svc/manifest-locator: - It would be good to move lines 56-108 to be after the do_service_discovery() function. That would make the "main" part of the script easier to read. - lines 78-84, and lines 102-106 is identical code, would be good to put it in a function. - lines 86-87 and lines 99-100 is also identical code, would also be good to put it in a function, even though it is 2 lines. It makes it more extensible in case anything need to be added to it in the future. - Question: why have a separate /usr/sbin/get_manifest program? Can't the code from get_manifest be a function in this script? - /usr/share/auto_install/default.xml and /var/ai/default.xml is hard coded all over the code. Please consider defining them as constants in 1 spot. That way, if these 2 values were to change in the future, we just need to fix it in 1 place. ** usr/src/cmd/auto-install/svc/manifest-locator.xml - the copyright year is not correct ** usr/src/cmd/distro_const/auto_install/Makefile - ai_plat_setup.py is a python script. In addition to just including it in the list of file here, please try to compile it at build time, so, syntax errors can be caught. See usr/src/cmd/distro_const/utils/Makefile for an example of what is done for the other python finalizer scripts. ** usr/src/cmd/distro_const/slim_cd/slim_cd_x86.xml - changes in this file also need to be made to all_lang_slim_cd_x86.xml. Also, please build and test the image from using all_lang_slim_cd_x86.xml. ** usr/src/cmd/slim-install/svc/live-fs-root - line 109: need to check for the return value from lofiadm explicitly, and exit, instead of using " || break" - line 140-148: These few lines won't ever get executed, since "MOUNTED" is set to 1 in line 123, and there's no code to reset it between 123 and 140. Actually, MOUNTED doesn't seem to be used anywhere else in the code, perhaps it is no longer needed? - line 130-138, would it make more sense to move these few lines to be with other code that does keyboard setting? Perhaps in line 138? ** usr/src/cmd/slim-install/svc/live-usr-fs.xml - need to update copyright ** usr/src/cmd/slim-install/svc/net-fs-root - Since this whole script is for AI over the net, I don't think line 36, line 65 and line 67 is necessary. The /.autoinstall file is already checked in line 55. - The variable MOUNTED is not necessary, as well as lines 278-284 Thanks, --Karen Alok Aggarwal wrote: > > On Fri, 11 Sep 2009, Alok Aggarwal wrote: > >> This is a code review request for the Bootable AI project [1]. > > [ .. ] > >> e) Changes to auto-install and the transfer module to have a >> built in timeout for the initial 'pkg' contact >> >> The last one (e) shows up in the webrev but still needs more >> work and testing. I will send a follow on webrev some time next >> week that has (e) in a more finished state. >> >> The webrev location is - >> >> http://cr.opensolaris.org/~aalok/bootable.ai/ > > I have updated the webrev with finished changes for (e), the > last remaining item. > > Please provide your review comments by Thursday, 9/17, 12pm PT. > > Alok > _______________________________________________ > caiman-discuss mailing list > caiman-discuss at opensolaris.org > http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
