Hi Joe, Please see my responses below.
Joseph J VLcek wrote: > Karen, > > Thank you for your feedback. I appreciate it. > > Karen Tung wrote: >> 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. >> -------------- > > That is correct. I can add similar text as a comment. > > >> >> 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" > > I check with Jack Schwartz on this. I turns out for python 2.4 the > extra level is needed. > I just wrote a little test program to check. You are right that it does need the extra level. I am still surprised that we need the extra level. I guess I was thinking about Java. > >> >> 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. > > "END BOOTADM" is not being used as the entry indication. The comment > line containing "END BOOTADM" is saved and then rewritten to the end > of the file. > end of the file? I guess that's where I got confused. I assume that you just write it to the end of the happy face boot entry. Why write it to end of the file? > The title line is being used to indicate the next entry. > > >> >> 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. > > I purposely put the "text boot" entry at the bottom. > > > Currently there is only one entry. I had initially coded this fix to > rely on that but I felt it would be safer to make the code a little > more robust. > > In the near future the extra "text boot" entry will not be needed > because support to interrupt the graphical boot using keyboard input > will provide visibility to the boot messages. > OK, this makes sense. Thanks, --Karen > >> >> Thanks, >> >> --Karen > > > Huge thanks Karen! > > Joe > > >> >> >> 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 >>> >> >
