[U-Boot] [PATCH 1/5] net: When checking prerequisites, consider boot_file_name
For net_boot_common, we allow the serverip to be specified as part of the boot file name. For net commands that require serverip, include that source as a valid specification of serverip.
Signed-off-by: Joe Hershberger joe.hershberger@ni.com ---
include/net.h | 3 +++ net/net.c | 7 ++++++- 2 files changed, 9 insertions(+), 1 deletion(-)
diff --git a/include/net.h b/include/net.h index f9984ae86c..de2d7bba19 100644 --- a/include/net.h +++ b/include/net.h @@ -839,6 +839,9 @@ ushort env_get_vlan(char *); /* copy a filename (allow for "..." notation, limit length) */ void copy_filename(char *dst, const char *src, int size);
+/* check if serverip is specified in filename from the command line */ +int is_serverip_in_cmd(void); + /* get a random source port */ unsigned int random_port(void);
diff --git a/net/net.c b/net/net.c index f35695b4fc..bff3e9c5b5 100644 --- a/net/net.c +++ b/net/net.c @@ -1341,7 +1341,7 @@ static int net_check_prereq(enum proto_t protocol) /* Fall through */ case TFTPGET: case TFTPPUT: - if (net_server_ip.s_addr == 0) { + if (net_server_ip.s_addr == 0 && !is_serverip_in_cmd()) { puts("*** ERROR: `serverip' not set\n"); return 1; } @@ -1512,6 +1512,11 @@ void copy_filename(char *dst, const char *src, int size) *dst = '\0'; }
+int is_serverip_in_cmd(void) +{ + return !!strchr(net_boot_file_name, ':'); +} + #if defined(CONFIG_CMD_NFS) || \ defined(CONFIG_CMD_SNTP) || \ defined(CONFIG_CMD_DNS)
With net autoload, we check the prerequisites for the initial command, but the greater prerequisites when autoloading are not checked.
If we would attempt to autoload, check those prerequisites too.
If we are not expecting a serverip from the server, then don't worry about it not being set, but don't attempt to load if it isn't.
Signed-off-by: Joe Hershberger joe.hershberger@ni.com ---
net/net.c | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+)
diff --git a/net/net.c b/net/net.c index bff3e9c5b5..42a50e60f8 100644 --- a/net/net.c +++ b/net/net.c @@ -332,6 +332,16 @@ void net_auto_load(void) const char *s = env_get("autoload");
if (s != NULL && strcmp(s, "NFS") == 0) { + if (net_check_prereq(NFS)) { +/* We aren't expecting to get a serverip, so just accept the assigned IP */ +#ifdef CONFIG_BOOTP_SERVERIP + net_set_state(NETLOOP_SUCCESS); +#else + printf("Cannot autoload with NFS\n"); + net_set_state(NETLOOP_FAIL); +#endif + return; + } /* * Use NFS to load the bootfile. */ @@ -347,6 +357,16 @@ void net_auto_load(void) net_set_state(NETLOOP_SUCCESS); return; } + if (net_check_prereq(TFTPGET)) { +/* We aren't expecting to get a serverip, so just accept the assigned IP */ +#ifdef CONFIG_BOOTP_SERVERIP + net_set_state(NETLOOP_SUCCESS); +#else + printf("Cannot autoload with TFTPGET\n"); + net_set_state(NETLOOP_FAIL); +#endif + return; + } tftp_start(TFTPGET); }
On 07/04/2018 02:36 AM, Joe Hershberger wrote:
With net autoload, we check the prerequisites for the initial command, but the greater prerequisites when autoloading are not checked.
If we would attempt to autoload, check those prerequisites too.
If we are not expecting a serverip from the server, then don't worry about it not being set, but don't attempt to load if it isn't.
Signed-off-by: Joe Hershberger joe.hershberger@ni.com
net/net.c | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+)
diff --git a/net/net.c b/net/net.c index bff3e9c5b5..42a50e60f8 100644 --- a/net/net.c +++ b/net/net.c @@ -332,6 +332,16 @@ void net_auto_load(void) const char *s = env_get("autoload");
if (s != NULL && strcmp(s, "NFS") == 0) {
if (net_check_prereq(NFS)) {+/* We aren't expecting to get a serverip, so just accept the assigned IP */ +#ifdef CONFIG_BOOTP_SERVERIP
net_set_state(NETLOOP_SUCCESS);+#else
printf("Cannot autoload with NFS\n");net_set_state(NETLOOP_FAIL);+#endif
I don't understand the #ifdef. In the CONFIG_BOOTP_SERVERIP case, you should already have net_server_ip set from the variable setter, so when do you realistically get into the case where net_check_prereq() fails here? I can only see that happening when serverip is not set (read: net_server_ip == 0.0.0.0) in which case we should also error out in the CONFIG_BOOTP_SERVERIP case, no?
Alex
On Wed, Jul 4, 2018 at 4:20 AM, Alexander Graf agraf@suse.de wrote:
On 07/04/2018 02:36 AM, Joe Hershberger wrote:
With net autoload, we check the prerequisites for the initial command, but the greater prerequisites when autoloading are not checked.
If we would attempt to autoload, check those prerequisites too.
If we are not expecting a serverip from the server, then don't worry about it not being set, but don't attempt to load if it isn't.
Signed-off-by: Joe Hershberger joe.hershberger@ni.com
net/net.c | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+)
diff --git a/net/net.c b/net/net.c index bff3e9c5b5..42a50e60f8 100644 --- a/net/net.c +++ b/net/net.c @@ -332,6 +332,16 @@ void net_auto_load(void) const char *s = env_get("autoload"); if (s != NULL && strcmp(s, "NFS") == 0) {
if (net_check_prereq(NFS)) {+/* We aren't expecting to get a serverip, so just accept the assigned IP */ +#ifdef CONFIG_BOOTP_SERVERIP
net_set_state(NETLOOP_SUCCESS);+#else
printf("Cannot autoload with NFS\n");net_set_state(NETLOOP_FAIL);+#endif
I don't understand the #ifdef. In the CONFIG_BOOTP_SERVERIP case, you should already have net_server_ip set from the variable setter, so when do you realistically get into the case where net_check_prereq() fails here? I can
My thinking here was that if the user is in control of the serverip and chooses not to set it, then at least populate the dhcp variables that were successful. If we return a fail from here, even though DHCP was successful, the result will not be saved to the env for the user.
only see that happening when serverip is not set (read: net_server_ip == 0.0.0.0) in which case we should also error out in the CONFIG_BOOTP_SERVERIP case, no?
Alex
U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
On 07/04/2018 06:23 PM, Joe Hershberger wrote:
On Wed, Jul 4, 2018 at 4:20 AM, Alexander Graf agraf@suse.de wrote:
On 07/04/2018 02:36 AM, Joe Hershberger wrote:
With net autoload, we check the prerequisites for the initial command, but the greater prerequisites when autoloading are not checked.
If we would attempt to autoload, check those prerequisites too.
If we are not expecting a serverip from the server, then don't worry about it not being set, but don't attempt to load if it isn't.
Signed-off-by: Joe Hershberger joe.hershberger@ni.com
net/net.c | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+)
diff --git a/net/net.c b/net/net.c index bff3e9c5b5..42a50e60f8 100644 --- a/net/net.c +++ b/net/net.c @@ -332,6 +332,16 @@ void net_auto_load(void) const char *s = env_get("autoload"); if (s != NULL && strcmp(s, "NFS") == 0) {
if (net_check_prereq(NFS)) {+/* We aren't expecting to get a serverip, so just accept the assigned IP */ +#ifdef CONFIG_BOOTP_SERVERIP
net_set_state(NETLOOP_SUCCESS);+#else
printf("Cannot autoload with NFS\n");net_set_state(NETLOOP_FAIL);+#endif
I don't understand the #ifdef. In the CONFIG_BOOTP_SERVERIP case, you should already have net_server_ip set from the variable setter, so when do you realistically get into the case where net_check_prereq() fails here? I can
My thinking here was that if the user is in control of the serverip and chooses not to set it, then at least populate the dhcp variables that were successful. If we return a fail from here, even though DHCP was successful, the result will not be saved to the env for the user.
IMHO CONFIG_BOOTP_SERVERIP is a very esoteric use case already. Let's not interpret too much into it. Instead, I would prefer if we could just treat it the same as the normal case as often as we can ...
Or maybe just move its functionality (do not set serverip from dhcp command) into an environment variable.
Alex
Hi Joe,
https://patchwork.ozlabs.org/patch/939041/ was applied to http://git.denx.de/?p=u-boot/u-boot-net.git
Thanks! -Joe
Rather than crashing, check the src ptr and set dst to empty string.
Signed-off-by: Joe Hershberger joe.hershberger@ni.com ---
net/net.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/net/net.c b/net/net.c index 42a50e60f8..333102ea79 100644 --- a/net/net.c +++ b/net/net.c @@ -1522,12 +1522,12 @@ void net_set_udp_header(uchar *pkt, struct in_addr dest, int dport, int sport,
void copy_filename(char *dst, const char *src, int size) { - if (*src && (*src == '"')) { + if (src && *src && (*src == '"')) { ++src; --size; }
- while ((--size > 0) && *src && (*src != '"')) + while ((--size > 0) && src && *src && (*src != '"')) *dst++ = *src++; *dst = '\0'; }
On 07/04/2018 02:36 AM, Joe Hershberger wrote:
Rather than crashing, check the src ptr and set dst to empty string.
Signed-off-by: Joe Hershberger joe.hershberger@ni.com
Wouldn't it make more sense to check for the existence outside at the caller's side? That way it's much easier to see what really is happening.
Alex
On Wed, Jul 4, 2018 at 4:25 AM, Alexander Graf agraf@suse.de wrote:
On 07/04/2018 02:36 AM, Joe Hershberger wrote:
Rather than crashing, check the src ptr and set dst to empty string.
Signed-off-by: Joe Hershberger joe.hershberger@ni.com
Wouldn't it make more sense to check for the existence outside at the caller's side? That way it's much easier to see what really is happening.
It's much easier to allow NULL so that we can directly pass the return result of getenv().
Alex
U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
On 07/04/2018 06:18 PM, Joe Hershberger wrote:
On Wed, Jul 4, 2018 at 4:25 AM, Alexander Graf agraf@suse.de wrote:
On 07/04/2018 02:36 AM, Joe Hershberger wrote:
Rather than crashing, check the src ptr and set dst to empty string.
Signed-off-by: Joe Hershberger joe.hershberger@ni.com
Wouldn't it make more sense to check for the existence outside at the caller's side? That way it's much easier to see what really is happening.
It's much easier to allow NULL so that we can directly pass the return result of getenv().
I know, and I see how it looks insanely smart and simple today. Until you realize that the amazing "copy_filename" function doesn't touch the target at all if it gets passed in NULL. And all of that implicitly. So implicitly it will leave the old value in the filename if nothing is set in env.
Imaging you're trying to read the code 4 years from now. Will you remember all of the side effects of copy_filename()? Imagine you're someone who's new to the code and doesn't know all the implicit side effects of its functions. Will you understand what is going on at a glimpse?
Alex
On Thu, Jul 5, 2018 at 6:49 AM, Alexander Graf agraf@suse.de wrote:
On 07/04/2018 06:18 PM, Joe Hershberger wrote:
On Wed, Jul 4, 2018 at 4:25 AM, Alexander Graf agraf@suse.de wrote:
On 07/04/2018 02:36 AM, Joe Hershberger wrote:
Rather than crashing, check the src ptr and set dst to empty string.
Signed-off-by: Joe Hershberger joe.hershberger@ni.com
Wouldn't it make more sense to check for the existence outside at the caller's side? That way it's much easier to see what really is happening.
It's much easier to allow NULL so that we can directly pass the return result of getenv().
I know, and I see how it looks insanely smart and simple today. Until you realize that the amazing "copy_filename" function doesn't touch the target at all if it gets passed in NULL. And all of that implicitly. So implicitly it will leave the old value in the filename if nothing is set in env.
I think you are mis-reading the code. If src is NULL, it will set dst[0] = '\0'; I think the behavior is quite reasonable.
Imaging you're trying to read the code 4 years from now. Will you remember all of the side effects of copy_filename()? Imagine you're someone who's new to the code and doesn't know all the implicit side effects of its functions. Will you understand what is going on at a glimpse?
That's an argument for documentation... and anyway, yes, I think the function is sensible and I would expect it to do what it does.
-Joe
Hey Alex,
On Thu, Jul 5, 2018 at 12:27 PM, Joe Hershberger joe.hershberger@ni.com wrote:
On Thu, Jul 5, 2018 at 6:49 AM, Alexander Graf agraf@suse.de wrote:
On 07/04/2018 06:18 PM, Joe Hershberger wrote:
On Wed, Jul 4, 2018 at 4:25 AM, Alexander Graf agraf@suse.de wrote:
On 07/04/2018 02:36 AM, Joe Hershberger wrote:
Rather than crashing, check the src ptr and set dst to empty string.
Signed-off-by: Joe Hershberger joe.hershberger@ni.com
Wouldn't it make more sense to check for the existence outside at the caller's side? That way it's much easier to see what really is happening.
It's much easier to allow NULL so that we can directly pass the return result of getenv().
I know, and I see how it looks insanely smart and simple today. Until you realize that the amazing "copy_filename" function doesn't touch the target at all if it gets passed in NULL. And all of that implicitly. So implicitly it will leave the old value in the filename if nothing is set in env.
I think you are mis-reading the code. If src is NULL, it will set dst[0] = '\0'; I think the behavior is quite reasonable.
Do you have any outstanding issues with this?
Imaging you're trying to read the code 4 years from now. Will you remember all of the side effects of copy_filename()? Imagine you're someone who's new to the code and doesn't know all the implicit side effects of its functions. Will you understand what is going on at a glimpse?
That's an argument for documentation... and anyway, yes, I think the function is sensible and I would expect it to do what it does.
-Joe
Am 11.07.2018 um 22:54 schrieb Joe Hershberger joe.hershberger@ni.com:
Hey Alex,
On Thu, Jul 5, 2018 at 12:27 PM, Joe Hershberger joe.hershberger@ni.com wrote:
On Thu, Jul 5, 2018 at 6:49 AM, Alexander Graf agraf@suse.de wrote:
On 07/04/2018 06:18 PM, Joe Hershberger wrote:
On Wed, Jul 4, 2018 at 4:25 AM, Alexander Graf agraf@suse.de wrote:
On 07/04/2018 02:36 AM, Joe Hershberger wrote:
Rather than crashing, check the src ptr and set dst to empty string.
Signed-off-by: Joe Hershberger joe.hershberger@ni.com
Wouldn't it make more sense to check for the existence outside at the caller's side? That way it's much easier to see what really is happening.
It's much easier to allow NULL so that we can directly pass the return result of getenv().
I know, and I see how it looks insanely smart and simple today. Until you realize that the amazing "copy_filename" function doesn't touch the target at all if it gets passed in NULL. And all of that implicitly. So implicitly it will leave the old value in the filename if nothing is set in env.
I think you are mis-reading the code. If src is NULL, it will set dst[0] = '\0'; I think the behavior is quite reasonable.
Do you have any outstanding issues with this?
Nope, your call :)
Alex
Hi Joe,
https://patchwork.ozlabs.org/patch/939042/ was applied to http://git.denx.de/?p=u-boot/u-boot-net.git
Thanks! -Joe
Instead of depending on a env callback for bootfile, read it explicitly.
We do this because the bootfile can be specified on the command line and if it is, we will overwrite the internal variable. If a netboot_common() is called again with no bootfile parameter, we want to use the one in the environment.
Signed-off-by: Joe Hershberger joe.hershberger@ni.com ---
cmd/net.c | 6 ++++++ net/net.c | 20 -------------------- 2 files changed, 6 insertions(+), 20 deletions(-)
diff --git a/cmd/net.c b/cmd/net.c index eca6dd8918..89721b8f8b 100644 --- a/cmd/net.c +++ b/cmd/net.c @@ -192,6 +192,9 @@ static int netboot_common(enum proto_t proto, cmd_tbl_t *cmdtp, int argc,
switch (argc) { case 1: + /* refresh bootfile name from env */ + copy_filename(net_boot_file_name, env_get("bootfile"), + sizeof(net_boot_file_name)); break;
case 2: /* @@ -203,6 +206,9 @@ static int netboot_common(enum proto_t proto, cmd_tbl_t *cmdtp, int argc, addr = simple_strtoul(argv[1], &end, 16); if (end == (argv[1] + strlen(argv[1]))) { load_addr = addr; + /* refresh bootfile name from env */ + copy_filename(net_boot_file_name, env_get("bootfile"), + sizeof(net_boot_file_name)); } else { net_boot_file_name_explicit = true; copy_filename(net_boot_file_name, argv[1], diff --git a/net/net.c b/net/net.c index 333102ea79..1b6781d358 100644 --- a/net/net.c +++ b/net/net.c @@ -216,26 +216,6 @@ int __maybe_unused net_busy_flag;
/**********************************************************************/
-static int on_bootfile(const char *name, const char *value, enum env_op op, - int flags) -{ - if (flags & H_PROGRAMMATIC) - return 0; - - switch (op) { - case env_op_create: - case env_op_overwrite: - copy_filename(net_boot_file_name, value, - sizeof(net_boot_file_name)); - break; - default: - break; - } - - return 0; -} -U_BOOT_ENV_CALLBACK(bootfile, on_bootfile); - static int on_ipaddr(const char *name, const char *value, enum env_op op, int flags) {
Hi Joe,
https://patchwork.ozlabs.org/patch/939044/ was applied to http://git.denx.de/?p=u-boot/u-boot-net.git
Thanks! -Joe
The same basic parsing was implemented in tftp and nfs, so add a helper function to do the work once.
Signed-off-by: Joe Hershberger joe.hershberger@ni.com
---
include/net.h | 11 +++++++++++ net/net.c | 20 ++++++++++++++++++++ net/nfs.c | 15 ++------------- net/tftp.c | 13 +------------ 4 files changed, 34 insertions(+), 25 deletions(-)
diff --git a/include/net.h b/include/net.h index de2d7bba19..62f82c4dca 100644 --- a/include/net.h +++ b/include/net.h @@ -842,6 +842,17 @@ void copy_filename(char *dst, const char *src, int size); /* check if serverip is specified in filename from the command line */ int is_serverip_in_cmd(void);
+/** + * net_parse_bootfile - Parse the bootfile env var / cmd line param + * + * @param ipaddr - a pointer to the ipaddr to populate if included in bootfile + * @param filename - a pointer to the string to save the filename part + * @param max_len - The longest - 1 that the filename part can be + * + * return 1 if parsed, 0 if bootfile is empty + */ +int net_parse_bootfile(struct in_addr *ipaddr, char *filename, int max_len); + /* get a random source port */ unsigned int random_port(void);
diff --git a/net/net.c b/net/net.c index 1b6781d358..31cf306ae7 100644 --- a/net/net.c +++ b/net/net.c @@ -1517,6 +1517,26 @@ int is_serverip_in_cmd(void) return !!strchr(net_boot_file_name, ':'); }
+int net_parse_bootfile(struct in_addr *ipaddr, char *filename, int max_len) +{ + char *colon; + + if (net_boot_file_name[0] == '\0') + return 0; + + colon = strchr(net_boot_file_name, ':'); + if (colon) { + if (ipaddr) + *ipaddr = string_to_ip(net_boot_file_name); + strncpy(filename, colon + 1, max_len); + } else { + strncpy(filename, net_boot_file_name, max_len); + } + filename[max_len - 1] = '\0'; + + return 1; +} + #if defined(CONFIG_CMD_NFS) || \ defined(CONFIG_CMD_SNTP) || \ defined(CONFIG_CMD_DNS) diff --git a/net/nfs.c b/net/nfs.c index 9a16765ba1..ed6a64d722 100644 --- a/net/nfs.c +++ b/net/nfs.c @@ -859,7 +859,8 @@ void nfs_start(void) return; }
- if (net_boot_file_name[0] == '\0') { + if (!net_parse_bootfile(&nfs_server_ip, nfs_path, + sizeof(nfs_path_buff))) { sprintf(nfs_path, "/nfsroot/%02X%02X%02X%02X.img", net_ip.s_addr & 0xFF, (net_ip.s_addr >> 8) & 0xFF, @@ -868,18 +869,6 @@ void nfs_start(void)
debug("*** Warning: no boot file name; using '%s'\n", nfs_path); - } else { - char *p = net_boot_file_name; - - p = strchr(p, ':'); - - if (p != NULL) { - nfs_server_ip = string_to_ip(net_boot_file_name); - ++p; - strcpy(nfs_path, p); - } else { - strcpy(nfs_path, net_boot_file_name); - } }
nfs_filename = basename(nfs_path); diff --git a/net/tftp.c b/net/tftp.c index 6671b1f7ca..68ffd81414 100644 --- a/net/tftp.c +++ b/net/tftp.c @@ -735,7 +735,7 @@ void tftp_start(enum proto_t protocol) tftp_block_size_option, timeout_ms);
tftp_remote_ip = net_server_ip; - if (net_boot_file_name[0] == '\0') { + if (!net_parse_bootfile(&tftp_remote_ip, tftp_filename, MAX_LEN)) { sprintf(default_filename, "%02X%02X%02X%02X.img", net_ip.s_addr & 0xFF, (net_ip.s_addr >> 8) & 0xFF, @@ -747,17 +747,6 @@ void tftp_start(enum proto_t protocol)
printf("*** Warning: no boot file name; using '%s'\n", tftp_filename); - } else { - char *p = strchr(net_boot_file_name, ':'); - - if (p == NULL) { - strncpy(tftp_filename, net_boot_file_name, MAX_LEN); - tftp_filename[MAX_LEN - 1] = 0; - } else { - tftp_remote_ip = string_to_ip(net_boot_file_name); - strncpy(tftp_filename, p + 1, MAX_LEN); - tftp_filename[MAX_LEN - 1] = 0; - } }
printf("Using %s device\n", eth_get_name());
Hi Joe,
https://patchwork.ozlabs.org/patch/939045/ was applied to http://git.denx.de/?p=u-boot/u-boot-net.git
Thanks! -Joe
On 07/04/2018 02:36 AM, Joe Hershberger wrote:
For net_boot_common, we allow the serverip to be specified as part of the boot file name. For net commands that require serverip, include that source as a valid specification of serverip.
Signed-off-by: Joe Hershberger joe.hershberger@ni.com
Reviewed-by: Alexander Graf agraf@suse.de
Alex
Hi Joe,
https://patchwork.ozlabs.org/patch/939043/ was applied to http://git.denx.de/?p=u-boot/u-boot-net.git
Thanks! -Joe
participants (2)
-
Alexander Graf -
Joe Hershberger