Hi Ethan. On 04/15/09 12:14, Ethan Quach wrote: > 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? I didn't think this was necessary. The call to find_network_nmask() is already checked in this way, and find_network_baseIP() operates (and could fail) the same way as find_network_nmask().
I suppose one could argue that one should treat all of find_network_* as black boxes though, and the code would be just a bit more maintainable... OK, I'll make the change. Delta: http://cr.opensolaris.org/~schwartz/090405.1/webrev.incr_5_6/ vs slim_source: http://cr.opensolaris.org/~schwartz/090405.1/webrev/ Please bless. Thanks, Jack > > > 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 >>>>>>> >>>>>> >>>> >>>> >>
