Jack, The changes look okay.
thanks, -ethan Jack Schwartz wrote: > 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 >>>>>>>> >>>>>>> >>>>> >>>>> >>> >
