ipv4: Update ip command line processing (take II)

PATCH In review Simon Horman 3 participants 6 messages Dec 26, 2007 – Jan 6, 2008

Subsystems (4)

Networking Network drivers Documentation IPv4/IPv6

lore

Share & tools

lore

Simon Horman

author

2007-12-26 02:59 UTC #0 lore raw

PATCH 1/1 ipv4: Update ip command line processing (take II)

Recently the documentation in Documentation/nfsroot.txt was update to note that in fact ip=off and ip=::::::off as the latter is ignored and the default (on) is used.

This was certainly a step in the direction of reducing confusion. But it seems to me that the code ought to be fixed up so that ip=::::::off actually turns off ip autoconfiguration.

This patch also notes more specifically that ip=on (aka ip=::::::on) is the default.

Cc Amos Waterland <[email protected]>

Signed-off-by Simon Horman <[email protected]>

---
Fri, 21 Dec 2007 16:48:16 +0900
* Removed check on if (name) in ic_proto_name(). It should have been if (*name == '\0'), but even then its a bit bogus, providing an early exit for a code-path that is called approximately once.

net/ipv4/ipconfig.c +4 -6

Index: net-2.6.25/net/ipv4/ipconfig.c
===================================================================
@@ -1417,6 +1417,10 @@ static int __init ic_proto_name(char *na
 	if (!strcmp(name, "on") || !strcmp(name, "any")) {
 		return 1;
 	}
+	if (!strcmp(name, "off") || !strcmp(name, "none")) {
+		ic_enable = 0;
+		return 1;
+	}
 #ifdef CONFIG_IP_PNP_DHCP
 	else if (!strcmp(name, "dhcp")) {
 		ic_proto_enabled &= ~IC_RARP;

@@ -1451,12 +1455,6 @@ static int __init ip_auto_config_setup(c
 
 	ic_set_manually = 1;
 
-	ic_enable = (*addrs &&
-		(strcmp(addrs, "off") != 0) &&
-		(strcmp(addrs, "none") != 0));
-	if (!ic_enable)
-		return 1;
-
 	if (ic_proto_name(addrs))
 		return 1;

Documentation/nfsroot.txt +0 -3

Index: net-2.6.25/Documentation/nfsroot.txt
===================================================================
@@ -97,10 +97,6 @@ ip=<client-ip>:<server-ip>:<gw-ip>:<netm
   autoconfiguration will take place.  The most common way to use this
   is "ip=dhcp".
 
-  Note that "ip=off" is not the same thing as "ip=::::::off", because in
-  the latter autoconfiguration will take place if any of DHCP, BOOTP or RARP
-  are compiled in the kernel.

David Miller

maintainer

2007-12-26 04:55 UTC #1 lore raw

From: Simon Horman <[email protected]>
Date: Wed, 26 Dec 2007 11:59:05 +0900

Quoted 8 lines

Recently the documentation in Documentation/nfsroot.txt was update to note that in fact ip=off and ip=::::::off as the latter is ignored and the default (on) is used.
This was certainly a step in the direction of reducing confusion. But it seems to me that the code ought to be fixed up so that ip=::::::off actually turns off ip autoconfiguration.

Quoted 5 lines

This patch also notes more specifically that ip=on (aka ip=::::::on) is the default.
Cc Amos Waterland <[email protected]>
Signed-off-by Simon Horman <[email protected]>

I applied this to net-2.6 since I think it's a bug fix, and I also tailored the changelog header to more it clear this is a fix not just an "Update". :-)

Thanks!

Simon Horman

author

2007-12-26 05:04 UTC #2 lore raw

Quoted 14 lines · David Miller

From: Simon Horman <[email protected]>
Date: Wed, 26 Dec 2007 11:59:05 +0900
Quoted 11 lines
Recently the documentation in Documentation/nfsroot.txt was update to note that in fact ip=off and ip=::::::off as the latter is ignored and the default (on) is used.
This was certainly a step in the direction of reducing confusion. But it seems to me that the code ought to be fixed up so that ip=::::::off actually turns off ip autoconfiguration.
This patch also notes more specifically that ip=on (aka ip=::::::on) is the default.

Quoted 6 lines

Quoted 2 lines
Cc Amos Waterland <[email protected]>
Signed-off-by Simon Horman <[email protected]>
I applied this to net-2.6 since I think it's a bug fix, and I also tailored the changelog header to more it clear this is a fix not just an "Update". :-)

Ok, thanks.

Signature · 1 line

Horms

Amos Waterland

2008-01-06 03:58 UTC #3 lore raw

PATCH 1/1 Fix regression in ip command line processing

In reply to Simon Horman

The recent changes for ip command line processing fixed some problems but unfortunately broke some common usage scenarios. In current 2.6.24-rc6 the following command line results in no IP address assignment, which is surely a regression:

ip=10.0.2.15::10.0.2.2:255.255.255.0::eth0:off

Please find below a patch that works for all cases I can find.

Signed-off-by Amos Waterland <[email protected]>

---

For convenience, here are the test cases phrased as qemu invocations:

ADDRESS ASSIGNED

qemu -kernel x86/arch/i386/boot/bzImage /dev/zero -append "ip=on"
qemu -kernel x86/arch/i386/boot/bzImage /dev/zero -append "ip=dhcp"
qemu -kernel x86/arch/i386/boot/bzImage /dev/zero -append "ip=both"
qemu -kernel x86/arch/i386/boot/bzImage /dev/zero -append "ip=any"
qemu -kernel x86/arch/i386/boot/bzImage /dev/zero -append "ip=::::::on"
qemu -kernel x86/arch/i386/boot/bzImage /dev/zero -append "ip=::::::dhcp"
qemu -kernel x86/arch/i386/boot/bzImage /dev/zero -append "ip=10.0.2.15::10.0.2.2:255.255.255.0::eth0:off" qemu -kernel x86/arch/i386/boot/bzImage /dev/zero -append "ip=10.0.2.15::10.0.2.2:255.255.255.0::eth0:on"

ADDRESS NOT ASSIGNED

qemu -kernel x86/arch/i386/boot/bzImage /dev/zero -append "ip=" qemu -kernel x86/arch/i386/boot/bzImage /dev/zero -append "ip=off" qemu -kernel x86/arch/i386/boot/bzImage /dev/zero -append "ip=::::::off"

---

 Documentation/nfsroot.txt |    1 +
 net/ipv4/ipconfig.c       |   19 +++++++++++++++----
 2 files changed, 16 insertions(+), 4 deletions(-)

Documentation/nfsroot.txt +1 -0

index c86dd38..1a4d199 100644
@@ -145,6 +145,7 @@ ip=<client-ip>:<server-ip>:<gw-ip>:<netmask>:<hostname>:<device>:<autoconf>
 		this option.
 
                   off or none: don't use autoconfiguration
+		      	       (do static IP assignment instead)
 		  on or any:   use any protocol available in the kernel
 			       (default)
 		  dhcp:        use DHCP

net/ipv4/ipconfig.c +15 -4

index 56a6757..8563b2e 100644
@@ -1404,8 +1404,7 @@ static int __init ic_proto_name(char *name)
 		return 1;
 	}
 	if (!strcmp(name, "off") || !strcmp(name, "none")) {
-		ic_enable = 0;
-		return 1;
+		return 0;
 	}
 #ifdef CONFIG_IP_PNP_DHCP
 	else if (!strcmp(name, "dhcp")) {

@@ -1442,10 +1441,20 @@ static int __init ip_auto_config_setup(char *addrs)
 	ic_set_manually = 1;
 	ic_enable = 1;
 
+	/*
+	 * If any dhcp, bootp etc options are set, leave autoconfig on
+	 * and skip the below static IP processing.
+	 */
 	if (ic_proto_name(addrs))
 		return 1;
 
-	/* Parse the whole string */
+	/* If no static IP is given, turn off autoconfig and bail.  */
+	if (*addrs == 0 || strcmp(addrs, "off") == 0 || strcmp(addrs, "none") == 0) {
+		ic_enable = 0;
+		return 1;
+	}
+
+	/* Parse string for static IP assignment.  */
 	ip = addrs;
 	while (ip && *ip) {
 		if ((cp = strchr(ip, ':')))

@@ -1483,7 +1492,9 @@ static int __init ip_auto_config_setup(char *addrs)
 				strlcpy(user_dev_name, ip, sizeof(user_dev_name));
 				break;
 			case 6:
-				ic_proto_name(ip);
+				if (ic_proto_name(ip) == 0 && ic_myaddr == NONE) {
+					ic_enable = 0;
+				}
 				break;
 			}
 		}

David Miller

maintainer

2008-01-06 07:20 UTC #4 lore raw

Re: [PATCH] Fix regression in ip command line processing

From: Amos Waterland <[email protected]>
Date: Sat, 5 Jan 2008 22:58:16 -0500

Quoted 11 lines

ADDRESS ASSIGNED
qemu -kernel x86/arch/i386/boot/bzImage /dev/zero -append "ip=on"
qemu -kernel x86/arch/i386/boot/bzImage /dev/zero -append "ip=dhcp"
qemu -kernel x86/arch/i386/boot/bzImage /dev/zero -append "ip=both"
qemu -kernel x86/arch/i386/boot/bzImage /dev/zero -append "ip=any"
qemu -kernel x86/arch/i386/boot/bzImage /dev/zero -append "ip=::::::on"
qemu -kernel x86/arch/i386/boot/bzImage /dev/zero -append "ip=::::::dhcp"
qemu -kernel x86/arch/i386/boot/bzImage /dev/zero -append "ip=10.0.2.15::10.0.2.2:255.255.255.0::eth0:off" qemu -kernel x86/arch/i386/boot/bzImage /dev/zero -append "ip=10.0.2.15::10.0.2.2:255.255.255.0::eth0:on"

Quoted 5 lines

ADDRESS NOT ASSIGNED
qemu -kernel x86/arch/i386/boot/bzImage /dev/zero -append "ip=" qemu -kernel x86/arch/i386/boot/bzImage /dev/zero -append "ip=off" qemu -kernel x86/arch/i386/boot/bzImage /dev/zero -append "ip=::::::off"

I really wish Simon had tested his original patch as extensively as you have, this is the second major regression it has caused. :-/

I'll apply your fix, although I'll break up some super long lines you've added, thanks a lot.

Simon Horman

author

2008-01-06 09:25 UTC #5 lore raw

Re: [PATCH] Fix regression in ip command line processing

Quoted 16 lines · David Miller

From: Amos Waterland <[email protected]>
Date: Sat, 5 Jan 2008 22:58:16 -0500
Quoted 13 lines
ADDRESS ASSIGNED
qemu -kernel x86/arch/i386/boot/bzImage /dev/zero -append "ip=on"
qemu -kernel x86/arch/i386/boot/bzImage /dev/zero -append "ip=dhcp"
qemu -kernel x86/arch/i386/boot/bzImage /dev/zero -append "ip=both"
qemu -kernel x86/arch/i386/boot/bzImage /dev/zero -append "ip=any"
qemu -kernel x86/arch/i386/boot/bzImage /dev/zero -append "ip=::::::on"
qemu -kernel x86/arch/i386/boot/bzImage /dev/zero -append "ip=::::::dhcp"
qemu -kernel x86/arch/i386/boot/bzImage /dev/zero -append "ip=10.0.2.15::10.0.2.2:255.255.255.0::eth0:off" qemu -kernel x86/arch/i386/boot/bzImage /dev/zero -append "ip=10.0.2.15::10.0.2.2:255.255.255.0::eth0:on"
ADDRESS NOT ASSIGNED

Quoted 7 lines

Quoted 3 lines
qemu -kernel x86/arch/i386/boot/bzImage /dev/zero -append "ip=" qemu -kernel x86/arch/i386/boot/bzImage /dev/zero -append "ip=off" qemu -kernel x86/arch/i386/boot/bzImage /dev/zero -append "ip=::::::off"
I really wish Simon had tested his original patch as extensively as you have, this is the second major regression it has caused. :-/

Ok, this time around its an issue with my understainding of how things are supposed to work. But sorry none the less.

Signature · 1 line

Horms