Hi Karen, Looks good now. Thanks for addressing these.
Joe Karen Tung wrote: > 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 >
