[U-Boot] [PATCH v3 0/3] net: Sanitize DHCP variable override

[U-Boot] [PATCH v3 0/3] net: Sanitize DHCP variable override

[Alexander Graf]

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

v2 -> v3:

  - also check for net_boot_file_name_explicit on option 67

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                  | 21 +++++++++++++++------
 net/net.c                    |  2 ++
 7 files changed, 39 insertions(+), 9 deletions(-)

-- 
2.12.3

[U-Boot] [PATCH v3 1/3] net: Prefer command line arguments

[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.

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 <agraf@suse.de>

---

v2 -> v3:

  - also check for net_boot_file_name_explicit on option 67
---
 cmd/net.c     | 10 ++++++++--
 include/net.h |  2 ++
 net/bootp.c   | 14 +++++++++-----
 net/net.c     |  2 ++
 4 files changed, 21 insertions(+), 7 deletions(-)

diff --git a/cmd/net.c b/cmd/net.c
index f83839c35e..eca6dd8918 100644
--- a/cmd/net.c
+++ b/cmd/net.c
@@ -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;
diff --git a/include/net.h b/include/net.h
index 5760685556..a259b7c530 100644
--- a/include/net.h
+++ b/include/net.h
@@ -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 */
diff --git a/net/bootp.c b/net/bootp.c
index 9d7cb5d30c..fdcb4374a0 100644
--- a/net/bootp.c
+++ b/net/bootp.c
@@ -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));
 	}
@@ -889,10 +890,13 @@ static void dhcp_process_options(uchar *popt, uchar *end)
 		case 66:	/* Ignore TFTP server name */
 			break;
 		case 67:	/* Bootfile option */
-			size = truncate_sz("Bootfile",
-					   sizeof(net_boot_file_name), oplen);
-			memcpy(&net_boot_file_name, popt + 2, size);
-			net_boot_file_name[size] = 0;
+			if (!net_boot_file_name_explicit) {
+				size = truncate_sz("Bootfile",
+						   sizeof(net_boot_file_name),
+						   oplen);
+				memcpy(&net_boot_file_name, popt + 2, size);
+				net_boot_file_name[size] = 0;
+			}
 			break;
 		default:
 #if defined(CONFIG_BOOTP_VENDOREX)
diff --git a/net/net.c b/net/net.c
index a4932f46d9..510d491271 100644
--- a/net/net.c
+++ b/net/net.c
@@ -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] [PATCH v3 2/3] net: Add option to prefer bootp/dhcp serverip

[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 <agraf@suse.de>

---

v1 -> v2:

  - remove README entry
  - improve Kconfig help texts
---
 cmd/Kconfig | 11 +++++++++++
 net/bootp.c |  7 ++++++-
 2 files changed, 17 insertions(+), 1 deletion(-)

diff --git a/cmd/Kconfig b/cmd/Kconfig
index e283cb9a8a..80a5af8a0c 100644
--- a/cmd/Kconfig
+++ b/cmd/Kconfig
@@ -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
diff --git a/net/bootp.c b/net/bootp.c
index fdcb4374a0..9a2b512e4a 100644
--- a/net/bootp.c
+++ b/net/bootp.c
@@ -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);
-- 
2.12.3

[U-Boot] [PATCH v3 3/3] ax25: Switch to CONFIG_BOOTP_PREFER_SERVERIP

[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 <agraf@suse.de>
---
 configs/ax25-ae350_defconfig | 1 +
 include/configs/ax25-ae350.h | 1 -
 2 files changed, 1 insertion(+), 1 deletion(-)

diff --git a/configs/ax25-ae350_defconfig b/configs/ax25-ae350_defconfig
index fc04c87485..a328555af6 100644
--- a/configs/ax25-ae350_defconfig
+++ b/configs/ax25-ae350_defconfig
@@ -40,3 +40,4 @@ CONFIG_DM_SPI=y
 CONFIG_ATCSPI200_SPI=y
 CONFIG_TIMER=y
 CONFIG_ATCPIT100_TIMER=y
+CONFIG_BOOTP_PREFER_SERVERIP=y
diff --git a/include/configs/ax25-ae350.h b/include/configs/ax25-ae350.h
index b1ca5ac11a..b230896734 100644
--- a/include/configs/ax25-ae350.h
+++ b/include/configs/ax25-ae350.h
@@ -11,7 +11,6 @@
  * CPU and Board Configuration Options
  */
 #define CONFIG_BOOTP_SEND_HOSTNAME
-#define CONFIG_BOOTP_SERVERIP
 
 /*
  * Miscellaneous configurable options
-- 
2.12.3

[U-Boot] [PATCH v3 2/3] net: Add option to prefer bootp/dhcp serverip

[Joe Hershberger]

On Fri, Jun 15, 2018 at 3:29 AM, Alexander Graf <agraf@suse.de> wrote:
> 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 <agraf@suse.de>

Acked-by: Joe Hershberger <joe.hershberger@ni.com>

[U-Boot] [PATCH v3 1/3] net: Prefer command line arguments

[Joe Hershberger]

On Fri, Jun 15, 2018 at 3:29 AM, Alexander Graf <agraf@suse.de> wrote:
> 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 <agraf@suse.de>

Acked-by: Joe Hershberger <joe.hershberger@ni.com>

[U-Boot] [PATCH v3 3/3] ax25: Switch to CONFIG_BOOTP_PREFER_SERVERIP

[Joe Hershberger]

On Fri, Jun 15, 2018 at 3:29 AM, Alexander Graf <agraf@suse.de> wrote:
> 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 <agraf@suse.de>

Acked-by: Joe Hershberger <joe.hershberger@ni.com>

[U-Boot] [PATCH v3 3/3] ax25: Switch to CONFIG_BOOTP_PREFER_SERVERIP

[Rick Chen]

2018-06-16 4:12 GMT+08:00 Joe Hershberger <joe.hershberger@ni.com>:
> On Fri, Jun 15, 2018 at 3:29 AM, Alexander Graf <agraf@suse.de> wrote:
>> 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 <agraf@suse.de>
>
> Acked-by: Joe Hershberger <joe.hershberger@ni.com>

Acked-by: Rick Chen <rick@andestech.com>

[U-Boot] net: Prefer command line arguments

[Joe Hershberger]

Hi Alexander,

https://patchwork.ozlabs.org/patch/929828/ was applied to http://git.denx.de/?p=u-boot/u-boot-net.git

Thanks!
-Joe

[U-Boot] net: Add option to prefer bootp/dhcp serverip

[Joe Hershberger]

Hi Alexander,

https://patchwork.ozlabs.org/patch/929826/ was applied to http://git.denx.de/?p=u-boot/u-boot-net.git

Thanks!
-Joe

[U-Boot] ax25: Switch to CONFIG_BOOTP_PREFER_SERVERIP

[Joe Hershberger]

Hi Alexander,

https://patchwork.ozlabs.org/patch/929829/ was applied to http://git.denx.de/?p=u-boot/u-boot-net.git

Thanks!
-Joe