Jack, Just a nit.
check-server-setup - 265 - maybe set this to some local variable here first and check if its non-empty before calling do_netmask_check() with it? thanks, -ethan Jack Schwartz wrote: > 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 >>>>>> >>>>> >>> >>> >
