On 03/24/10 01:15 PM, Joseph J VLcek wrote: > On 03/24/10 04:03 PM, Tony Nguyen wrote: >> >>> -------- Original Message -------- >>> Subject: Re: [caiman-discuss] DC code review - urgent >>> Date: Wed, 24 Mar 2010 13:56:29 -0400 >>> From: Joseph J VLcek <Joseph.Vlcek at sun.com> >>> To: caiman-discuss at opensolaris.org >>> >>> >>> >>> On 03/24/10 04:04 AM, Karen Tung wrote: >>> > Please review my changes for the following bug: >>> > >>> > *Bug >>> 15148*<http://defect.opensolaris.org/bz/show_bug.cgi?id=15148> - >>> > Required DC updates to work with Early Manifest Import project >>> changes >>> > http://defect.opensolaris.org/bz/show_bug.cgi?id=15148 >>> > >>> > I would like to integrate my changes by COB of Wed 3/24, if >>> possible. >>> > Therefore, please try to send your comments by 2pm, pacific time. >>> > >>> > webrev: >>> > http://cr.opensolaris.org/~ktung/15148/ >>> > >>> > Testing: >>> > >>> > The changes are provided by the SMF team, and they have built >>> > LiveCD and AI images and tested them successfully. >>> > >>> > I took their changes and did the following tests so far: >>> > - I built Slim CD, x86 AI, and sparc AI images using a build 135 >>> repo. >>> > This verified >>> > the changes can be used to build pre-EMI images successfully. >>> > - I also built Slim CD, x86 AI, and sparc AI images using a repo >>> > containing all >>> > the EMI code. This verified the changes can be used to build >>> post-EMI >>> > images successfully. >>> > - I verified that all the pre-EMI images can be booted correctly, >>> and >>> > they install >>> > correctly. >>> > - I verified the post-EMI slim cd image is able to boot and install >>> > correctly. >>> > >>> > I will be performing the following tests Wed morning, and will >>> not integrate >>> > until I confirm that they all pass. >>> > - Build x86 Text Installer and sparc Text installer images using a >>> > pre-EMI change repo, >>> > and make sure the images can boot and install correctly. >>> > - Build x86 Text Installer and sparc Text installer images using a >>> > post-EMI repo, >>> > and make sure the images can boot and install correctly. >>> > - Verify the post-EMI sparc and x86 images are booting and >>> installing >>> > correctly. >>> > >>> > Thanks, >>> > >>> > --Karen >>> > >>> > >>> > >>> > >>> > >>> > >>> > _______________________________________________ >>> > caiman-discuss mailing list >>> > caiman-discuss at opensolaris.org >>> > http://mail.opensolaris.org/mailman/listinfo/caiman-discuss >>> >>> Hey Karen, >>> >>> Issue 2 and 7 should be addressed. >>> >>> The others are nit but I think would be nice to have. >>> >>> Joe >>> >>> >>> >>> Issue 1 (nit): >>> -------- >>> usr/src/cmd/distro_const/utils/boot_archive_configure >>> >>> >>> Total nit: maybe not all that important but that I would point it >>> out... >>> >>> Use "==" not "=" >>> >>> 260 [ "${emi}" = "true" -a ${etc_profile} -ne 0 ]&& continue >>> 261 [ "${emi}" = "false" -a ${etc_profile} -eq 0 ]&& continue >>> >>> >>> From ksh93(1): >>> string = pattern Same as ==, but is obsolete. >>> >>> >>> Issue 2: >>> -------- >>> >>> http://installzone-wiki.central.sun.com/wiki/index.php/Ksh93_Tips >>> # 9 use true and false ksh93 builtins >>> >>> I think this could work but it is wrong and should be changed. >>> >>> 260 [ "${emi}" = "true" -a ${etc_profile} -ne 0 ]&& continue >>> 261 [ "${emi}" = "false" -a ${etc_profile} -eq 0 ]&& continue >>> >>> You should not check against the strings "true" and "false". You should >>> check against the builtins true and false >>> >>> e.g.: >>> >>> 260 [ ${emi} -a ${etc_profile} -ne 0 ]&& continue >>> 261 [ ${emi} -a ${etc_profile} -eq 0 ]&& continue >>> >>> Issue 3 (nit): >>> ------- >>> >>> 259 etc_profile=$? >>> >>> etc_profile should be an integer >>> >>> e.g.: >>> >>> typeset -i etc_profile at the top of the program >>> >>> >>> Issue 4 (nit): >>> -------- >>> usr/src/cmd/distro_const/utils/mkrepo >>> >>> nit: >>> >>> 120 rm -f $logf >>> >>> rm should be defined as a builtin before being used. >>> >>> e.g. >>> >>> add this line to the top of the file: >>> >>> builtin rm >>> >>> Issue 5 (nit): >>> -------- >>> >>> Same issue with grep, cat and echo >>> >>> 129 grep "smf(5) service descriptions failed to load" >>> $logf> /dev/null 2>&1 >>> >>> >>> Issue 6 (nit): >>> -------- >>> >>> fgrep might be a better choice then grep >>> >>> Issue 7: >>> -------- >>> >>> http://installzone-wiki.central.sun.com/wiki/index.php/Ksh93_Tips >>> # 4 Use "print" and "printf" in place of "echo" >>> >>> >>> >>> Issue 8 (nit): >>> -------- >>> >>> usr/src/cmd/slim-install/svc/live_fs_include.sh >>> >>> Nit: >>> >>> use >>> >>> builtin cd >>> >>> >> >> Hi Joseph, >> >> Thank you for your comments. Many of the changes here correspond to code >> in ON's manifest-import script. I think there's value in keeping the >> code parity to ease maintenance. The current code *works* and is >> well-tested, with the current time constraint I really don't want to >> cause additional test burden on Karen. If you really insist, Karen and I >> will make the suggested changes. >> >> Thanks, >> tn > > > Tony > > Karen has told me she has made the changes to archive_configure and > live_fs_include.sh and that you would be responding to my comments for > mkrepo. > > > My feedback for mkrepo was only style convention/nits. It is OK if you > the urgency of this leads you to forgo them. > > Joe > And mkrepo re-uses code from ON's manifest-import script so it makes sense to leave them intact as much as possible. Thank you for your understanding.
-tn
