diff mbox

[OpenWrt-Devel] package/config/netifd: Replace ifconfig/route with ip command

Message ID 1453264709-105906-1-git-send-email-openwrt@daniel.thecshore.com
State Changes Requested
Headers show

Commit Message

Daniel Dickinson Jan. 20, 2016, 4:38 a.m. UTC 
From: Daniel Dickinson <openwrt@daniel.thecshore.com>

ip from busybox is now standard and carrying both
sets of commands is undesirale, therefore move
the only consumer of ifconfig/route still in base
to using ip command.

Signed-off-by: Daniel Dickinson <openwrt@daniel.thecshore.com>
---
 .../netifd/files/usr/share/udhcpc/default.script     | 20 ++++++++++++--------
 1 file changed, 12 insertions(+), 8 deletions(-)

Comments

Bastian Bittorf Jan. 20, 2016, 7:10 a.m. UTC | #1 
* openwrt@daniel.thecshore.com <openwrt@daniel.thecshore.com> [20.01.2016 07:21]:
> @@ -5,30 +5,34 @@ set_classless_routes() {
>  	local max=128
>  	local type

thanks for that, i have it also on my todo-list.
please remove also the 'local type' here.

>  	done
>  }
>  
>  setup_interface() {
> -	echo "udhcpc: ifconfig $interface $ip netmask ${subnet:-255.255.255.0} broadcast ${broadcast:-+}"
> -	ifconfig $interface $ip netmask ${subnet:-255.255.255.0} broadcast ${broadcast:-+}
> +	local prefix="$(
> +		eval "$(ipcalc.sh 0.0.0.0 ${subnet:-255.255.255.0})"
> +		echo -n $PREFIX

dont use '-n'

> +	)"
> +		
> +	echo "udhcpc: ip address add $ip/${prefix:-24} ${broadcast:-+} dev $interface"
> +	ip address add $ip/${prefix:-24} ${broadcast:-+} dev $interface"

please dont double-fallback. It's ok to have it once default to '255.255.255.0',
so just use $prefix

maybe we can even have a function in /lib/functions.sh for that:

!#/bin/sh
mask2cidr()
{
	local x=${1##*255.}
	local allones=$(( (${#1} - ${#x}) * 2 ))
	local tbl='0^^^128^192^224^240^248^252^254^'

	x=${tbl%%${x%%.*}*}
	export CIDR=$(( allones + (${#x}/4) ))
}

mask2cidr 255.255.255.224
echo $CIDR


>  
>  	[ -n "$router" ] && [ "$router" != "0.0.0.0" ] && [ "$router" != "255.255.255.255" ] && {
>  		echo "udhcpc: setting default routers: $router"
>  
>  		local valid_gw=""
>  		for i in $router ; do
> -			route add default gw $i dev $interface
> +			ip route add default via $i dev $interface
>  			valid_gw="${valid_gw:+$valid_gw|}$i"
>  		done
>  		
> -		eval $(route -n | awk '
> -			/^0.0.0.0\W{9}('$valid_gw')\W/ {next}
> -			/^0.0.0.0/ {print "route del -net "$1" gw "$2";"}
> +		eval $(ip route | awk '
> +			/^default\Wvia\W('$valid_gw')/ {next}
> +			/^default/ {print "ip route del "$1" via "$3";"}

the code leaves the default-gateway if already set and removes all other
default routes. i dont like the awk-approach, maybe something like:

root@box:~ ip route list exact '0.0.0.0/0'
default via 217.0.116.253 dev pppoe-wan  proto static 
default via 10.63.21.98 dev eth0.1  metric 7 

#!/bin/sh

replace_default_gw()
{
	ip route list exact '0.0.0.0/0' | while read LINE; do
		set -- $LINE
		[ "$3" = "$valid_gw" ] || ip route del default via $3
	done
}

the rest looks good! there are still a lot of other users
for route/ifconfig, but thats a good start!

bye, bastian
Daniel Dickinson Jan. 20, 2016, 9:17 a.m. UTC | #2 
On 20/01/16 02:10 AM, Bastian Bittorf wrote:
> * openwrt@daniel.thecshore.com <openwrt@daniel.thecshore.com> [20.01.2016 07:21]:
>> @@ -5,30 +5,34 @@ set_classless_routes() {
>>   	local max=128
>>   	local type
>
> thanks for that, i have it also on my todo-list.
> please remove also the 'local type' here.

Missed that.

>
>>   	done
>>   }
>>
>>   setup_interface() {
>> -	echo "udhcpc: ifconfig $interface $ip netmask ${subnet:-255.255.255.0} broadcast ${broadcast:-+}"
>> -	ifconfig $interface $ip netmask ${subnet:-255.255.255.0} broadcast ${broadcast:-+}
>> +	local prefix="$(
>> +		eval "$(ipcalc.sh 0.0.0.0 ${subnet:-255.255.255.0})"
>> +		echo -n $PREFIX
>
> dont use '-n'

Why not?  It prevents echo from emitting an unwanted newline.

>
>> +	)"
>> +		
>> +	echo "udhcpc: ip address add $ip/${prefix:-24} ${broadcast:-+} dev $interface"
>> +	ip address add $ip/${prefix:-24} ${broadcast:-+} dev $interface"
>
> please dont double-fallback. It's ok to have it once default to '255.255.255.0',
> so just use $prefix

The second fallback is in case the interpolation fails.

>
> maybe we can even have a function in /lib/functions.sh for that:
>
> !#/bin/sh
> mask2cidr()
> {
> 	local x=${1##*255.}
> 	local allones=$(( (${#1} - ${#x}) * 2 ))
> 	local tbl='0^^^128^192^224^240^248^252^254^'
>
> 	x=${tbl%%${x%%.*}*}
> 	export CIDR=$(( allones + (${#x}/4) ))
> }
>
> mask2cidr 255.255.255.224
> echo $CIDR
>
>
>>
>>   	[ -n "$router" ] && [ "$router" != "0.0.0.0" ] && [ "$router" != "255.255.255.255" ] && {
>>   		echo "udhcpc: setting default routers: $router"
>>
>>   		local valid_gw=""
>>   		for i in $router ; do
>> -			route add default gw $i dev $interface
>> +			ip route add default via $i dev $interface
>>   			valid_gw="${valid_gw:+$valid_gw|}$i"
>>   		done
>>   		
>> -		eval $(route -n | awk '
>> -			/^0.0.0.0\W{9}('$valid_gw')\W/ {next}
>> -			/^0.0.0.0/ {print "route del -net "$1" gw "$2";"}
>> +		eval $(ip route | awk '
>> +			/^default\Wvia\W('$valid_gw')/ {next}
>> +			/^default/ {print "ip route del "$1" via "$3";"}
>
> the code leaves the default-gateway if already set and removes all other
> default routes. i dont like the awk-approach, maybe something like:

I wasn't planning on reworking the udhcpc script beyond making it work 
with ip vs ifconfig/route.  You're talking about changing more unrelated 
things, which really should go in a separate patch.

>
> root@box:~ ip route list exact '0.0.0.0/0'
> default via 217.0.116.253 dev pppoe-wan  proto static
> default via 10.63.21.98 dev eth0.1  metric 7
>
> #!/bin/sh
>
> replace_default_gw()
> {
> 	ip route list exact '0.0.0.0/0' | while read LINE; do
> 		set -- $LINE
> 		[ "$3" = "$valid_gw" ] || ip route del default via $3
> 	done
> }
>
> the rest looks good! there are still a lot of other users
> for route/ifconfig, but thats a good start!
>

Actually according to grep, only openvpn after this in base (there are < 
10 others in packages feed as well, and I am planning on creating a 
minimalist busybox package (calling the binary e.g. net-tools) to supply 
ifconfig/route for those packages that aren't converted yet, or for 
third party use of ifconfig/route (in the packages feeds; doesn't belong 
in base).

Regards,

Daniel
Bastian Bittorf Jan. 20, 2016, 9:24 a.m. UTC | #3 
* Daniel Dickinson <openwrt@daniel.thecshore.com> [20.01.2016 10:18]:
> >>+	local prefix="$(
> >>+		eval "$(ipcalc.sh 0.0.0.0 ${subnet:-255.255.255.0})"
> >>+		echo -n $PREFIX
> >
> >dont use '-n'
> 
> Why not?  It prevents echo from emitting an unwanted newline.

it's not POSIX and really unneeded here (it does not matter for eval).
BTW: if you need this, use: printf '%s' "$string" or just: printf "$string"

> >>+	echo "udhcpc: ip address add $ip/${prefix:-24} ${broadcast:-+} dev $interface"
> >>+	ip address add $ip/${prefix:-24} ${broadcast:-+} dev $interface"
> >
> >please dont double-fallback. It's ok to have it once default to '255.255.255.0',
> >so just use $prefix
> 
> The second fallback is in case the interpolation fails.

ok, i will not discuss this and accept.

> >>-		eval $(route -n | awk '
> >>-			/^0.0.0.0\W{9}('$valid_gw')\W/ {next}
> >>-			/^0.0.0.0/ {print "route del -net "$1" gw "$2";"}
> >>+		eval $(ip route | awk '
> >>+			/^default\Wvia\W('$valid_gw')/ {next}
> >>+			/^default/ {print "ip route del "$1" via "$3";"}
> >
> >the code leaves the default-gateway if already set and removes all other
> >default routes. i dont like the awk-approach, maybe something like:
> 
> I wasn't planning on reworking the udhcpc script beyond making it
> work with ip vs ifconfig/route.  You're talking about changing more
> unrelated things, which really should go in a separate patch.

ok.

> >the rest looks good! there are still a lot of other users
> >for route/ifconfig, but thats a good start!
> 
> Actually according to grep, only openvpn after this in base (there
> are < 10 others in packages feed as well, and I am planning on
> creating a minimalist busybox package (calling the binary e.g.
> net-tools) to supply ifconfig/route for those packages that aren't
> converted yet, or for third party use of ifconfig/route (in the
> packages feeds; doesn't belong in base).

git grep 'ifconfig'
shows a *lot* - but dont mind - thanks for your input.

bye, bastian
Bastian Bittorf Jan. 20, 2016, 9:29 a.m. UTC | #4 
* Daniel Dickinson <openwrt@daniel.thecshore.com> [20.01.2016 10:18]:
> >>+	local prefix="$(
> >>+		eval "$(ipcalc.sh 0.0.0.0 ${subnet:-255.255.255.0})"
> >>+		echo -n $PREFIX
> >
> >dont use '-n'
> 
> Why not?  It prevents echo from emitting an unwanted newline.

by the way:
is somebody fluent enough in AWK to fix this ipcalc-issue
https://dev.openwrt.org/ticket/20750

maybe i should ask 'TheMozg' 8-)
https://github.com/TheMozg/awk-raycaster

bye, bastian
Daniel Dickinson Jan. 20, 2016, 9:32 a.m. UTC | #5 
On 20/01/16 04:24 AM, Bastian Bittorf wrote:
> * Daniel Dickinson <openwrt@daniel.thecshore.com> [20.01.2016 10:18]:
>>>> +	local prefix="$(
>>>> +		eval "$(ipcalc.sh 0.0.0.0 ${subnet:-255.255.255.0})"
>>>> +		echo -n $PREFIX
>>>
>>> dont use '-n'
>>
>> Why not?  It prevents echo from emitting an unwanted newline.
>
> it's not POSIX and really unneeded here (it does not matter for eval).

ah, ok didn't know it wasn't POSIX and forgot about eval.

> BTW: if you need this, use: printf '%s' "$string" or just: printf "$string"

>
>>>> +	echo "udhcpc: ip address add $ip/${prefix:-24} ${broadcast:-+} dev $interface"
>>>> +	ip address add $ip/${prefix:-24} ${broadcast:-+} dev $interface"
>>>
>>> please dont double-fallback. It's ok to have it once default to '255.255.255.0',
>>> so just use $prefix
>>
>> The second fallback is in case the interpolation fails.
>
> ok, i will not discuss this and accept.
>
>>>> -		eval $(route -n | awk '
>>>> -			/^0.0.0.0\W{9}('$valid_gw')\W/ {next}
>>>> -			/^0.0.0.0/ {print "route del -net "$1" gw "$2";"}
>>>> +		eval $(ip route | awk '
>>>> +			/^default\Wvia\W('$valid_gw')/ {next}
>>>> +			/^default/ {print "ip route del "$1" via "$3";"}
>>>
>>> the code leaves the default-gateway if already set and removes all other
>>> default routes. i dont like the awk-approach, maybe something like:
>>
>> I wasn't planning on reworking the udhcpc script beyond making it
>> work with ip vs ifconfig/route.  You're talking about changing more
>> unrelated things, which really should go in a separate patch.
>
> ok.
>
>>> the rest looks good! there are still a lot of other users
>>> for route/ifconfig, but thats a good start!
>>
>> Actually according to grep, only openvpn after this in base (there
>> are < 10 others in packages feed as well, and I am planning on
>> creating a minimalist busybox package (calling the binary e.g.
>> net-tools) to supply ifconfig/route for those packages that aren't
>> converted yet, or for third party use of ifconfig/route (in the
>> packages feeds; doesn't belong in base).
>
> git grep 'ifconfig'
> shows a *lot* - but dont mind - thanks for your input.
>

There is are only two uses in packages after this (qos-scripts (which I 
missed) and openvpn), but I missed target, which has a bunch of preinit use.


Regards,

Daniel
Daniel Dickinson Jan. 20, 2016, 9:44 a.m. UTC | #6 
On 20/01/16 04:24 AM, Bastian Bittorf wrote:
>>> please dont double-fallback. It's ok to have it once default to '255.255.255.0',
>>> so just use $prefix
>>
>> The second fallback is in case the interpolation fails.

> ok, i will not discuss this and accept.

On second thought I don't like relying on ipcalc.sh and would prefer the 
replace that bit (which is new new anyway with something like the code 
you posted), and what would kill two birds with one stone.

Regards,

Daniel
diff mbox

Patch

diff --git a/package/network/config/netifd/files/usr/share/udhcpc/default.script b/package/network/config/netifd/files/usr/share/udhcpc/default.script
index ac765a6..9e002c8 100755
--- a/package/network/config/netifd/files/usr/share/udhcpc/default.script
+++ b/package/network/config/netifd/files/usr/share/udhcpc/default.script
@@ -5,30 +5,34 @@  set_classless_routes() {
 	local max=128
 	local type
 	while [ -n "$1" -a -n "$2" -a $max -gt 0 ]; do
-		[ ${1##*/} -eq 32 ] && type=host || type=net
 		echo "udhcpc: adding route for $type $1 via $2"
-		route add -$type "$1" gw "$2" dev "$interface"
+		ip route add "$1" via "$2" dev "$interface"
 		max=$(($max-1))
 		shift 2
 	done
 }
 
 setup_interface() {
-	echo "udhcpc: ifconfig $interface $ip netmask ${subnet:-255.255.255.0} broadcast ${broadcast:-+}"
-	ifconfig $interface $ip netmask ${subnet:-255.255.255.0} broadcast ${broadcast:-+}
+	local prefix="$(
+		eval "$(ipcalc.sh 0.0.0.0 ${subnet:-255.255.255.0})"
+		echo -n $PREFIX
+	)"
+		
+	echo "udhcpc: ip address add $ip/${prefix:-24} ${broadcast:-+} dev $interface"
+	ip address add $ip/${prefix:-24} ${broadcast:-+} dev $interface"
 
 	[ -n "$router" ] && [ "$router" != "0.0.0.0" ] && [ "$router" != "255.255.255.255" ] && {
 		echo "udhcpc: setting default routers: $router"
 
 		local valid_gw=""
 		for i in $router ; do
-			route add default gw $i dev $interface
+			ip route add default via $i dev $interface
 			valid_gw="${valid_gw:+$valid_gw|}$i"
 		done
 		
-		eval $(route -n | awk '
-			/^0.0.0.0\W{9}('$valid_gw')\W/ {next}
-			/^0.0.0.0/ {print "route del -net "$1" gw "$2";"}
+		eval $(ip route | awk '
+			/^default\Wvia\W('$valid_gw')/ {next}
+			/^default/ {print "ip route del "$1" via "$3";"}
 		')
 	}