Great Jan! I have one comment below.
Joe Jan Damborsky wrote: > Hey Joe, > > > Joseph J. VLcek wrote: >> Jan Damborsky wrote: >>> Hi Ethan, Sundar >>> >>> could I please ask one of you guys to review the fix for following >>> bug ? >>> Other reviewers are also welcome. >>> >>> 9549 Sparc AI client doesn't bring up network if not booted using DHCP >>> >>> * Webrev: >>> http://cr.opensolaris.org/~dambi/bug-9549 >>> >>> Short story is that this fix add support for following scenarios: >>> >>> * Sparc Automated Installation without need for DHCP server >>> (making Sparc AI more 'WAN') >>> * Automated Installation of Sparc clients without wanboot support >>> in OBP (wanboot binary loaded from media instead) >>> >>> Please see bug report for more details. >>> >>> Alok, as we were discussing during MPK work week, these modifications >>> might affect bootable AI image (particularly refactoring SMF services >>> part). >>> Could you please take a look at those changes just to verify if there >>> are any pieces which might be source of potential issues ? >>> >>> Thank you very much, >>> Jan >>> >>> >>> Modules affected and tested: >>> * Distro Constructor (ai_sparc_image.xml - Sparc AI manifest) >>> * AI client (live-fs-root) >>> * installadm(1M) tools (set up for Sparc install.conf) >>> >>> tests carried out: >>> * please see attached file for tests carried out >>> and test results >>> ------------------------------------------------------------------------ >>> >>> >>> _______________________________________________ >>> caiman-discuss mailing list >>> caiman-discuss at opensolaris.org >>> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss >> >> Hey Jan, >> >> Looks good. Just a couple of comments >> >> I see you have Alok on the cc list. Make sure you connect with him as >> I know he is reworking some of this code for the bootable AI image. > > I will do - At this point I plan to postpone the push until Alok's > code is integrated. > >> >> Joe >> >> - - - >> >> >> >> usr/src/cmd/slim-install/svc/live-fs-root >> --------------------------------------------------------------- >> >> Line 396: >> >> The netbootinfo return code is not checked here. The command could >> have failed but the logic assumes it did not. I see there is an final >> else clause which will handle this but perhaps logging that the >> netbootinfo command failed, if it did, in that else clause might be >> helpful for future diagnostics. > > You are right. Taking into account Dave's comment as well, > if netbootinfo fails or doesn't recognize network strategy, the > script will abort and user will be informed. > >> >> >> usr/src/cmd/installadm/setup-sparc.sh >> ------------------------------------------------------------- >> >> 97 dns_servers=`$GREP "$NAMESERVER_STRING" $RESOLV_CONF_FILE >> 2>/dev/null | \ >> 98 $AWK '{printf("%s ", $2)}'` >> 99 dns_domain=`$GREP "$DOMAIN_STRING" $RESOLV_CONF_FILE >> 2>/dev/null | \ >> 100 $HEAD -1 | $AWK '{printf("%s", $2)}' >> >> This code could benefit from the ksh93 (/sbin/sh) built in extended >> regular expression pattern matching. For examples see: >> >> http://installzone-wiki.central.sun.com/wiki/index.php/Ksh93_Tips#Use_built_in_extended_regular_expression_pattern_matching >> >> > > I am not sure if I could take advantage of this feature in this > particular > case, as I need to extract particular line from file, not only find > the match. > Could it be also used in such scenario ? This would work but in this case I am not convinced it is actually better than what you already have. $ dn="" $ line="" $ line=`$GREP "${DOMAIN_STRING}" ${RESOLV_CONF_FILE} 2>/dev/null` $ if [[ "${line}" == ~(E)^domain.* ]]; then ^Jdn="${line#domain }"^Jfi $ echo $dn sfbay.sun.com So perhaps you should leave it as it is. > >> >> usr/src/cmd/installadm/installadm-common.sh >> -------------------------------------------------------------------------- >> >> >> 59 NAMESERVER_STRING="^nameserver[ ]" >> 60 DOMAIN_STRING="^domain[ ]" >> >> Please comment what characters are in the "[" "]". is it a tab and a >> space? > > Yes - I will clarify this. > > Thank you very much for your comments. > Jan >
