diff mbox series

[v3,1/3] net: Prefer command line arguments

Message ID 20180615082929.82491-2-agraf@suse.de
State Accepted
Commit 449312c1c0c686ad28c51e6429d8bbdd13812b10
Headers show
Series net: Sanitize DHCP variable override | expand

Commit Message

Alexander Graf June 15, 2018, 8:29 a.m. UTC
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(-)

Comments

Joe Hershberger June 15, 2018, 8:12 p.m. UTC | #1
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>
Joe Hershberger July 2, 2018, 7:50 p.m. UTC | #2
Hi Alexander,

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

Thanks!
-Joe
diff mbox series

Patch

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 */