Jack Schwartz wrote: > 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).
The bug there is that the netmasks man page fails to acknowledge other information sources, though they are rarely used. Referring to getent doesn't give the user a clue where to fix it (getent is an implementation detail of the script), whereas referring to the netmasks table manpage does. So no, I don't agree with using getent in these messages. Other changes are fine. Dave
