Hi Dave. Thanks for reviewing.
Webrev are updated with changes from both your and Sue'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 replies are inline. On 04/06/09 13:57, Dave Miner wrote: > 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 >> > > General comments: > > - I believe there was already a validation against loopback somewhere > in installadm (Jan mentioned this in email the other day). I would > have expected that to be removed in favor of this. Any reason why > it's not? Removed. > > - no internationalization of messages? Is that consistent with the > rest of installadm? If so, there needs to be a bug about that. > check-server-setup.sh: Filed: 8016 Scripts invoked by installadm need to be localized > > 133: "is". Please use sentences in messages. OK. I've added "the"s and "is"s, etc. > > 153: This message is too detailed. It would be better to say > something like, "The hostname $HOSTNAME is associated with the > loopback IP address, which is incorrect for an automated installation > service." Changed to: "Server hostname $HOSTNAME resolved as a loopback IP address." which is similar to what Jan's code said. > 174: need work on this message as well. Something more like "The IP > address $GETENT_IP is not assigned to any of the system's network > interfaces" Changed to your suggestion. > > 181ff: I would have done the service check first. It's simpler and > more likely to be the primary problem, as if you've switched from one > to the other properly, the more detailed problems shouldn't be an > issue any longer. And if it is wrong, the error message should > point to a little more than just disabling nwam and enabling > network/physical:default, since you actually need to configure > interfaces in order for network/physical:default to do anything. Moved service checks to the top of the first routine called. Left the error messages alone here, because they will be complemented by the other messages will follow them as other things get tested by the script. > > 233: error message would be better as "The netmask for network > $ds_ipaddr is not configured in the netmasks(4) table." How about this: The netmask for network $ds_ipaddr cannot be found or is improperly configured. The reason the original message referred to getent(1M) instead of netmasks(4) was because I assumed getent could potentially find the netmask from somewhere other than /etc/netmasks(4), since there is a netmasks entry in /etc/nsswitch.conf. Am I correct? I don't want to refer to netmasks(4) when some other source for netmasks exists and may be used by getent. This would only confuse the user. If netmasks(4) is really the only place the netmask can come from and you don't like my suggestion above, I'll change the message to refer to netmasks(4). > 240: error message is too detailed here, too. "The netmask obtained > from the netmasks(4) table for network $ds_ipaddr does not equal the > netmask configured on the network interface for that network" How about your message but instead of "the netmasks(4) table" it says "getent(1M)" for the same reasons as above? > > 252: delete the "which start in column 0". OK > You could improve this with a [ -f /etc/resolv.conf ] to disambiguate > the two cases into a more precise diagnosis. Done. > > 253: I thought we'd agreed in prior discussion that this is a warning > case, not an error. There are certainly configurations where AI can > be used without DNS at all (if the manifests reference services by IP > address rather than name), and even if DNS is needed and the server is > not appropriately configured, the administrator can easily correct > this using dhtadm or dhcpmgr to supply the correct information via > DHCP. We just need to warn them of the likely need. OK. I thought we agreed just not to check for dns in ipnodes and hosts entries in nsswitch.conf, but I'll make this a warning and not a (fatal) error. Removed changing valid to "False" for this test. > > 256-287: I don't get what you're trying to check here. The default > gateway certainly doesn't have to (and generally won't) be the AI server. OK. I had set this because I had to do this in the lab and Sundar's setup instructions mentioned it, but you're right that there's more than one right way to set up a network. I think the point of those original instructions was to be able to access the network, but you're right in that that's just one scenario. Removed. > > > 289-310: This can be removed. Not having a default route known by the > server for the network that the client is on is not an error. Ethan's > fix for 5589 et al will have the correct resolution for routing > configuration, by including a default route when it's known and > warning when it's not. This can probably be removed. OK. Removed. > > Dave Thanks, Jack
