Evan Layton wrote:
> Joseph J VLcek wrote:
>> Hey Evan,
>>
>> Issues 1, 2 & 3 are minimal.
>>
>> Issue 4 needs to be addressed.
>>
>> See below.
>>
>> Joe
>>
>>
>> Evan Layton wrote:
>>>
>>> Hi Ginnie and Joe,
>>>
>>> Could you review this change for me?
>>>
>>> Bug 7148 installadm should give correct dhcp macro for the subnet
>>> being configrued on the server.
>>> http://defect.opensolaris.org/bz/show_bug.cgi?id=7148
>>>
>>> Webrev:
>>> http://cr.opensolaris.org/~evanl/7148/
>>>
>>> Some background:
>>> We had originally thought that we would want to create a macro for
>>> every subnet on the system. However in discussing things it made a
>>> lot more sense to only create a macro for the subnet that the user
>>> has requested using the -i option. There fore the change made was to
>>> use the specified subnet and pass the gateway of the subnet to the
>>> dhtadm command generated. If there is no gateway for that address or
>>> we determine that we can't use the gateway we print an error telling
>>> the user they will need to manually add the proper router of gateway
>>> for the subnet.
>>>
>>> Thanks!
>>> -evan
>>
>> Issue 1:
>> --------
>>
>> It's not clear to me why the need to check the subnet address of the
>> "gateway" against that of "net". The code just put that together...
>>
>> e.g.:
>>
>> 196 if [ X$gateway != X -a `echo $net | cut -d'.' -f1-3` = \
>> 197 `echo $gateway | cut -d'.' -f1-3` ]; then
>
> I think this relates to comment #$4. If we're not checking "net" and
> "gateway" against the netmask then we need someway to determine if "net"
> and "gateway" are actually in the same subnet which this check does.
> However if we're going to have to provide code to check against the
> netmask then yes this should go away in favor of that check.
>
>>
>> I think could be done with:
>> -------------------
>>
>> 196 if [ X${gateway} != X ]; then
>>
>>
>> Issue 2: (just for your awareness)
>> --------
>>
>> I realize this is not ksh93 but in general the guidelines found under
>> could be useful when doing shell work.
>>
>> http://installzone-wiki.central.sun.com/wiki/index.php/Ksh93_Tips
>>
>> I don't think conforming to these suggestions is a blocking issue for
>> this fix. Just good suggestions to know...
>
> OK I can look through this a bit more but I will not be going through
> the entire file doing this...
>
> Was there something specific you wanted me to look at with respect to this?
No just general knowledge.
>
>>
>> Issue 3:
>> --------
>>
>> However I think a couple shell suggestions could be good helpful here:
>>
>> - Either define PATH or use full path names for commands.
>>
>> - use {} around variable names.
>>
>> - from ksh93(1): string = pattern Same as ==, but is obsolete.
>>
>> e.g.
>>
>> For:
>> ----
>>
>> 194 gateway=`netstat -rn | awk '/'${net}'/ { print $2
>> }'`
>> 195
>> 196 if [ X$gateway != X -a `echo $net | cut -d'.'
>> -f1-3` = \
>> 197 `echo $gateway | cut -d'.' -f1-3` ]; then
>> 198 $DHCPCONFIG -N ${net} -t ${gateway}
>>
>
> OK I see what you're referring to, will do.
>
>>
>> I would suggest breaking it up for clearity:
>> --------------------------------------------
>>
>>
>> gateway=`/bin/netstat -rn | /bin/awk '/'${net}'/ { print $2 }'`
>> network_net=`/bin/echo ${net} | /bin/cut -d'.' -f1-3`
>> gateway_net=`/bin/echo ${gateway} | /bin/cut -d'.' -f1-3`
>>
>>
>> if [ X${gateway} != X -a ${network_net} == ${gateway_net} ] ; then
>> $DHCPCONFIG -N ${net} -t ${gateway}
>>
>> Issue 4:
>> --------
>>
>> 194 gateway=`netstat -rn | awk '/'${net}'/ { print $2 }'`
>>
>>
>> I think this may be broken. Don't we need to know what the netmask is?
>>
>> You are using the variable "net" as generated on lines 174 & 175. This
>> could work for dhcpconfig because if the "-m" flag is not specified
>> dhcpconfig will apply netmasks to the IP address specified in "-N
>> network_address". See man dhcpconfig(1M).
>>
>> But using the variable "net" may not work for the changes you are
>> making at line 194, unless the netmask is 255.255.255.0
>>
>> It just happens that the netmask on indian-build-sp.central is
>> 255.255.0.0
>>
>> In experimenting with your code changes on indian-build-sp.central I saw:
>>
>> If the user specified the starting IP address: 172.20.27.100
>>
>> and
>>
>> "netstat -rn" produced a matching line:
>>
>> 172.20.0.0 172.20.27.199 U 1 164 e1000g0
>
> Sundar had indicated that this should be treated as a separate issue
> (see 5589). I had originally done a check using the netmask but based on
> comments from Sundar removed them at least until we have complete
> support for class C addresses.
>
OK I see bug 5589 cover this issue.
>>
>> Then, even though there should be a match, the following code won't
>> find a match.
>>
>> indiana-build-sp > net=172.20.27.0
>> indiana-build-sp > /bin/netstat -rn | /bin/awk '/'${net}'/ { print $2}'
>> indiana-build-sp >
>>
>
