Hi Karen, On Wed, 16 Sep 2009, Karen Tung wrote:
> Hi Alok, > > I have reviewed all the file. I only list the files that I have comments. Thanks for doing the review! > ** usr/src/cmd/auto-install/config/get_manifest: > > - line 77: Please check the return value for the "mkdir" call. Will do. > - line 69: I think an error message about failing to download the given file > should be printed. Otherwise, people would be wondering what happened. If there's a download failure or something, the error does get printed. I've also removed the redirection to /dev/null. > - lines 81-85: what happened to the indentation? Restored. > ** 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. Jan had the same comment, done. > - lines 78-84, and lines 102-106 is identical code, would be good > to put it in a function. Good idea, will do! > - 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. Will change. > - Question: why have a separate /usr/sbin/get_manifest program? > Can't the code from get_manifest be a function in this script? There's no reason it can't but it's cleaner to have it as a separate entity that can evolve independant of the manifest-locator service itself. > - /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. Will do. > ** usr/src/cmd/auto-install/svc/manifest-locator.xml > > - the copyright year is not correct Will change. > ** 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. Will change that, I also saw your comment about letting it be a .py file in the other email and I agree with you. > ** 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. Done. > ** 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" Will change. > - 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? It looks like dead code to me too, I'll yank it out. > - 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? Did you mean line 128 instead? I've moved 130-138 to be up on line 128 instead. > ** usr/src/cmd/slim-install/svc/live-usr-fs.xml > > - need to update copyright Will change. > ** 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 Will change. Alok
