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
