Hi Ethan. On 04/14/09 20:10, Ethan Quach wrote: > 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. Renamed to find_network_nmask(). > > 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. Nicely put. Done. > > > 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. OK, changed.
FWIW, my experience with "getent netmasks" was different than you describe, and so my code worked for me. It returned something if it finds an entry in netmasks(4) which "matches" (as defined below). match = (IP address & mask) == (network num & mask) Maybe someone has a better explanation; I'm no expert at this. On my system I have the following in netmasks: 129.146.226.0 255.255.254.0 $ getent netmasks 129.146.226.0 129.146.226.0 255.255.254.0 $ getent netmasks 129.146.226.123 129.146.226.123 255.255.254.0 $ getent netmasks 129.146.227.123 129.146.227.123 255.255.254.0 $ getent netmasks 129.147.230.123 $ > > I was expecting that you'd pass the IP of the interface of the > network that $ds_client_ipaddr belongs to. OK. I'll change this as it makes sense. > > 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. Yes. That's how I'll get the above interface IP. Webrevs updated: delta: http://cr.opensolaris.org/~schwartz/090405.1/webrev.incr_4_5/ vs slim_source: http://cr.opensolaris.org/~schwartz/090405.1/webrev/ Please bless. Thanks, Jack > > > > 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 >>>>> >>>> >> >>
