Karen Tung wrote:
> I would like to have 2 or more reviewers for the following changes.
> These changes involved changes to ICT and installadm-common.sh,
> and many DC files.
> So, perhaps somebody with expertise in those areas can take a look.
>
> 7751 AI image shows warning about /etc/system during boot
> http://defect.opensolaris.org/bz/show_bug.cgi?id=7751
>
> 5416 GRUB shouldn't track nevada build numbers on official releases
> http://defect.opensolaris.org/bz/show_bug.cgi?id=5416
>
> webrev:
> http://cr.opensolaris.org/~ktung/April3/
>
> Testing Done:
> ----------------------
> - Built Live CD image with special title specified. Make sure special
> title is displayed
> in the grub menu. Install system, make sure the installed system have
> the special grub menu.
>
> - Built Live CD image without the special title specified. Make sure
> everything still works
> the same way today, and the resulting system have the grub menu entries
> based on neveda build numbers.
>
> - Built AI X86 image with special title specified. Check to see that
> when image is booted up
> in client, special title is displayed in grub menu. Make sure the
> installed system have the
> special grub menu.
>
> - Built AI X86 image without special title specified. Make sure
> everything works the same way
> today, and the resulting system have the grub menu entries based on
> neveda build numbers.
>
> - Built AI sparc image. To verify 7751 is fixed, make sure that no
> warning is displayed.
>
> Thanks,
>
> --Karen
> _______________________________________________
> caiman-discuss mailing list
> caiman-discuss at opensolaris.org
> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
This looks great Karen!
My comments below. Call me if you would like to discuss.
Thanks, Joe
-------------
usr/src/cmd/distro_const/DC-manifest.defval.xml
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
Looks OK
usr/src/cmd/distro_const/DC-manifest.rng
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
Looks OK
usr/src/cmd/distro_const/DC_defs.py
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
Looks OK
usr/src/cmd/distro_const/ValidatorModule.py
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
Looks OK
usr/src/cmd/distro_const/auto_install/ai_x86_image.xml
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
Looks OK
usr/src/cmd/distro_const/distro_const.py
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
Looks OK
usr/src/cmd/distro_const/slim_cd/all_lang_slim_cd_x86.xml
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
Looks OK
usr/src/cmd/distro_const/slim_cd/slim_cd_x86.xml
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
Looks OK
usr/src/cmd/distro_const/utils/bootroot_archive.py
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
Looks OK
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" ?
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
Issue 1:
--------
176 if [ "XX${grub_title}" == "XX" ] ; then
Just a nit actually. Why use "XX"? Convention seems to be to use "X".
usr/src/cmd/slim-install/svc/live-fs-root
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
Looks OK
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"
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())
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".
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()
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.
usr/src/lib/libtransfer/transfer_mod.py
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
Looks OK
