diff mbox

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

Message ID 1453314115-79400-1-git-send-email-openwrt@daniel.thecshore.com
State Changes Requested
Delegated to: Felix Fietkau
Headers show

Commit Message

Daniel Dickinson Jan. 20, 2016, 6:21 p.m. UTC 
From: Daniel Dickinson <openwrt@daniel.thecshore.com>

v3: Drop comment thanking user who gave mask2cidr at their
    request
  : Fix echo had correct CIDR but actual command did not
  : Fix style issue
  : Use full -family in ip command line instead of -f

v2: Also update previously missed deconfig use of ifconfig
  : Replace ipcalc.sh callout with pure shell mask2cidr
  : Remove unused local variable

ip from busybox is now standard and it would be good to
eventually drop the ancient and 10+ year deprecated
upstream commands ifconfig and route, so eliminate
one of the last consumers of ifconfig and route in
the base system.

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

Comments

Felix Fietkau Jan. 28, 2016, 10:52 p.m. UTC | #1 
On 2016-01-20 19:21, openwrt@daniel.thecshore.com wrote:
> From: Daniel Dickinson <openwrt@daniel.thecshore.com>
> 
> v3: Drop comment thanking user who gave mask2cidr at their
>     request
>   : Fix echo had correct CIDR but actual command did not
>   : Fix style issue
>   : Use full -family in ip command line instead of -f
> 
> v2: Also update previously missed deconfig use of ifconfig
>   : Replace ipcalc.sh callout with pure shell mask2cidr
>   : Remove unused local variable
> 
> ip from busybox is now standard and it would be good to
> eventually drop the ancient and 10+ year deprecated
> upstream commands ifconfig and route, so eliminate
> one of the last consumers of ifconfig and route in
> the base system.
> 
> Signed-off-by: Daniel Dickinson <openwrt@daniel.thecshore.com>
> ---
>  .../netifd/files/usr/share/udhcpc/default.script   | 34 +++++++++++++++-------
>  1 file changed, 23 insertions(+), 11 deletions(-)
> 
> 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..5eeeec0 100755
> --- a/package/network/config/netifd/files/usr/share/udhcpc/default.script
> +++ b/package/network/config/netifd/files/usr/share/udhcpc/default.script
> @@ -1,34 +1,46 @@
>  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 CIDR
> +
> +	mask2cidr ${subnet:-255.255.255.0}
> +
> +	echo "udhcpc: ip address add $ip/${CIDR} ${broadcast:-+} dev $interface"
> +	ip address add $ip/${CIDR} ${broadcast:-+} dev $interface"
This doesn't work. Did you test this code?

- Felix
Daniel Dickinson Jan. 30, 2016, 4:55 a.m. UTC | #2 
It worked with a /24 subnet but that might be because of defaults.  I do 
not currently have test bed for testing other configurations.

I plan on setting that up once I'm back from travelling.

Regards,

Daniel

On 28/01/16 05:52 PM, Felix Fietkau wrote:
> On 2016-01-20 19:21, openwrt@daniel.thecshore.com wrote:
>> From: Daniel Dickinson <openwrt@daniel.thecshore.com>
>>
>> v3: Drop comment thanking user who gave mask2cidr at their
>>      request
>>    : Fix echo had correct CIDR but actual command did not
>>    : Fix style issue
>>    : Use full -family in ip command line instead of -f
>>
>> v2: Also update previously missed deconfig use of ifconfig
>>    : Replace ipcalc.sh callout with pure shell mask2cidr
>>    : Remove unused local variable
>>
>> ip from busybox is now standard and it would be good to
>> eventually drop the ancient and 10+ year deprecated
>> upstream commands ifconfig and route, so eliminate
>> one of the last consumers of ifconfig and route in
>> the base system.
>>
>> Signed-off-by: Daniel Dickinson <openwrt@daniel.thecshore.com>
>> ---
>>   .../netifd/files/usr/share/udhcpc/default.script   | 34 +++++++++++++++-------
>>   1 file changed, 23 insertions(+), 11 deletions(-)
>>
>> 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..5eeeec0 100755
>> --- a/package/network/config/netifd/files/usr/share/udhcpc/default.script
>> +++ b/package/network/config/netifd/files/usr/share/udhcpc/default.script
>> @@ -1,34 +1,46 @@
>>   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 CIDR
>> +
>> +	mask2cidr ${subnet:-255.255.255.0}
>> +
>> +	echo "udhcpc: ip address add $ip/${CIDR} ${broadcast:-+} dev $interface"
>> +	ip address add $ip/${CIDR} ${broadcast:-+} dev $interface"
> This doesn't work. Did you test this code?
>
> - Felix
>
Felix Fietkau Jan. 30, 2016, 10:39 a.m. UTC | #3 
On 2016-01-30 05:55, Daniel Dickinson wrote:
> It worked with a /24 subnet but that might be because of defaults.  I do 
> not currently have test bed for testing other configurations.
> 
> I plan on setting that up once I'm back from travelling.

Please avoid top posting, it is a rather inconvenient quoting style.
The broken line is this:
> ip address add $ip/${CIDR} ${broadcast:-+} dev $interface"

It seems to me that you simply kept pieces of ifconfig syntax (the
broadcast +) without testing if they can be used with ip. Please be more
careful with that sort of stuff

- Felix
Daniel Dickinson Jan. 31, 2016, 5:02 a.m. UTC | #4 
On 30/01/16 02:39 AM, Felix Fietkau wrote:
> On 2016-01-30 05:55, Daniel Dickinson wrote:
>> It worked with a /24 subnet but that might be because of defaults.  I do
>> not currently have test bed for testing other configurations.
>>
>> I plan on setting that up once I'm back from travelling.
>
> Please avoid top posting, it is a rather inconvenient quoting style.

Sorry, I am forced to use Outlook for work and haven't figured out how 
to get it to do sane inline conversations, and have bad habits now 
because of that, even when using my personal Mozilla mail.

> The broken line is this:
>> ip address add $ip/${CIDR} ${broadcast:-+} dev $interface"
>
> It seems to me that you simply kept pieces of ifconfig syntax (the
> broadcast +) without testing if they can be used with ip. Please be more
> careful with that sort of stuff

Oh, that explains it.  I actually looked at the required command but had 
a brain fart and thought ${broadcast:-+} meant that I had the word 
broadcast on the command line (i.e. the required parameter of broadcast 
x.x.x.x for ip command), instead of reading it correctly and realizing 
it was just the variable name.

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..5eeeec0 100755
--- a/package/network/config/netifd/files/usr/share/udhcpc/default.script
+++ b/package/network/config/netifd/files/usr/share/udhcpc/default.script
@@ -1,34 +1,46 @@ 
 #!/bin/sh
 [ -z "$1" ] && echo "Error: should be run by udhcpc" && exit 1
 
+mask2cidr()
+{
+	local x=${1##*255.}
+	local allones=$(( (${#1} - ${#x}) * 2 ))
+	local tbl='0^^^128^192^224^240^248^252^254^'
+
+	x=${tbl%%${x%%.*}*}
+	CIDR=$(( allones + (${#x}/4) ))
+}
+
 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"
-		max=$(($max-1))
+		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 CIDR
+
+	mask2cidr ${subnet:-255.255.255.0}
+
+	echo "udhcpc: ip address add $ip/${CIDR} ${broadcast:-+} dev $interface"
+	ip address add $ip/${CIDR} ${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";"}
 		')
 	}
 
@@ -41,7 +53,7 @@  setup_interface() {
 applied=
 case "$1" in
 	deconfig)
-		ifconfig "$interface" 0.0.0.0
+		ip -family inet addr flush dev "$interface"
 	;;
 	renew)
 		setup_interface update