Hi Joe, I am having trouble following the code as it is written, even with the comments. At least, the comments need to be improved. From my understanding of the problem that need to be solved, and reading the existing code many times, I *think* the code is doing the following.
-------------- Find the first entry in the menu.lst file that boots the installed rpool, and enable Happy Face Boot for that entry. The original content of the entry will be saved to create an additional entry that doesn't have Happy Face Boot enabled. Comments and white space from the original entry will not be included in the additional entry. If more than one entry exist in the original menu.lst file, only the first entry will be modified to enable Happy Face Boot. -------------- Other comments I have about the code. 1) You don't need to have 2 "try". You can just have 1 try with with multiple "except" and the "finally" 2) You seem to use "END BOOTADM" as the indication of end of the entry. I don't think that's a good idea, since that's just a comment. I think one entry should go from one title line to the next title line. 3) The way it is coded now, the "text boot" entry will be the last one in the menu.lst instead of immediately following the line that we have enabled Happy Face Boot. Is that the desired behavior? I would think that it makes more sense to have it immediately follow the Happy Face boot line. Thanks, --Karen Joseph J VLcek wrote: > Hello, > > Can two people please do a code review for bug: > > 4673 Grub text only mode is required > http://defect.opensolaris.org/bz/show_bug.cgi?id=4673 > > > The webrev is available at: > http://cr.opensolaris.org/~joev/bug4673/ > > This fix involves a rewrite of ICT enable_happy_face_boot > > I also fixed some begin and end comment block issues that were causing > vim to display some of the code in the color of a comment. > > * The modules affected and tested: > > ict.py > > * Testing done on logic > > I wrote a stand alone wrapper around the changes so they could be > exercised outside of an installation environment. > > I tested: > > - a menu.lst file with multiple entries > - a menu.lst file with a single entry > - a menu.lst file with embedded comments > - a non-existing menu.lst file, reports an error > - a read only menu.lst files, reports an error > > * Testing done for GUI Install > > I booted a 101 live Image and used mount -F lofs to applied the updated > version of ict.py > > After the installation I performed reboots selecting the default before > the timeout, I let the timeout select the default and I selected the > text boot entry > > * Results: > > The installation completed successfully and the menu.lst file contained > the default graphical boot entry and a second, text boot entry and both > entries provide the expected behavior. > > * Testing done for AI > > No AI testing was performed. > > > Thank you, > Joe > > _______________________________________________ > caiman-discuss mailing list > caiman-discuss at opensolaris.org > http://mail.opensolaris.org/mailman/listinfo/caiman-discuss >
