Hi Joe, Thank you for your code review. Please see my responses inline.
The updated webrev is at: http://cr.opensolaris.org/~ktung/April6/ Joseph J VLcek wrote: > > > usr/src/cmd/distro_const/utils/grub_setup.py > +-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-= > > > > The code looks OK. I only have one question about the file. > > Q1. > --- > Should this file be marked: "executable file: mode 755" ? I didn't update the file permission to be 755. It was like that in the gate. But it really shouldn't be 755, I will change it to 644 like all the other *.py files. > > > usr/src/cmd/installadm/installadm-common.sh > +-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-= > > > > > Issue 1: > -------- > > 169 key=`echo $line | cut -d'=' -f1` > 170 if [ ${key} == ${GRUB_TITLE_KEYWORD} ] ; > then > 171 grub_title=`echo $line | cut > -d'=' -f2-` > > Consider using built in extended regular expression pattern matching > > if [[ "${line}" == ~(E)^${GRUB_TITLE_KEYWORD}=.* ]] ; then > grub_title="${line#*=}" > > See: > http://installzone-wiki.central.sun.com/wiki/index.php/Ksh93_Tips > That's a good idea. Changed as suggested. > Issue 1: > -------- > > 176 if [ "XX${grub_title}" == "XX" ] ; then > > Just a nit actually. Why use "XX"? Convention seems to be to use "X". No particular reason to use "XX". Will change to "X". > > > usr/src/lib/libict_pymod/ict.py > +-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-= > > > 153 lines changed: 141 ins; 0 del; 12 mod; 1957 unchg > > Issue 1: > -------- > > Should this file be marked: "mode change: 644 to 755" ict.py shouldn't be 755. I will change it back to 644. > > Issue 2: > -------- > > 1618 prerror("Error in reading " + img_info_file) > > Consider also logging the traceback. e.g.: > > 1618 prerror("Error in reading " + img_info_file) > prerror(traceback.format_exc()) > Fixed as suggested. > Issue 2: > -------- > > 1631 of "Solaris" in grub entry titles with "Open > Solaris". > > Shouldn't this be "OpenSolaris" without the space? : > > 1631 of "Solaris" in grub entry titles with > "OpenSolaris". > Yes, should be "OpenSolaris" without the space. Fixed. > Issue 3: > -------- > > Nit/Suggestion Do what you think is best. > > > I think it might be safest to initialize grub_title=None before > calling get_special_grub_entry. > I realize currently get_special_grub_entry will return or a string but > a possible > future update to get_special_grub_entry might inadvertently not do that. > Initializing grub_title=None before line 1647 might help prevent > future issues. > > > e.g.: > 1646 > 1647 grub_title = None > 1647 grub_title = self.get_special_grub_entry() > Actually, I will prefer to not initialize it. The get_special_grub_entry() will always return a value at this time and the rest of the code depends on what gets returned. So, if get_special_grub_entry() will be updated in the future, the person updating it should pay attention to how it is used in the rest of the file and adjust accordingly. If we initialize it to None, so the rest of the code will flow, we might mask potential problems in get_special_grub_entry(). > Issue 4: > -------- > > Suggestion: > > I think you could the use of the sed command at linse 1723->1725 with > the new python > code you added read the file and write the updates ? > > 1648 if (grub_title != None): > ... > else: > grub_title="OpenSolaris" > > 1680 old_grub_fd = None > 1681 new_grub_fd = None > 1682 try: > ... > > Then the code between lines 1680 and 1721 could be 1 less indentation > level and used > for both cases. Then code code from lines 1722 and 1733 could be removed. > I liked this idea. It simplifies the code, but I had to do it a little differently in order for the thing to work. In the "else" clause, I also need to determine the "release_line" value. What I ended up doing is to have 1680 and 1721 do the generic replacement of values in the grub title. In the above, I will compute the appropriate "old title" and "new title". Such as: if (grub_title != None): new_title = grub_title old_title = the first line of /etc/release else: new_title = "OpenSolaris" old_title = "Solaris" -Do the substitution. Thanks again for the code review. --Karen
