> -------- 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 -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://mail.opensolaris.org/pipermail/caiman-discuss/attachments/20100324/a3f308f6/attachment-0001.html>
