Jack, installadm-common.sh ------------------------------------ 784 - Can you rename this function to something like find_network_nmask() to be a bit more clear? We already have a get_ip_netmask() function which find_netmask() sounds like it'd be doing, but it does something different.
In addition, line 786 could be clarified to something like -- Given an IP address, figure out which network on this server it belongs to, and return that network's netmask. check-server-setup.sh ----------------------------------- 264 - this doesn't look right, I wasn't expecting that you'd pass $ds_client_ipaddr to do_netmask_check(). I don't think the getent() call at line 200 would necessarily yield anything if its just some new dhcp ip the user is planning to add to the dhcp server. I was expecting that you'd pass the IP of the interface of the network that $ds_client_ipaddr belongs to. Looks like you can easily augment find_network_attr() to pass back yet another type of attribute, the IP of the interface itself, and write a new wrapper function to get that. thanks, -ethan Jack Schwartz wrote: > Hi Ethan. > > Thanks for your review. > > On 04/14/09 01:06, Ethan Quach wrote: >> Jack, >> >> Have you thought about doing 237-253 for the ipaddr of the interface >> that's actually going to be used with the given starting_client_dhcp >> ipaddr? >> As it stands, it could be verifying a netmask for network X on the >> server, >> but the starting_client_dhcp ipaddr could be for network Y. Since the >> dhcp configuration is what actually wants the network table to be >> correct >> for the network it's configuring, we could still see a failure later on >> during the dhcp configuration. > OK. You are correct. I now to the same test for the client network > as I do for the system network. > > Aside from some function shuffling, I have added a new function in > installadm-common.sh called find_netmask which is almost the same as > find_network but returns a netmask instead. > > Delta webrev: > http://cr.opensolaris.org/~schwartz/090405.1/webrev.incr_3_4/ > > Vs slim_source: http://cr.opensolaris.org/~schwartz/090405.1/webrev/ > > Thanks, > Jack >> >> >> thanks, >> -ethan >> >> >> Jack Schwartz wrote: >>> Hi Dave. >>> >>> Thanks for round 2... We're almost there... >>> >>> On 04/13/09 10:28, Dave Miner wrote: >>>>>> 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. >>> This message (at line 240) didn't refer to either getent or >>> netmasks. Sounds like you want me to reference netmasks(4) there to >>> better clue the user in on what to change. So we're back to your >>> original suggestion of: >>> >>> "The netmask for network $ds_ipaddr is not configured in the >>> netmasks(4) table." >>> >>> There was one message on line 248 which did refer to getent, which I >>> will change from: >>> "The netmask obtained from getent(1M) for network $ds_ipaddr does >>> not equal the netmask configured for the interface for that network." >>> >>> to: >>> "The netmask obtained from netmasks(4) for network $ds_ipaddr >>> does not equal the netmask configured for the interface for that >>> network." >>> >>> Does this work for you? >>> >>> One other change which Ethan pointed out to me in person will be to >>> call the get_host_ip() from installadm-common rather than call >>> geteng myself on line 165. >>> >>> I webrevs updated with these changes. Please bless. >>> >>> Delta: http://cr.opensolaris.org/~schwartz/090405.1/webrev.incr_2_3/ >>> >>> Vs slim_source: http://cr.opensolaris.org/~schwartz/090405.1/webrev/ >>> >>> Thanks, >>> Jack >>>> >>>> Other changes are fine. >>>> >>>> Dave >>>> >>> > >
