Hi Sue. Thanks for your review.
Webrev are updated with changes from both your and Dave's comments: Delta webrev: http://cr.opensolaris.org/~schwartz/090405.1/webrev.incr_1_2 Webrev vs slim_source: http://cr.opensolaris.org/~schwartz/090405.1/webrev/ My responses inline. Susan Sohn wrote: > On 04/05/09 20:10, Jack Schwartz wrote: >> Hi everyone. >> >> Please review fix for: >> 6252 /etc/nsswitch.dns issues with AI setup if you install from >> livecd >> >> webrev: http://cr.opensolaris.org/~schwartz/090405.1/webrev/ >> bug: http://defect.opensolaris.org/bz/show_bug.cgi?id=6252 >> >> Dave, are you available to be one of the reviewers please, as you are >> a network expert? >> Sue, are you available to code review as well please? >> >> I've tested each code path in the script standalone, tweeking an >> install server's setup environment to verify that problems are caught. >> >> I'd like to get this back before I go on vacation if possible. I >> leave on Wednesday. >> >> Note: these changes don't modify installadm as it is, except to >> invoke checks and stop if the checks fail. >> >> Thanks, >> Jack >> >> P.S. One thing I am not sure about and need more time to look into >> deals with a different issue regarding /etc/netmasks entries. I'll >> file a separate bug for that checking, if it is needed. > > > Hi Jack, > > General: > o Since you are doing things that are ksh specific, like: > print -u 2 "error string" > perhaps you should change this to a ksh script? Originally I thought that since sh is really ksh, I could do print -u. Thinking more, it is best to be consistent with other scripts, so I've changed print -u 2 to the tiny print_err function as what is done in setup-image.sh. Note: I didn't put setup-image.sh in installadm-common.sh even though it is used in two scripts, because I didn't want setup-image.sh to have to include and process the whole of installadm-common to get a single three line function. If there really is no overhead involved with this, let me know and I'll do it that way instead. > o Does script need to only run as root? No. It only checks. It doesn't change anything. > > 26: (nit) > checks for basic network setup, so that an AI server can function > -> > checks for basic network setup necessary for an AI server to function Done. > 54-63 - Any particular reason for including the "_CMD" in the name of the > scripts? I found that it made the code less readable, i.e., I'd rather > see: > $IFCONFIG -a > than > $IFCONFIG_CMD -a The one sentence version: It started with $HOSTNAME, and one thing led to another. Changed. > > 111,113 routine -> function Done. > > 112 - who is "its" in "correlates to its hostname"? Changed "its" to "the system's". > > 140,150 and elsewhere- don't think you need the backslash for $1 Yes you are right. The single quotes handle the $ so no backslash is required. Fixed. > > 147: Didn't Jan send mail saying one of his stopgap fixes should be > taken out > once the loopback check was done in this bug fix? Dave flagged this too. Fixed. > > 155, 177: Are you purposefully not returning here and why? If the > reason is that > you want to check the smf services, then why not move that code to the > top of the > function? Done. > > 181: Is this comment true only if valid="True" here? Yes. Comment adjusted. > > 321: > if [ "$#" != "0" -a "$#" != "1" ] ; then > -> > if [ $# -gt 1 ]; then D'oh. Fixed. > > 322 - why not output something more like usage text. Maybe something > like: > check_server_setup [<client_ip_address>] OK. Changed to: Usage: $0: [ <client_ip_address> ] where dhcp-server checks are made when <client_ip_address> is provided > > 324, 353, 355: > return -> exit OK > > 326: > elif [ "$#" = "1" ] ; then > -> > elif [ $# -eq 1 ] ; then OK. Changed all numeric expressions to be like this. > > 331: > if [ "$?" != "0" ] ; then > -> > if [ $? -ne 0 ] ; then Changed. > > 351: > Installations will not work > -> > Automated Installations will not work Changed. Thanks again, Jack > > Sue
