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?
o Does script need to only run as root?
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
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
111,113 routine -> function
112 - who is "its" in "correlates to its hostname"?
140,150 and elsewhere- don't think you need the backslash for $1
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?
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?
181: Is this comment true only if valid="True" here?
321:
if [ "$#" != "0" -a "$#" != "1" ] ; then
->
if [ $# -gt 1 ]; then
322 - why not output something more like usage text. Maybe something like:
check_server_setup [<client_ip_address>]
324, 353, 355:
return -> exit
326:
elif [ "$#" = "1" ] ; then
->
elif [ $# -eq 1 ] ; then
331:
if [ "$?" != "0" ] ; then
->
if [ $? -ne 0 ] ; then
351:
Installations will not work
->
Automated Installations will not work
Sue
