net: Sanitize DHCP variable override

PATCH Superseded Alexander Graf 3 participants 13 messages Jun 14, 2018 – Jun 15, 2018 1 message not in the archive Networking

lore

Share & tools

lore

Alexander Graf

author

2018-06-14 10:04 UTC #0 lore raw

v2 cover net: Sanitize DHCP variable override

While trying to boot from network on a RISC-V AX25 platform, I saw that the DHCP IP address did not get populated from the DHCP server IP address. The reason for that was simple: CONFIG_BOOTP_SERVERIP was set.

I don't know the history of that option, but it seems to decrease intuitivity levels of the dhcp command rather than improve it.

What I usually would expect is that explicitly set values populate through all layers. So if I set a TFTP file name, it populates. If I set a target IP address, it populates. If I don't set anything, the values get filled in automatically.

This patch set is trying to move us into that direction without breaking people that rely on the existing behavior. With this patch set applied, boards have the option to prefer the 'serverip' environment variable (ax25-ae350 gets moved to it) over the DHCP given address and any value explicitly set on the command line is always preferred.

This hopefully makes the command line a bit more intuitive.

v1 -> v2:

- new patch: net: Prefer command line arguments
- remove README entry
- improve Kconfig help texts

Alexander Graf (3):
net: Prefer command line arguments
net: Add option to prefer bootp/dhcp serverip
ax25: Switch to CONFIG_BOOTP_PREFER_SERVERIP

 cmd/Kconfig                  | 11 +++++++++++
 cmd/net.c                    | 10 ++++++++--
 configs/ax25-ae350_defconfig |  1 +
 include/configs/ax25-ae350.h |  1 -
 include/net.h                |  2 ++
 net/bootp.c                  | 10 ++++++++--
 net/net.c                    |  2 ++
 7 files changed, 32 insertions(+), 5 deletions(-)

Signature · 1 line

2.12.3

Alexander Graf

author

2018-06-14 10:04 UTC #1 lore raw

PATCH v2 1/3 net: Prefer command line arguments

We can call commands like dhcp and bootp without arguments or with explicit command line arguments that really should tell the code where to look for files instead.

Unfortunately, the current code simply overwrites command line arguments in the dhcp case with dhcp values.

This patch allows the code to preserve the command line values if they were set on the command line. That way the semantics are slightly more intuitive.

The reason this patch does that by introducing a new variable is that we can not rely on net_boot_file_name[0] being unset, as today it's completely legal to call "dhcp" and afterwards run "tftp" and expect the latter to repeat the same query as before. I would prefer not to break that behavior in case anyone relies on it.

Signed-off-by Alexander Graf <[email protected]>

---
 cmd/net.c     | 10 ++++++++--
 include/net.h |  2 ++
 net/bootp.c   |  3 ++-
 net/net.c     |  2 ++
 4 files changed, 14 insertions(+), 3 deletions(-)

cmd/net.c +8 -2

index f83839c35e..eca6dd8918 100644
@@ -183,6 +183,8 @@ static int netboot_common(enum proto_t proto, cmd_tbl_t *cmdtp, int argc,
 	int   size;
 	ulong addr;
 
+	net_boot_file_name_explicit = false;
+
 	/* pre-set load_addr */
 	s = env_get("loadaddr");
 	if (s != NULL)

@@ -199,15 +201,18 @@ static int netboot_common(enum proto_t proto, cmd_tbl_t *cmdtp, int argc,
 		 * mis-interpreted as a valid number.
 		 */
 		addr = simple_strtoul(argv[1], &end, 16);
-		if (end == (argv[1] + strlen(argv[1])))
+		if (end == (argv[1] + strlen(argv[1]))) {
 			load_addr = addr;
-		else
+		} else {
+			net_boot_file_name_explicit = true;
 			copy_filename(net_boot_file_name, argv[1],
 				      sizeof(net_boot_file_name));
+		}
 		break;
 
 	case 3:
 		load_addr = simple_strtoul(argv[1], NULL, 16);
+		net_boot_file_name_explicit = true;
 		copy_filename(net_boot_file_name, argv[2],
 			      sizeof(net_boot_file_name));

@@ -220,6 +225,7 @@ static int netboot_common(enum proto_t proto, cmd_tbl_t *cmdtp, int argc,
 			printf("Invalid address/size\n");
 			return CMD_RET_USAGE;
 		}
+		net_boot_file_name_explicit = true;
 		copy_filename(net_boot_file_name, argv[3],
 			      sizeof(net_boot_file_name));
 		break;

include/net.h +2 -0

index 5760685556..a259b7c530 100644
@@ -539,6 +539,8 @@ enum proto_t {
 };
 
 extern char	net_boot_file_name[1024];/* Boot File name */
+/* Indicates whether the file name was specified on the command line */
+extern bool	net_boot_file_name_explicit;
 /* The actual transferred size of the bootfile (in bytes) */
 extern u32	net_boot_file_size;
 /* Boot file size in blocks as reported by the DHCP server */

net/bootp.c +2 -1

index 9d7cb5d30c..f3da8572b7 100644
@@ -157,7 +157,8 @@ static void store_net_params(struct bootp_hdr *bp)
 #if defined(CONFIG_CMD_DHCP)
 	    !(dhcp_option_overload & OVERLOAD_FILE) &&
 #endif
-	    (strlen(bp->bp_file) > 0)) {
+	    (strlen(bp->bp_file) > 0) &&
+	    !net_boot_file_name_explicit) {
 		copy_filename(net_boot_file_name, bp->bp_file,
 			      sizeof(net_boot_file_name));
 	}

net/net.c +2 -0

index a4932f46d9..510d491271 100644
@@ -165,6 +165,8 @@ ushort		net_native_vlan = 0xFFFF;
 
 /* Boot File name */
 char net_boot_file_name[1024];
+/* Indicates whether the file name was specified on the command line */
+bool net_boot_file_name_explicit;
 /* The actual transferred size of the bootfile (in bytes) */
 u32 net_boot_file_size;
 /* Boot file size in blocks as reported by the DHCP server */

Signature · 1 line

2.12.3

Joe Hershberger

2018-06-14 16:34 UTC #2 lore raw

PATCH v2 1/3 net: Prefer command line arguments

Quoted 85 lines · Alexander Graf

We can call commands like dhcp and bootp without arguments or with explicit command line arguments that really should tell the code where to look for files instead.

Unfortunately, the current code simply overwrites command line arguments in the dhcp case with dhcp values.

This patch allows the code to preserve the command line values if they were set on the command line. That way the semantics are slightly more intuitive.

Signed-off-by Alexander Graf <[email protected]>

---
 cmd/net.c     | 10 ++++++++--
 include/net.h |  2 ++
 net/bootp.c   |  3 ++-
 net/net.c     |  2 ++
 4 files changed, 14 insertions(+), 3 deletions(-)

cmd/net.c +8 -2

index f83839c35e..eca6dd8918 100644
@@ -183,6 +183,8 @@ static int netboot_common(enum proto_t proto, cmd_tbl_t *cmdtp, int argc,
        int   size;
        ulong addr;
 
+       net_boot_file_name_explicit = false;
+
        /* pre-set load_addr */
        s = env_get("loadaddr");
        if (s != NULL)

@@ -199,15 +201,18 @@ static int netboot_common(enum proto_t proto, cmd_tbl_t *cmdtp, int argc,
                 * mis-interpreted as a valid number.
                 */
                addr = simple_strtoul(argv[1], &end, 16);
-               if (end == (argv[1] + strlen(argv[1])))
+               if (end == (argv[1] + strlen(argv[1]))) {
                        load_addr = addr;
-               else
+               } else {
+                       net_boot_file_name_explicit = true;
                        copy_filename(net_boot_file_name, argv[1],
                                      sizeof(net_boot_file_name));
+               }
                break;
 
        case 3:
                load_addr = simple_strtoul(argv[1], NULL, 16);
+               net_boot_file_name_explicit = true;
                copy_filename(net_boot_file_name, argv[2],
                              sizeof(net_boot_file_name));

@@ -220,6 +225,7 @@ static int netboot_common(enum proto_t proto, cmd_tbl_t *cmdtp, int argc,
                        printf("Invalid address/size\n");
                        return CMD_RET_USAGE;
                }
+               net_boot_file_name_explicit = true;
                copy_filename(net_boot_file_name, argv[3],
                              sizeof(net_boot_file_name));
                break;

include/net.h +2 -0

index 5760685556..a259b7c530 100644
@@ -539,6 +539,8 @@ enum proto_t {
 };
 
 extern char    net_boot_file_name[1024];/* Boot File name */
+/* Indicates whether the file name was specified on the command line */
+extern bool    net_boot_file_name_explicit;
 /* The actual transferred size of the bootfile (in bytes) */
 extern u32     net_boot_file_size;
 /* Boot file size in blocks as reported by the DHCP server */

net/bootp.c +0 -0

index 9d7cb5d30c..f3da8572b7 100644

net/bootp.c static void store_net_params(struct bootp_hdr *bp) · line 161

Quoted 6 lines

net/bootp.c +2 -1

@@ -157,7 +157,8 @@ static void store_net_params(struct bootp_hdr *bp)
 #if defined(CONFIG_CMD_DHCP)
            !(dhcp_option_overload & OVERLOAD_FILE) &&
 #endif
-           (strlen(bp->bp_file) > 0)) {
+           (strlen(bp->bp_file) > 0) &&
+           !net_boot_file_name_explicit) {

This seems like a reasonable check, but it also needs to happen in the case that the file is passed under option 67.

Quoted 23 lines

net/bootp.c +0 -0

                copy_filename(net_boot_file_name, bp->bp_file,
                              sizeof(net_boot_file_name));
        }

net/net.c +2 -1

index a4932f46d9..510d491271 100644
@@ -165,6 +165,8 @@ ushort              net_native_vlan = 0xFFFF;
 
 /* Boot File name */
 char net_boot_file_name[1024];
+/* Indicates whether the file name was specified on the command line */
+bool net_boot_file_name_explicit;
 /* The actual transferred size of the bootfile (in bytes) */
 u32 net_boot_file_size;
 /* Boot file size in blocks as reported by the DHCP server */
--

2.12.3

_______________________________________________
U-Boot mailing list
U-Boot at lists.denx.de
https://lists.denx.de/listinfo/u-boot

Alexander Graf

author

2018-06-14 10:04 UTC #3 lore raw

PATCH v2 2/3 net: Add option to prefer bootp/dhcp serverip

In reply to Alexander Graf

Currently we can choose between 2 different types of behavior for the serverip variable:

1) Always overwrite it with the DHCP server IP address (default)
2) Ignore what the DHCP server says (CONFIG_BOOTP_SERVERIP)

This patch adds a 3rd option:

3) Use serverip from DHCP if no serverip is given (CONFIG_BOOTP_PREFER_SERVERIP)

With this new option, we can have the default case that a boot file gets loaded from the DHCP provided TFTP server work while allowing users to specify their own serverip variable to explicitly use a different tftp server.

Signed-off-by Alexander Graf <[email protected]>

---

v1 -> v2:

- remove README entry
- improve Kconfig help texts

---
 cmd/Kconfig | 11 +++++++++++
 net/bootp.c |  7 ++++++-
 2 files changed, 17 insertions(+), 1 deletion(-)

cmd/Kconfig +11 -0

index e283cb9a8a..80a5af8a0c 100644
@@ -1121,6 +1121,17 @@ config BOOTP_HOSTNAME
 	help
 	  The name may or may not be qualified with the local domain name.
 
+config BOOTP_PREFER_SERVERIP
+	bool "Serverip variable takes precedent over DHCP server IP.
+	default n
+	depends on CMD_BOOTP
+	help
+	  By default a BOOTP/DHCP reply will overwrite the 'serverip' variable.
+
+	  With this option enabled, the 'serverip' variable in the environment
+	  takes precedence over DHCP server IP and will only be set by the DHCP
+	  server if not already set in the environment.
+
 config BOOTP_SUBNETMASK
 	bool "Request & store 'netmask' from BOOTP/DHCP server"
 	default y

net/bootp.c +6 -1

index f3da8572b7..154c71d694 100644
@@ -147,9 +147,14 @@ static void store_net_params(struct bootp_hdr *bp)
 {
 #if !defined(CONFIG_BOOTP_SERVERIP)
 	struct in_addr tmp_ip;
+	bool overwrite_serverip = true;
+
+#if defined(CONFIG_BOOTP_PREFER_SERVERIP)
+	overwrite_serverip = false;
+#endif
 
 	net_copy_ip(&tmp_ip, &bp->bp_siaddr);
-	if (tmp_ip.s_addr != 0)
+	if (tmp_ip.s_addr != 0 && (overwrite_serverip || !net_server_ip.s_addr))
 		net_copy_ip(&net_server_ip, &bp->bp_siaddr);
 	memcpy(net_server_ethaddr,
 	       ((struct ethernet_hdr *)net_rx_packet)->et_src, 6);

Signature · 1 line

2.12.3

Joe Hershberger

2018-06-14 16:57 UTC #4 lore raw

PATCH v2 2/3 net: Add option to prefer bootp/dhcp serverip

Quoted 11 lines · Alexander Graf

Currently we can choose between 2 different types of behavior for the serverip variable:
1) Always overwrite it with the DHCP server IP address (default)
2) Ignore what the DHCP server says (CONFIG_BOOTP_SERVERIP)
This patch adds a 3rd option:
3) Use serverip from DHCP if no serverip is given (CONFIG_BOOTP_PREFER_SERVERIP)

Quoted 6 lines

With this new option, we can have the default case that a boot file gets loaded from the DHCP provided TFTP server work while allowing users to specify their own serverip variable to explicitly use a different tftp server.
Signed-off-by Alexander Graf <[email protected]>

Acked-by Joe Hershberger <[email protected]>

Alexander Graf

author

2018-06-14 10:04 UTC #5 lore raw

PATCH v2 3/3 ax25: Switch to CONFIG_BOOTP_PREFER_SERVERIP

In reply to Alexander Graf

The ax25-ae350 target currently uses CONFIG_BOOTP_SERVERIP which means we ignore the DHCP provided TFTP ip address. This breaks every case where we do now provide a serverip environment variable.

Instead, let's use the new CONFIG_BOOT_PREFER_SERVERIP option to fall back to the DHCP provided TFTP IP if no serverip environment variable is set.

Signed-off-by Alexander Graf <[email protected]>

---
 configs/ax25-ae350_defconfig | 1 +
 include/configs/ax25-ae350.h | 1 -
 2 files changed, 1 insertion(+), 1 deletion(-)

configs/ax25-ae350_defconfig +1 -0

index fc04c87485..a328555af6 100644
@@ -40,3 +40,4 @@ CONFIG_DM_SPI=y
 CONFIG_ATCSPI200_SPI=y
 CONFIG_TIMER=y
 CONFIG_ATCPIT100_TIMER=y
+CONFIG_BOOTP_PREFER_SERVERIP=y

include/configs/ax25-ae350.h +0 -1

index b1ca5ac11a..b230896734 100644
@@ -11,7 +11,6 @@
  * CPU and Board Configuration Options
  */
 #define CONFIG_BOOTP_SEND_HOSTNAME
-#define CONFIG_BOOTP_SERVERIP
 
 /*
  * Miscellaneous configurable options

Signature · 1 line

2.12.3

Joe Hershberger

2018-06-14 16:58 UTC #6 lore raw

PATCH v2 3/3 ax25: Switch to CONFIG_BOOTP_PREFER_SERVERIP

Quoted 8 lines · Alexander Graf

The ax25-ae350 target currently uses CONFIG_BOOTP_SERVERIP which means we ignore the DHCP provided TFTP ip address. This breaks every case where we do now provide a serverip environment variable.
Instead, let's use the new CONFIG_BOOT_PREFER_SERVERIP option to fall back to the DHCP provided TFTP IP if no serverip environment variable is set.
Signed-off-by Alexander Graf <[email protected]>

Reviewed-by Joe Hershberger <[email protected]>

Quoted 19 lines

---
 configs/ax25-ae350_defconfig | 1 +
 include/configs/ax25-ae350.h | 1 -
 2 files changed, 1 insertion(+), 1 deletion(-)

configs/ax25-ae350_defconfig +1 -0

index fc04c87485..a328555af6 100644
@@ -40,3 +40,4 @@ CONFIG_DM_SPI=y
 CONFIG_ATCSPI200_SPI=y
 CONFIG_TIMER=y
 CONFIG_ATCPIT100_TIMER=y
+CONFIG_BOOTP_PREFER_SERVERIP=y

include/configs/ax25-ae350.h +0 -0

index b1ca5ac11a..b230896734 100644

include/configs/ax25-ae350.h line 14

Quoted 4 lines

include/configs/ax25-ae350.h +0 -1

@@ -11,7 +11,6 @@
  * CPU and Board Configuration Options
  */
 #define CONFIG_BOOTP_SEND_HOSTNAME
-#define CONFIG_BOOTP_SERVERIP

Feel like moving this to Kconfig?

Quoted 10 lines

include/configs/ax25-ae350.h +0 -1
 /*
  * Miscellaneous configurable options
--
2.12.3
_______________________________________________
U-Boot mailing list
U-Boot at lists.denx.de
https://lists.denx.de/listinfo/u-boot

Alexander Graf

author

2018-06-15 08:24 UTC #7 lore raw

PATCH v2 3/3 ax25: Switch to CONFIG_BOOTP_PREFER_SERVERIP

Quoted 29 lines · Joe Hershberger

Quoted 8 lines · Alexander Graf
The ax25-ae350 target currently uses CONFIG_BOOTP_SERVERIP which means we ignore the DHCP provided TFTP ip address. This breaks every case where we do now provide a serverip environment variable.
Instead, let's use the new CONFIG_BOOT_PREFER_SERVERIP option to fall back to the DHCP provided TFTP IP if no serverip environment variable is set.
Signed-off-by Alexander Graf <[email protected]>
Reviewed-by Joe Hershberger <[email protected]>
Quoted 17 lines
---
 configs/ax25-ae350_defconfig | 1 +
 include/configs/ax25-ae350.h | 1 -
 2 files changed, 1 insertion(+), 1 deletion(-)
configs/ax25-ae350_defconfig +1 -0
index fc04c87485..a328555af6 100644
@@ -40,3 +40,4 @@ CONFIG_DM_SPI=y
 CONFIG_ATCSPI200_SPI=y
 CONFIG_TIMER=y
 CONFIG_ATCPIT100_TIMER=y
+CONFIG_BOOTP_PREFER_SERVERIP=y
include/configs/ax25-ae350.h +0 -1
index b1ca5ac11a..b230896734 100644
--- a/include/configs/ax25-ae350.h

Quoted 8 lines

include/configs/ax25-ae350.h line 14
Quoted 6 lines
+++ b/include/configs/ax25-ae350.h
include/configs/ax25-ae350.h +0 -1
@@ -11,7 +11,6 @@
  * CPU and Board Configuration Options
  */
 #define CONFIG_BOOTP_SEND_HOSTNAME
-#define CONFIG_BOOTP_SERVERIP
Feel like moving this to Kconfig?

I would actually prefer to remove it altogether ;)

Alex

Joe Hershberger

2018-06-15 20:08 UTC #8 lore raw

PATCH v2 3/3 ax25: Switch to CONFIG_BOOTP_PREFER_SERVERIP

Quoted 41 lines · Alexander Graf

Quoted 29 lines · Joe Hershberger
Quoted 8 lines · Alexander Graf
The ax25-ae350 target currently uses CONFIG_BOOTP_SERVERIP which means we ignore the DHCP provided TFTP ip address. This breaks every case where we do now provide a serverip environment variable.
Instead, let's use the new CONFIG_BOOT_PREFER_SERVERIP option to fall back to the DHCP provided TFTP IP if no serverip environment variable is set.
Signed-off-by Alexander Graf <[email protected]>
Reviewed-by Joe Hershberger <[email protected]>
Quoted 17 lines
---
 configs/ax25-ae350_defconfig | 1 +
 include/configs/ax25-ae350.h | 1 -
 2 files changed, 1 insertion(+), 1 deletion(-)
configs/ax25-ae350_defconfig +1 -0
index fc04c87485..a328555af6 100644
@@ -40,3 +40,4 @@ CONFIG_DM_SPI=y
 CONFIG_ATCSPI200_SPI=y
 CONFIG_TIMER=y
 CONFIG_ATCPIT100_TIMER=y
+CONFIG_BOOTP_PREFER_SERVERIP=y
include/configs/ax25-ae350.h +0 -1
index b1ca5ac11a..b230896734 100644
--- a/include/configs/ax25-ae350.h
Quoted 8 lines
include/configs/ax25-ae350.h line 14
Quoted 6 lines
+++ b/include/configs/ax25-ae350.h
include/configs/ax25-ae350.h +0 -1
@@ -11,7 +11,6 @@
  * CPU and Board Configuration Options
  */
 #define CONFIG_BOOTP_SEND_HOSTNAME
-#define CONFIG_BOOTP_SERVERIP
Feel like moving this to Kconfig?

Quoted 1 line

I would actually prefer to remove it altogether ;)

I'm with you, actually... though I think the behavior should be to always ignore the DHCP server's settings when they are on the command line or in the environment. If you want the DHCP server's info, the user's script can remove the variables explicitly.

Quoted 6 lines

Alex
_______________________________________________
U-Boot mailing list
U-Boot at lists.denx.de
https://lists.denx.de/listinfo/u-boot

Alexander Graf

author

2018-06-15 20:33 UTC #9 lore raw

PATCH v2 3/3 ax25: Switch to CONFIG_BOOTP_PREFER_SERVERIP

Quoted 42 lines · Joe Hershberger

Quoted 41 lines · Alexander Graf
Quoted 37 lines · Joe Hershberger
Quoted 8 lines · Alexander Graf
The ax25-ae350 target currently uses CONFIG_BOOTP_SERVERIP which means we ignore the DHCP provided TFTP ip address. This breaks every case where we do now provide a serverip environment variable.
Instead, let's use the new CONFIG_BOOT_PREFER_SERVERIP option to fall back to the DHCP provided TFTP IP if no serverip environment variable is set.
Signed-off-by Alexander Graf <[email protected]>
Reviewed-by Joe Hershberger <[email protected]>
Quoted 19 lines
---
 configs/ax25-ae350_defconfig | 1 +
 include/configs/ax25-ae350.h | 1 -
 2 files changed, 1 insertion(+), 1 deletion(-)
configs/ax25-ae350_defconfig +1 -0
index fc04c87485..a328555af6 100644
@@ -40,3 +40,4 @@ CONFIG_DM_SPI=y
 CONFIG_ATCSPI200_SPI=y
 CONFIG_TIMER=y
 CONFIG_ATCPIT100_TIMER=y
+CONFIG_BOOTP_PREFER_SERVERIP=y
include/configs/ax25-ae350.h +0 -0
index b1ca5ac11a..b230896734 100644
include/configs/ax25-ae350.h line 14
Quoted 4 lines
include/configs/ax25-ae350.h +0 -1
@@ -11,7 +11,6 @@
  * CPU and Board Configuration Options
  */
 #define CONFIG_BOOTP_SEND_HOSTNAME
-#define CONFIG_BOOTP_SERVERIP
Feel like moving this to Kconfig?

Quoted 6 lines

Quoted 1 line
I would actually prefer to remove it altogether ;)
I'm with you, actually... though I think the behavior should be to always ignore the DHCP server's settings when they are on the command line or in the environment. If you want the DHCP server's info, the user's script can remove the variables explicitly.

Yeah, the only case I can think of where the current model could hurt us is if you run "dhcp" and then call "saveenv". Because that saveenv will contain all those glorious variables that may now override the next dhcp request.

Btw, you wouldn't happen to work with Julia? ;)

Alex

Joe Hershberger

2018-06-15 20:36 UTC #10 lore raw

PATCH v2 3/3 ax25: Switch to CONFIG_BOOTP_PREFER_SERVERIP

Quoted 52 lines · Alexander Graf

Quoted 42 lines · Joe Hershberger
Quoted 41 lines · Alexander Graf
Quoted 37 lines · Joe Hershberger
Quoted 8 lines · Alexander Graf
The ax25-ae350 target currently uses CONFIG_BOOTP_SERVERIP which means we ignore the DHCP provided TFTP ip address. This breaks every case where we do now provide a serverip environment variable.
Instead, let's use the new CONFIG_BOOT_PREFER_SERVERIP option to fall back to the DHCP provided TFTP IP if no serverip environment variable is set.
Signed-off-by Alexander Graf <[email protected]>
Reviewed-by Joe Hershberger <[email protected]>
Quoted 19 lines
---
 configs/ax25-ae350_defconfig | 1 +
 include/configs/ax25-ae350.h | 1 -
 2 files changed, 1 insertion(+), 1 deletion(-)
configs/ax25-ae350_defconfig +1 -0
index fc04c87485..a328555af6 100644
@@ -40,3 +40,4 @@ CONFIG_DM_SPI=y
 CONFIG_ATCSPI200_SPI=y
 CONFIG_TIMER=y
 CONFIG_ATCPIT100_TIMER=y
+CONFIG_BOOTP_PREFER_SERVERIP=y
include/configs/ax25-ae350.h +0 -0
index b1ca5ac11a..b230896734 100644
include/configs/ax25-ae350.h line 14
Quoted 4 lines
include/configs/ax25-ae350.h +0 -1
@@ -11,7 +11,6 @@
  * CPU and Board Configuration Options
  */
 #define CONFIG_BOOTP_SEND_HOSTNAME
-#define CONFIG_BOOTP_SERVERIP
Feel like moving this to Kconfig?
Quoted 6 lines
Quoted 1 line
I would actually prefer to remove it altogether ;)
I'm with you, actually... though I think the behavior should be to always ignore the DHCP server's settings when they are on the command line or in the environment. If you want the DHCP server's info, the user's script can remove the variables explicitly.

Quoted 4 lines

Yeah, the only case I can think of where the current model could hurt us is if you run "dhcp" and then call "saveenv". Because that saveenv will contain all those glorious variables that may now override the next dhcp request.

I think in that case you could explicitly clean up those variables... I've intended to implement ephemeral variables anyway... this is one of the reasons.

Quoted 1 line

Btw, you wouldn't happen to work with Julia? ;)

Not directly, but yes. :)

Quoted 5 lines

Alex
_______________________________________________
U-Boot mailing list
U-Boot at lists.denx.de
https://lists.denx.de/listinfo/u-boot

Alexander Graf

author

2018-06-15 20:39 UTC #11 lore raw

PATCH v2 3/3 ax25: Switch to CONFIG_BOOTP_PREFER_SERVERIP

Quoted 53 lines · Joe Hershberger

Quoted 52 lines · Alexander Graf
Quoted 48 lines · Joe Hershberger
Quoted 41 lines · Alexander Graf
Quoted 29 lines · Joe Hershberger
Quoted 8 lines · Alexander Graf
The ax25-ae350 target currently uses CONFIG_BOOTP_SERVERIP which means we ignore the DHCP provided TFTP ip address. This breaks every case where we do now provide a serverip environment variable.
Instead, let's use the new CONFIG_BOOT_PREFER_SERVERIP option to fall back to the DHCP provided TFTP IP if no serverip environment variable is set.
Signed-off-by Alexander Graf <[email protected]>
Reviewed-by Joe Hershberger <[email protected]>
Quoted 17 lines
---
 configs/ax25-ae350_defconfig | 1 +
 include/configs/ax25-ae350.h | 1 -
 2 files changed, 1 insertion(+), 1 deletion(-)
configs/ax25-ae350_defconfig +1 -0
index fc04c87485..a328555af6 100644
@@ -40,3 +40,4 @@ CONFIG_DM_SPI=y
 CONFIG_ATCSPI200_SPI=y
 CONFIG_TIMER=y
 CONFIG_ATCPIT100_TIMER=y
+CONFIG_BOOTP_PREFER_SERVERIP=y
include/configs/ax25-ae350.h +0 -1
index b1ca5ac11a..b230896734 100644
--- a/include/configs/ax25-ae350.h
Quoted 8 lines
include/configs/ax25-ae350.h line 14
Quoted 6 lines
+++ b/include/configs/ax25-ae350.h
include/configs/ax25-ae350.h +0 -1
@@ -11,7 +11,6 @@
  * CPU and Board Configuration Options
  */
 #define CONFIG_BOOTP_SEND_HOSTNAME
-#define CONFIG_BOOTP_SERVERIP
Feel like moving this to Kconfig?
Quoted 1 line
I would actually prefer to remove it altogether ;)
I'm with you, actually... though I think the behavior should be to always ignore the DHCP server's settings when they are on the command line or in the environment. If you want the DHCP server's info, the user's script can remove the variables explicitly.

Quoted 8 lines

Quoted 4 lines
Yeah, the only case I can think of where the current model could hurt us is if you run "dhcp" and then call "saveenv". Because that saveenv will contain all those glorious variables that may now override the next dhcp request.
I think in that case you could explicitly clean up those variables... I've intended to implement ephemeral variables anyway... this is one of the reasons.

I'll be happy to pass this one on to you then :). I am already too deep down into rabbit holes.

Quoted 3 lines

Quoted 1 line
Btw, you wouldn't happen to work with Julia? ;)
Not directly, but yes. :)

Awesome, just met her last weekend ;)

Alex

A message that is not in the archive (sent off-list or not yet received): [email protected]

Rick Chen

2018-06-15 06:12 UTC #13 lore raw

v2 cover net: Sanitize DHCP variable override

Quoted 47 lines

From: Alexander Graf [mailto:agraf at suse.de]
Sent: Thursday, June 14, 2018 6:04 PM
To: u-boot at lists.denx.de
Cc: Rick Jian-Zhi Chen(陳建志); Joe Hershberger; Greentime Ying-Han Hu(胡英漢); Simon Glass
Subject: [PATCH v2 0/3] net: Sanitize DHCP variable override
While trying to boot from network on a RISC-V AX25 platform, I saw that the DHCP IP address did not get populated from the DHCP server IP address. The reason for that was simple: CONFIG_BOOTP_SERVERIP was set.
I don't know the history of that option, but it seems to decrease intuitivity levels of the dhcp command rather than improve it.
What I usually would expect is that explicitly set values populate through all layers. So if I set a TFTP file name, it populates. If I set a target IP address, it populates. If I don't set anything, the values get filled in automatically.
This patch set is trying to move us into that direction without breaking people that rely on the existing behavior. With this patch set applied, boards have the option to prefer the 'serverip'
environment variable (ax25-ae350 gets moved to it) over the DHCP given address and any value explicitly set on the command line is always preferred.
This hopefully makes the command line a bit more intuitive.
v1 -> v2:
- new patch: net: Prefer command line arguments
- remove README entry
- improve Kconfig help texts
Alexander Graf (3):
net: Prefer command line arguments
net: Add option to prefer bootp/dhcp serverip
ax25: Switch to CONFIG_BOOTP_PREFER_SERVERIP
 cmd/Kconfig                  | 11 +++++++++++
 cmd/net.c                    | 10 ++++++++--
 configs/ax25-ae350_defconfig |  1 +
 include/configs/ax25-ae350.h |  1 -
 include/net.h                |  2 ++
 net/bootp.c                  | 10 ++++++++--
 net/net.c                    |  2 ++
 7 files changed, 32 insertions(+), 5 deletions(-)

Quoted 2 lines

--
2.12.3

Hi Alex

The V2 is ok about the dhcp verification.

Message as below:

U-Boot 2018.07-rc1-00075-g3411a40 (Jun 15 2018 - 14:05:21 +0800)

DRAM: 1 GiB
No arch specific invalidate_icache_all available!

Flash: 64 MiB
MMC:   mmc at f0e00000: 0
Loading Environment from SPI Flash... SF: Detected mx25u1635e with
page size 256 Bytes, erase size 4 KiB, total 2 MiB
OK
In:    serial at f0300000
Out:   serial at f0300000
Err:   serial at f0300000
Net:   no alias for ethernet0

Warning: mac at e0100000 (eth0) using random MAC address - 7e:c1:32:b2:20:cf
eth0: mac at e0100000
Hit any key to stop autoboot: 0
RISC-V #
RISC-V # env print
baudrate=38400
bootcmd=fatload mmc 0:1 0x20000000 ae350_64.dtb;fatload mmc 0:1 0x0 bbl-ae350.bin;go 0x0
bootdelay=3
bootfile=10.0.4.97:boomimage-310y-ag101p.bin
ethact=mac at e0100000
fdtcontroladdr=3fede290
fileaddr=600000
filesize=1bb7d34
stderr=serial at f0300000
stdin=serial at f0300000
stdout=serial at f0300000

Environment size: 329/8188 bytes
RISC-V # dhcp 0x600000 10.0.4.97:boomimage-310y-ag101p.bin BOOTP broadcast 1
BOOTP broadcast 2
BOOTP broadcast 3
BOOTP broadcast 4
DHCP client bound to address 10.0.4.124 (4596 ms)
Using mac at e0100000 device
TFTP from server 10.0.4.97; our IP address is 10.0.4.124 Filename 'boomimage-310y-ag101p.bin'.
Load address: 0x600000
Loading: #################################################################

         #################################################################
         #################################################################
         #################################################################
         #################################################################
         #################################################################
         #################################################################
         #################################################################
         #################################################################
         #################################################################
         #################################################################
         #################################################################
         #################################################################
         #################################################################
         ########################################
         174.8 KiB/s

done
Bytes transferred = 13938796 (d4b06c hex)
RISC-V #

Rick