[v4,10/14] net-lwip: add wget command

Add support for the wget command with NET_LWIP.

Based on code initially developed by Maxim U.

Signed-off-by: Jerome Forissier <jerome.forissier@linaro.org>
Co-developed-by: Maxim Uvarov <muvarov@gmail.com>
Cc: Maxim Uvarov <muvarov@gmail.com>
Signed-off-by: Jerome Forissier <jerome.forissier@linaro.org>
---
 cmd/Kconfig        |   7 ++
 cmd/net-lwip.c     |   8 ++
 include/net-lwip.h |  18 +++
 net-lwip/Makefile  |   1 +
 net-lwip/wget.c    | 269 +++++++++++++++++++++++++++++++++++++++++++++
 5 files changed, 303 insertions(+)
 create mode 100644 net-lwip/wget.c

Jerome Forissier <jerome.forissier@linaro.org> writes:

> Add support for the wget command with NET_LWIP.
>
> Based on code initially developed by Maxim U.
>
> Signed-off-by: Jerome Forissier <jerome.forissier@linaro.org>
> Co-developed-by: Maxim Uvarov <muvarov@gmail.com>
> Cc: Maxim Uvarov <muvarov@gmail.com>
> Signed-off-by: Jerome Forissier <jerome.forissier@linaro.org>
> ---
>  cmd/Kconfig        |   7 ++
>  cmd/net-lwip.c     |   8 ++
>  include/net-lwip.h |  18 +++
>  net-lwip/Makefile  |   1 +
>  net-lwip/wget.c    | 269 +++++++++++++++++++++++++++++++++++++++++++++
>  5 files changed, 303 insertions(+)
>  create mode 100644 net-lwip/wget.c
>
> diff --git a/cmd/Kconfig b/cmd/Kconfig
> index 6ef0b52cd34..d9a86540be6 100644
> --- a/cmd/Kconfig
> +++ b/cmd/Kconfig
> @@ -2117,6 +2117,13 @@ config CMD_TFTPBOOT
>  	help
>  	  tftpboot - load file via network using TFTP protocol
>  
> +config CMD_WGET
> +	bool "wget"
> +	select PROT_TCP_LWIP
> +	help
> +	  wget is a simple command to download kernel, or other files,
> +	  from a http server over TCP.
> +
>  endif
>  
>  endif
> diff --git a/cmd/net-lwip.c b/cmd/net-lwip.c
> index c021da6a674..42f8bd6b259 100644
> --- a/cmd/net-lwip.c
> +++ b/cmd/net-lwip.c
> @@ -35,3 +35,11 @@ U_BOOT_CMD(
>  	"hostname [envvar]"
>  );
>  #endif
> +
> +#if defined(CONFIG_CMD_WGET)
> +U_BOOT_CMD(
> +	wget,   3,      1,      do_wget,
> +	"boot image via network using HTTP protocol",
> +	"[loadAddress] URL"
> +);
> +#endif
> diff --git a/include/net-lwip.h b/include/net-lwip.h
> index 4d41b0208a3..ddf25e61417 100644
> --- a/include/net-lwip.h
> +++ b/include/net-lwip.h
> @@ -11,8 +11,26 @@ struct netif *net_lwip_new_netif_noip(void);
>  void net_lwip_remove_netif(struct netif *netif);
>  struct netif *net_lwip_get_netif(void);
>  
> +/**
> + * wget_with_dns() - runs dns host IP address resulution before wget
> + *
> + * @dst_addr:	destination address to download the file
> + * @uri:	uri string of target file of wget
> + * Return:	downloaded file size, negative if failed
> + */
> +
> +int wget_with_dns(ulong dst_addr, char *uri);
> +/**
> + * wget_validate_uri() - varidate the uri
> + *
> + * @uri:	uri string of target file of wget
> + * Return:	true if uri is valid, false if uri is invalid
> + */
> +bool wget_validate_uri(char *uri);
> +
>  int do_dhcp(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]);
>  int do_dns(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]);
>  int do_ping(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]);
> +int do_wget(struct cmd_tbl *cmdtp, int flag, int argc, char * const argv[]);
>  
>  #endif /* __NET_LWIP_H__ */
> diff --git a/net-lwip/Makefile b/net-lwip/Makefile
> index aa247859483..bc011bb0e32 100644
> --- a/net-lwip/Makefile
> +++ b/net-lwip/Makefile
> @@ -10,6 +10,7 @@ obj-$(CONFIG_CMD_DHCP) += dhcp.o
>  obj-$(CONFIG_CMD_DNS) += dns.o
>  obj-$(CONFIG_CMD_PING) += ping.o
>  obj-$(CONFIG_CMD_TFTPBOOT) += tftp.o
> +obj-$(CONFIG_CMD_WGET) += wget.o
>  
>  # Disable this warning as it is triggered by:
>  # sprintf(buf, index ? "foo%d" : "foo", index)
> diff --git a/net-lwip/wget.c b/net-lwip/wget.c
> new file mode 100644
> index 00000000000..069299bd469
> --- /dev/null
> +++ b/net-lwip/wget.c
> @@ -0,0 +1,269 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/* Copyright (C) 2024 Linaro Ltd. */
> +
> +#include <command.h>
> +#include <console.h>
> +#include <display_options.h>
> +#include <image.h>
> +#include <lwip/apps/http_client.h>
> +#include <lwip/timeouts.h>
> +#include <net.h>
> +#include <time.h>
> +
> +#define SERVER_NAME_SIZE 200
> +#define HTTP_PORT_DEFAULT 80
> +#define PROGRESS_PRINT_STEP_BYTES (100 * 1024)
> +
> +enum done_state {
> +        NOT_DONE = 0,
> +        SUCCESS = 1,
> +        FAILURE = 2
> +};
> +
> +struct wget_ctx {
> +	ulong daddr;
> +	ulong saved_daddr;
> +	ulong size;
> +	ulong prevsize;
> +	ulong start_time;
> +	enum done_state done;
> +};
> +
> +static int parse_url(char *url, char *host, u16 *port, char **path)
> +{
> +	char *p, *pp;
> +	long lport;
> +
> +	p = strstr(url, "http://");
> +	if (!p)
> +		return -EINVAL;
> +
> +	p += strlen("http://");
> +
> +	/* Parse hostname */
> +	pp = strchr(p, ':');
> +	if (!pp)
> +		pp = strchr(p, '/');
> +	if (!pp)
> +		return -EINVAL;
> +
> +	if (p + SERVER_NAME_SIZE <= pp)
> +		return -EINVAL;
> +
> +	memcpy(host, p, pp - p);
> +	host[pp - p + 1] = '\0';
> +
> +	if (*pp == ':') {
> +		/* Parse port number */
> +		p = pp + 1;
> +		lport = simple_strtol(p, &pp, 10);
> +		if (pp && *pp != '/')
> +			return -EINVAL;
> +		if (lport > 65535)
> +			return -EINVAL;
> +		*port = (u16)lport;
> +	} else {
> +		*port = HTTP_PORT_DEFAULT;
> +	}
> +	if (*pp != '/')
> +		return -EINVAL;
> +	*path = pp;
> +
> +	return 0;
> +}
> +
> +static err_t httpc_recv_cb(void *arg, struct altcp_pcb *pcb, struct pbuf *pbuf,
> +			   err_t err)
> +{
> +	struct wget_ctx *ctx = arg;
> +	struct pbuf *buf;
> +
> +	if (!pbuf)
> +		return ERR_BUF;
> +
> +	for (buf = pbuf; buf; buf = buf->next) {
> +		memcpy((void *)ctx->daddr, buf->payload, buf->len);
> +		ctx->daddr += buf->len;
> +		ctx->size += buf->len;
> +		if (ctx->size - ctx->prevsize > PROGRESS_PRINT_STEP_BYTES) {
> +			printf("#");
> +			ctx->prevsize = ctx->size;
> +		}
> +	}
> +
> +	altcp_recved(pcb, pbuf->tot_len);
> +	pbuf_free(pbuf);
> +	return ERR_OK;
> +}
> +
> +static void httpc_result_cb(void *arg, httpc_result_t httpc_result,
> +			    u32_t rx_content_len, u32_t srv_res, err_t err)
> +{
> +	struct wget_ctx *ctx = arg;
> +	ulong elapsed;
> +
> +	if (httpc_result != HTTPC_RESULT_OK) {
> +		log_err("\nHTTP client error %d\n", httpc_result);
> +		ctx->done = FAILURE;
> +		return;
> +	}
> +
> +	elapsed = get_timer(ctx->start_time);
> +	if (rx_content_len > PROGRESS_PRINT_STEP_BYTES)
> +		printf("\n");
> +        printf("%u bytes transferred in %lu ms (", rx_content_len,
> +	       get_timer(ctx->start_time));
> +        print_size(rx_content_len / elapsed * 1000, "/s)\n");
> +
> +	if (env_set_hex("filesize", rx_content_len) ||
> +	    env_set_hex("fileaddr", ctx->saved_daddr)) {
> +		log_err("Could not set filesize or fileaddr\n");
> +		ctx->done = FAILURE;
> +		return;
> +	}
> +
> +	ctx->done = SUCCESS;
> +}
> +
> +int wget_with_dns(ulong dst_addr, char *uri)
> +{
> +	char server_name[SERVER_NAME_SIZE];
> +	httpc_connection_t conn;
> +	httpc_state_t *state;
> +	struct netif *netif;
> +	struct wget_ctx ctx;
> +	char *path;
> +	u16 port;
> +
> +	ctx.daddr = dst_addr;
> +	ctx.saved_daddr = dst_addr;
> +	ctx.done = NOT_DONE;
> +	ctx.size = 0;
> +	ctx.prevsize = 0;
> +
> +	if (parse_url(uri, server_name, &port, &path))
> +		return CMD_RET_USAGE;
> +
> +	netif = net_lwip_new_netif();
> +	if (!netif)
> +		return -1;
> +
> +	memset(&conn, 0, sizeof(conn));
> +	conn.result_fn = httpc_result_cb;
> +	ctx.start_time = get_timer(0);
> +	if (httpc_get_file_dns(server_name, port, path, &conn, httpc_recv_cb,
> +			       &ctx, &state)) {
> +		net_lwip_remove_netif(netif);
> +		return CMD_RET_FAILURE;
> +	}
> +
> +	while (!ctx.done) {
> +		eth_rx();
> +		sys_check_timeouts();
> +		if (ctrlc())
> +			break;
> +	}
> +
> +	net_lwip_remove_netif(netif);
> +
> +	if (ctx.done == SUCCESS)
> +		return 0;
> +
> +	return -1;
> +}
> +
> +int do_wget(struct cmd_tbl *cmdtp, int flag, int argc, char * const argv[])
> +{
> +	char *end;
> +	char *url;
> +	ulong dst_addr;
> +
> +	if (argc < 2 || argc > 3)
> +		return CMD_RET_USAGE;
> +
> +	dst_addr = hextoul(argv[1], &end);
> +        if (end == (argv[1] + strlen(argv[1]))) {
> +		if (argc < 3)
> +			return CMD_RET_USAGE;
> +		url = argv[2];
> +	} else {
> +		dst_addr = image_load_addr;
> +		url = argv[1];
> +	}
> +
> +	if (wget_with_dns(dst_addr, url))
> +		return CMD_RET_FAILURE;
> +
> +	return CMD_RET_SUCCESS;
> +}
> +
> +/**
> + * wget_validate_uri() - validate the uri for wget
> + *
> + * @uri:	uri string
> + *
> + * This function follows the current U-Boot wget implementation.
> + * scheme: only "http:" is supported
> + * authority:
> + *   - user information: not supported
> + *   - host: supported
> + *   - port: not supported(always use the default port)
> + *
> + * Uri is expected to be correctly percent encoded.
> + * This is the minimum check, control codes(0x1-0x19, 0x7F, except '\0')
> + * and space character(0x20) are not allowed.
> + *
> + * TODO: stricter uri conformance check
> + *
> + * Return:	true on success, false on failure
> + */
> +bool wget_validate_uri(char *uri)
> +{
> +	char c;
> +	bool ret = true;
> +	char *str_copy, *s, *authority;
> +
> +	for (c = 0x1; c < 0x21; c++) {
> +		if (strchr(uri, c)) {
> +			log_err("invalid character is used\n");
> +			return false;
> +		}
> +	}
> +	if (strchr(uri, 0x7f)) {
> +		log_err("invalid character is used\n");
> +		return false;
> +	}
> +
> +	if (strncmp(uri, "http://", 7)) {
> +		log_err("only http:// is supported\n");
> +		return false;
> +	}
> +	str_copy = strdup(uri);
> +	if (!str_copy)
> +		return false;
> +
> +	s = str_copy + strlen("http://");
> +	authority = strsep(&s, "/");
> +	if (!s) {
> +		log_err("invalid uri, no file path\n");
> +		ret = false;
> +		goto out;
> +	}
> +	s = strchr(authority, '@');
> +	if (s) {
> +		log_err("user information is not supported\n");
> +		ret = false;
> +		goto out;
> +	}
> +	s = strchr(authority, ':');
> +	if (s) {
> +		log_err("user defined port is not supported\n");
> +		ret = false;
> +		goto out;
> +	}
> +
> +out:
> +	free(str_copy);
> +
> +	return ret;
> +}
> -- 
> 2.40.1

Hi Jerome, I am trying out the lwIP stack on TI boards.

For wget, I noticed a bug and a user experience improvement. The bug is an
off-by-one array access when parsing the url. The improvement emits a
message if the url isn't an http://, since that is all we are supporting,
and without the message, the wget command silently fails, which is
confusing to the user.

I will post the 2 patches as a reply to this email. LMK if you want to use
another method to collaborate. I can already see that efi http boot support
will not work. wget_validate_uri() looks to be copied vertabim from the
current implementation, and IMO we should completely remove it.

Jon

Subject: [PATCH] net-lwip: fixes off-by-one array access error with wget

When wget parses the url and extracts the host, it is off by one on the
index to terminate the character array.

Signed-off-by: Jonathan Humphreys <j-humphreys@ti.com>
---
 net-lwip/wget.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net-lwip/wget.c b/net-lwip/wget.c
index 069299bd469..63b19b99112 100644
--- a/net-lwip/wget.c
+++ b/net-lwip/wget.c
@@ -51,7 +51,7 @@ static int parse_url(char *url, char *host, u16 *port, char **path)
 		return -EINVAL;
 
 	memcpy(host, p, pp - p);
-	host[pp - p + 1] = '\0';
+	host[pp - p] = '\0';
 
 	if (*pp == ':') {
 		/* Parse port number */

Subject: [PATCH] net-lwip: Add message if not using http:// for wget

U-Boot's wget only supports http://, so give the user a clue if they don't
use it.

Signed-off-by: Jonathan Humphreys <j-humphreys@ti.com>
---
 net-lwip/wget.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/net-lwip/wget.c b/net-lwip/wget.c
index 63b19b99112..6b9c953ef51 100644
--- a/net-lwip/wget.c
+++ b/net-lwip/wget.c
@@ -35,8 +35,10 @@ static int parse_url(char *url, char *host, u16 *port, char **path)
 	long lport;
 
 	p = strstr(url, "http://");
-	if (!p)
+	if (!p) {
+		log_err("only http:// is supported\n");
 		return -EINVAL;
+	}
 
 	p += strlen("http://");

On 6/24/24 08:21, Jon Humphreys wrote:
> Jerome Forissier <jerome.forissier@linaro.org> writes:
> 
>> Add support for the wget command with NET_LWIP.
>>
>> Based on code initially developed by Maxim U.
>>
>> Signed-off-by: Jerome Forissier <jerome.forissier@linaro.org>
>> Co-developed-by: Maxim Uvarov <muvarov@gmail.com>
>> Cc: Maxim Uvarov <muvarov@gmail.com>
>> Signed-off-by: Jerome Forissier <jerome.forissier@linaro.org>
>> ---
>>  cmd/Kconfig        |   7 ++
>>  cmd/net-lwip.c     |   8 ++
>>  include/net-lwip.h |  18 +++
>>  net-lwip/Makefile  |   1 +
>>  net-lwip/wget.c    | 269 +++++++++++++++++++++++++++++++++++++++++++++
>>  5 files changed, 303 insertions(+)
>>  create mode 100644 net-lwip/wget.c
>>
>> diff --git a/cmd/Kconfig b/cmd/Kconfig
>> index 6ef0b52cd34..d9a86540be6 100644
>> --- a/cmd/Kconfig
>> +++ b/cmd/Kconfig
>> @@ -2117,6 +2117,13 @@ config CMD_TFTPBOOT
>>  	help
>>  	  tftpboot - load file via network using TFTP protocol
>>  
>> +config CMD_WGET
>> +	bool "wget"
>> +	select PROT_TCP_LWIP
>> +	help
>> +	  wget is a simple command to download kernel, or other files,
>> +	  from a http server over TCP.
>> +
>>  endif
>>  
>>  endif
>> diff --git a/cmd/net-lwip.c b/cmd/net-lwip.c
>> index c021da6a674..42f8bd6b259 100644
>> --- a/cmd/net-lwip.c
>> +++ b/cmd/net-lwip.c
>> @@ -35,3 +35,11 @@ U_BOOT_CMD(
>>  	"hostname [envvar]"
>>  );
>>  #endif
>> +
>> +#if defined(CONFIG_CMD_WGET)
>> +U_BOOT_CMD(
>> +	wget,   3,      1,      do_wget,
>> +	"boot image via network using HTTP protocol",
>> +	"[loadAddress] URL"
>> +);
>> +#endif
>> diff --git a/include/net-lwip.h b/include/net-lwip.h
>> index 4d41b0208a3..ddf25e61417 100644
>> --- a/include/net-lwip.h
>> +++ b/include/net-lwip.h
>> @@ -11,8 +11,26 @@ struct netif *net_lwip_new_netif_noip(void);
>>  void net_lwip_remove_netif(struct netif *netif);
>>  struct netif *net_lwip_get_netif(void);
>>  
>> +/**
>> + * wget_with_dns() - runs dns host IP address resulution before wget
>> + *
>> + * @dst_addr:	destination address to download the file
>> + * @uri:	uri string of target file of wget
>> + * Return:	downloaded file size, negative if failed
>> + */
>> +
>> +int wget_with_dns(ulong dst_addr, char *uri);
>> +/**
>> + * wget_validate_uri() - varidate the uri
>> + *
>> + * @uri:	uri string of target file of wget
>> + * Return:	true if uri is valid, false if uri is invalid
>> + */
>> +bool wget_validate_uri(char *uri);
>> +
>>  int do_dhcp(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]);
>>  int do_dns(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]);
>>  int do_ping(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]);
>> +int do_wget(struct cmd_tbl *cmdtp, int flag, int argc, char * const argv[]);
>>  
>>  #endif /* __NET_LWIP_H__ */
>> diff --git a/net-lwip/Makefile b/net-lwip/Makefile
>> index aa247859483..bc011bb0e32 100644
>> --- a/net-lwip/Makefile
>> +++ b/net-lwip/Makefile
>> @@ -10,6 +10,7 @@ obj-$(CONFIG_CMD_DHCP) += dhcp.o
>>  obj-$(CONFIG_CMD_DNS) += dns.o
>>  obj-$(CONFIG_CMD_PING) += ping.o
>>  obj-$(CONFIG_CMD_TFTPBOOT) += tftp.o
>> +obj-$(CONFIG_CMD_WGET) += wget.o
>>  
>>  # Disable this warning as it is triggered by:
>>  # sprintf(buf, index ? "foo%d" : "foo", index)
>> diff --git a/net-lwip/wget.c b/net-lwip/wget.c
>> new file mode 100644
>> index 00000000000..069299bd469
>> --- /dev/null
>> +++ b/net-lwip/wget.c
>> @@ -0,0 +1,269 @@
>> +// SPDX-License-Identifier: GPL-2.0+
>> +/* Copyright (C) 2024 Linaro Ltd. */
>> +
>> +#include <command.h>
>> +#include <console.h>
>> +#include <display_options.h>
>> +#include <image.h>
>> +#include <lwip/apps/http_client.h>
>> +#include <lwip/timeouts.h>
>> +#include <net.h>
>> +#include <time.h>
>> +
>> +#define SERVER_NAME_SIZE 200
>> +#define HTTP_PORT_DEFAULT 80
>> +#define PROGRESS_PRINT_STEP_BYTES (100 * 1024)
>> +
>> +enum done_state {
>> +        NOT_DONE = 0,
>> +        SUCCESS = 1,
>> +        FAILURE = 2
>> +};
>> +
>> +struct wget_ctx {
>> +	ulong daddr;
>> +	ulong saved_daddr;
>> +	ulong size;
>> +	ulong prevsize;
>> +	ulong start_time;
>> +	enum done_state done;
>> +};
>> +
>> +static int parse_url(char *url, char *host, u16 *port, char **path)
>> +{
>> +	char *p, *pp;
>> +	long lport;
>> +
>> +	p = strstr(url, "http://");
>> +	if (!p)
>> +		return -EINVAL;
>> +
>> +	p += strlen("http://");
>> +
>> +	/* Parse hostname */
>> +	pp = strchr(p, ':');
>> +	if (!pp)
>> +		pp = strchr(p, '/');
>> +	if (!pp)
>> +		return -EINVAL;
>> +
>> +	if (p + SERVER_NAME_SIZE <= pp)
>> +		return -EINVAL;
>> +
>> +	memcpy(host, p, pp - p);
>> +	host[pp - p + 1] = '\0';
>> +
>> +	if (*pp == ':') {
>> +		/* Parse port number */
>> +		p = pp + 1;
>> +		lport = simple_strtol(p, &pp, 10);
>> +		if (pp && *pp != '/')
>> +			return -EINVAL;
>> +		if (lport > 65535)
>> +			return -EINVAL;
>> +		*port = (u16)lport;
>> +	} else {
>> +		*port = HTTP_PORT_DEFAULT;
>> +	}
>> +	if (*pp != '/')
>> +		return -EINVAL;
>> +	*path = pp;
>> +
>> +	return 0;
>> +}
>> +
>> +static err_t httpc_recv_cb(void *arg, struct altcp_pcb *pcb, struct pbuf *pbuf,
>> +			   err_t err)
>> +{
>> +	struct wget_ctx *ctx = arg;
>> +	struct pbuf *buf;
>> +
>> +	if (!pbuf)
>> +		return ERR_BUF;
>> +
>> +	for (buf = pbuf; buf; buf = buf->next) {
>> +		memcpy((void *)ctx->daddr, buf->payload, buf->len);
>> +		ctx->daddr += buf->len;
>> +		ctx->size += buf->len;
>> +		if (ctx->size - ctx->prevsize > PROGRESS_PRINT_STEP_BYTES) {
>> +			printf("#");
>> +			ctx->prevsize = ctx->size;
>> +		}
>> +	}
>> +
>> +	altcp_recved(pcb, pbuf->tot_len);
>> +	pbuf_free(pbuf);
>> +	return ERR_OK;
>> +}
>> +
>> +static void httpc_result_cb(void *arg, httpc_result_t httpc_result,
>> +			    u32_t rx_content_len, u32_t srv_res, err_t err)
>> +{
>> +	struct wget_ctx *ctx = arg;
>> +	ulong elapsed;
>> +
>> +	if (httpc_result != HTTPC_RESULT_OK) {
>> +		log_err("\nHTTP client error %d\n", httpc_result);
>> +		ctx->done = FAILURE;
>> +		return;
>> +	}
>> +
>> +	elapsed = get_timer(ctx->start_time);
>> +	if (rx_content_len > PROGRESS_PRINT_STEP_BYTES)
>> +		printf("\n");
>> +        printf("%u bytes transferred in %lu ms (", rx_content_len,
>> +	       get_timer(ctx->start_time));
>> +        print_size(rx_content_len / elapsed * 1000, "/s)\n");
>> +
>> +	if (env_set_hex("filesize", rx_content_len) ||
>> +	    env_set_hex("fileaddr", ctx->saved_daddr)) {
>> +		log_err("Could not set filesize or fileaddr\n");
>> +		ctx->done = FAILURE;
>> +		return;
>> +	}
>> +
>> +	ctx->done = SUCCESS;
>> +}
>> +
>> +int wget_with_dns(ulong dst_addr, char *uri)
>> +{
>> +	char server_name[SERVER_NAME_SIZE];
>> +	httpc_connection_t conn;
>> +	httpc_state_t *state;
>> +	struct netif *netif;
>> +	struct wget_ctx ctx;
>> +	char *path;
>> +	u16 port;
>> +
>> +	ctx.daddr = dst_addr;
>> +	ctx.saved_daddr = dst_addr;
>> +	ctx.done = NOT_DONE;
>> +	ctx.size = 0;
>> +	ctx.prevsize = 0;
>> +
>> +	if (parse_url(uri, server_name, &port, &path))
>> +		return CMD_RET_USAGE;
>> +
>> +	netif = net_lwip_new_netif();
>> +	if (!netif)
>> +		return -1;
>> +
>> +	memset(&conn, 0, sizeof(conn));
>> +	conn.result_fn = httpc_result_cb;
>> +	ctx.start_time = get_timer(0);
>> +	if (httpc_get_file_dns(server_name, port, path, &conn, httpc_recv_cb,
>> +			       &ctx, &state)) {
>> +		net_lwip_remove_netif(netif);
>> +		return CMD_RET_FAILURE;
>> +	}
>> +
>> +	while (!ctx.done) {
>> +		eth_rx();
>> +		sys_check_timeouts();
>> +		if (ctrlc())
>> +			break;
>> +	}
>> +
>> +	net_lwip_remove_netif(netif);
>> +
>> +	if (ctx.done == SUCCESS)
>> +		return 0;
>> +
>> +	return -1;
>> +}
>> +
>> +int do_wget(struct cmd_tbl *cmdtp, int flag, int argc, char * const argv[])
>> +{
>> +	char *end;
>> +	char *url;
>> +	ulong dst_addr;
>> +
>> +	if (argc < 2 || argc > 3)
>> +		return CMD_RET_USAGE;
>> +
>> +	dst_addr = hextoul(argv[1], &end);
>> +        if (end == (argv[1] + strlen(argv[1]))) {
>> +		if (argc < 3)
>> +			return CMD_RET_USAGE;
>> +		url = argv[2];
>> +	} else {
>> +		dst_addr = image_load_addr;
>> +		url = argv[1];
>> +	}
>> +
>> +	if (wget_with_dns(dst_addr, url))
>> +		return CMD_RET_FAILURE;
>> +
>> +	return CMD_RET_SUCCESS;
>> +}
>> +
>> +/**
>> + * wget_validate_uri() - validate the uri for wget
>> + *
>> + * @uri:	uri string
>> + *
>> + * This function follows the current U-Boot wget implementation.
>> + * scheme: only "http:" is supported
>> + * authority:
>> + *   - user information: not supported
>> + *   - host: supported
>> + *   - port: not supported(always use the default port)
>> + *
>> + * Uri is expected to be correctly percent encoded.
>> + * This is the minimum check, control codes(0x1-0x19, 0x7F, except '\0')
>> + * and space character(0x20) are not allowed.
>> + *
>> + * TODO: stricter uri conformance check
>> + *
>> + * Return:	true on success, false on failure
>> + */
>> +bool wget_validate_uri(char *uri)
>> +{
>> +	char c;
>> +	bool ret = true;
>> +	char *str_copy, *s, *authority;
>> +
>> +	for (c = 0x1; c < 0x21; c++) {
>> +		if (strchr(uri, c)) {
>> +			log_err("invalid character is used\n");
>> +			return false;
>> +		}
>> +	}
>> +	if (strchr(uri, 0x7f)) {
>> +		log_err("invalid character is used\n");
>> +		return false;
>> +	}
>> +
>> +	if (strncmp(uri, "http://", 7)) {
>> +		log_err("only http:// is supported\n");
>> +		return false;
>> +	}
>> +	str_copy = strdup(uri);
>> +	if (!str_copy)
>> +		return false;
>> +
>> +	s = str_copy + strlen("http://");
>> +	authority = strsep(&s, "/");
>> +	if (!s) {
>> +		log_err("invalid uri, no file path\n");
>> +		ret = false;
>> +		goto out;
>> +	}
>> +	s = strchr(authority, '@');
>> +	if (s) {
>> +		log_err("user information is not supported\n");
>> +		ret = false;
>> +		goto out;
>> +	}
>> +	s = strchr(authority, ':');
>> +	if (s) {
>> +		log_err("user defined port is not supported\n");
>> +		ret = false;
>> +		goto out;
>> +	}
>> +
>> +out:
>> +	free(str_copy);
>> +
>> +	return ret;
>> +}
>> -- 
>> 2.40.1

Hi Jon,

> Hi Jerome, I am trying out the lwIP stack on TI boards.
> 
> For wget, I noticed a bug and a user experience improvement. The bug is an
> off-by-one array access when parsing the url.

Indeed, someone reported this bug to me off-list and it's already fixed in
my WIP branch.

> The improvement emits a
> message if the url isn't an http://, since that is all we are supporting,
> and without the message, the wget command silently fails, which is
> confusing to the user.

I agree.

> I will post the 2 patches as a reply to this email. LMK if you want to use
> another method to collaborate. I can already see that efi http boot support
> will not work. wget_validate_uri() looks to be copied vertabim from the
> current implementation, and IMO we should completely remove it.

It is indeed copied. It is needed to avoid an unresolved symbol but I
understand more work may be needed for EFI HTTP boot support (not tested by
me).

If you are willing to help with this then you are welcome to post patches as
replies and I will integrate them in the series.

Thanks,

On 6/24/24 08:25, Jon Humphreys wrote:
> Subject: [PATCH] net-lwip: fixes off-by-one array access error with wget
> 
> When wget parses the url and extracts the host, it is off by one on the
> index to terminate the character array.
> 
> Signed-off-by: Jonathan Humphreys <j-humphreys@ti.com>
> ---
>  net-lwip/wget.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/net-lwip/wget.c b/net-lwip/wget.c
> index 069299bd469..63b19b99112 100644
> --- a/net-lwip/wget.c
> +++ b/net-lwip/wget.c
> @@ -51,7 +51,7 @@ static int parse_url(char *url, char *host, u16 *port, char **path)
>  		return -EINVAL;
>  
>  	memcpy(host, p, pp - p);
> -	host[pp - p + 1] = '\0';
> +	host[pp - p] = '\0';
>  
>  	if (*pp == ':') {
>  		/* Parse port number */

Hi Jon,

As I said previously I am ignoring this patch since I already have it in
my working tree. Thanks for reporting.

On 6/24/24 08:26, Jon Humphreys wrote:
> Subject: [PATCH] net-lwip: Add message if not using http:// for wget
> 
> U-Boot's wget only supports http://, so give the user a clue if they don't
> use it.
> 
> Signed-off-by: Jonathan Humphreys <j-humphreys@ti.com>
> ---
>  net-lwip/wget.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/net-lwip/wget.c b/net-lwip/wget.c
> index 63b19b99112..6b9c953ef51 100644
> --- a/net-lwip/wget.c
> +++ b/net-lwip/wget.c
> @@ -35,8 +35,10 @@ static int parse_url(char *url, char *host, u16 *port, char **path)
>  	long lport;
>  
>  	p = strstr(url, "http://");
> -	if (!p)
> +	if (!p) {
> +		log_err("only http:// is supported\n");
>  		return -EINVAL;
> +	}
>  
>  	p += strlen("http://");
>  

I am folding that into "net-lwip: add wget command" if you don't mind.
Thanks!

Jerome Forissier <jerome.forissier@linaro.org> writes:

> On 6/24/24 08:21, Jon Humphreys wrote:
>> Jerome Forissier <jerome.forissier@linaro.org> writes:
>> 
>>> Add support for the wget command with NET_LWIP.
>>>
>>> Based on code initially developed by Maxim U.
>>>
>>> Signed-off-by: Jerome Forissier <jerome.forissier@linaro.org>
>>> Co-developed-by: Maxim Uvarov <muvarov@gmail.com>
>>> Cc: Maxim Uvarov <muvarov@gmail.com>
>>> Signed-off-by: Jerome Forissier <jerome.forissier@linaro.org>
>>> ---
>>>  cmd/Kconfig        |   7 ++
>>>  cmd/net-lwip.c     |   8 ++
>>>  include/net-lwip.h |  18 +++
>>>  net-lwip/Makefile  |   1 +
>>>  net-lwip/wget.c    | 269 +++++++++++++++++++++++++++++++++++++++++++++
>>>  5 files changed, 303 insertions(+)
>>>  create mode 100644 net-lwip/wget.c
>>>
>>> diff --git a/cmd/Kconfig b/cmd/Kconfig
>>> index 6ef0b52cd34..d9a86540be6 100644
>>> --- a/cmd/Kconfig
>>> +++ b/cmd/Kconfig
>>> @@ -2117,6 +2117,13 @@ config CMD_TFTPBOOT
>>>  	help
>>>  	  tftpboot - load file via network using TFTP protocol
>>>  
>>> +config CMD_WGET
>>> +	bool "wget"
>>> +	select PROT_TCP_LWIP
>>> +	help
>>> +	  wget is a simple command to download kernel, or other files,
>>> +	  from a http server over TCP.
>>> +
>>>  endif
>>>  
>>>  endif
>>> diff --git a/cmd/net-lwip.c b/cmd/net-lwip.c
>>> index c021da6a674..42f8bd6b259 100644
>>> --- a/cmd/net-lwip.c
>>> +++ b/cmd/net-lwip.c
>>> @@ -35,3 +35,11 @@ U_BOOT_CMD(
>>>  	"hostname [envvar]"
>>>  );
>>>  #endif
>>> +
>>> +#if defined(CONFIG_CMD_WGET)
>>> +U_BOOT_CMD(
>>> +	wget,   3,      1,      do_wget,
>>> +	"boot image via network using HTTP protocol",
>>> +	"[loadAddress] URL"
>>> +);
>>> +#endif
>>> diff --git a/include/net-lwip.h b/include/net-lwip.h
>>> index 4d41b0208a3..ddf25e61417 100644
>>> --- a/include/net-lwip.h
>>> +++ b/include/net-lwip.h
>>> @@ -11,8 +11,26 @@ struct netif *net_lwip_new_netif_noip(void);
>>>  void net_lwip_remove_netif(struct netif *netif);
>>>  struct netif *net_lwip_get_netif(void);
>>>  
>>> +/**
>>> + * wget_with_dns() - runs dns host IP address resulution before wget
>>> + *
>>> + * @dst_addr:	destination address to download the file
>>> + * @uri:	uri string of target file of wget
>>> + * Return:	downloaded file size, negative if failed
>>> + */
>>> +
>>> +int wget_with_dns(ulong dst_addr, char *uri);
>>> +/**
>>> + * wget_validate_uri() - varidate the uri
>>> + *
>>> + * @uri:	uri string of target file of wget
>>> + * Return:	true if uri is valid, false if uri is invalid
>>> + */
>>> +bool wget_validate_uri(char *uri);
>>> +
>>>  int do_dhcp(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]);
>>>  int do_dns(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]);
>>>  int do_ping(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]);
>>> +int do_wget(struct cmd_tbl *cmdtp, int flag, int argc, char * const argv[]);
>>>  
>>>  #endif /* __NET_LWIP_H__ */
>>> diff --git a/net-lwip/Makefile b/net-lwip/Makefile
>>> index aa247859483..bc011bb0e32 100644
>>> --- a/net-lwip/Makefile
>>> +++ b/net-lwip/Makefile
>>> @@ -10,6 +10,7 @@ obj-$(CONFIG_CMD_DHCP) += dhcp.o
>>>  obj-$(CONFIG_CMD_DNS) += dns.o
>>>  obj-$(CONFIG_CMD_PING) += ping.o
>>>  obj-$(CONFIG_CMD_TFTPBOOT) += tftp.o
>>> +obj-$(CONFIG_CMD_WGET) += wget.o
>>>  
>>>  # Disable this warning as it is triggered by:
>>>  # sprintf(buf, index ? "foo%d" : "foo", index)
>>> diff --git a/net-lwip/wget.c b/net-lwip/wget.c
>>> new file mode 100644
>>> index 00000000000..069299bd469
>>> --- /dev/null
>>> +++ b/net-lwip/wget.c
>>> @@ -0,0 +1,269 @@
>>> +// SPDX-License-Identifier: GPL-2.0+
>>> +/* Copyright (C) 2024 Linaro Ltd. */
>>> +
>>> +#include <command.h>
>>> +#include <console.h>
>>> +#include <display_options.h>
>>> +#include <image.h>
>>> +#include <lwip/apps/http_client.h>
>>> +#include <lwip/timeouts.h>
>>> +#include <net.h>
>>> +#include <time.h>
>>> +
>>> +#define SERVER_NAME_SIZE 200
>>> +#define HTTP_PORT_DEFAULT 80
>>> +#define PROGRESS_PRINT_STEP_BYTES (100 * 1024)
>>> +
>>> +enum done_state {
>>> +        NOT_DONE = 0,
>>> +        SUCCESS = 1,
>>> +        FAILURE = 2
>>> +};
>>> +
>>> +struct wget_ctx {
>>> +	ulong daddr;
>>> +	ulong saved_daddr;
>>> +	ulong size;
>>> +	ulong prevsize;
>>> +	ulong start_time;
>>> +	enum done_state done;
>>> +};
>>> +
>>> +static int parse_url(char *url, char *host, u16 *port, char **path)
>>> +{
>>> +	char *p, *pp;
>>> +	long lport;
>>> +
>>> +	p = strstr(url, "http://");
>>> +	if (!p)
>>> +		return -EINVAL;
>>> +
>>> +	p += strlen("http://");
>>> +
>>> +	/* Parse hostname */
>>> +	pp = strchr(p, ':');
>>> +	if (!pp)
>>> +		pp = strchr(p, '/');
>>> +	if (!pp)
>>> +		return -EINVAL;
>>> +
>>> +	if (p + SERVER_NAME_SIZE <= pp)
>>> +		return -EINVAL;
>>> +
>>> +	memcpy(host, p, pp - p);
>>> +	host[pp - p + 1] = '\0';
>>> +
>>> +	if (*pp == ':') {
>>> +		/* Parse port number */
>>> +		p = pp + 1;
>>> +		lport = simple_strtol(p, &pp, 10);
>>> +		if (pp && *pp != '/')
>>> +			return -EINVAL;
>>> +		if (lport > 65535)
>>> +			return -EINVAL;
>>> +		*port = (u16)lport;
>>> +	} else {
>>> +		*port = HTTP_PORT_DEFAULT;
>>> +	}
>>> +	if (*pp != '/')
>>> +		return -EINVAL;
>>> +	*path = pp;
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static err_t httpc_recv_cb(void *arg, struct altcp_pcb *pcb, struct pbuf *pbuf,
>>> +			   err_t err)
>>> +{
>>> +	struct wget_ctx *ctx = arg;
>>> +	struct pbuf *buf;
>>> +
>>> +	if (!pbuf)
>>> +		return ERR_BUF;
>>> +
>>> +	for (buf = pbuf; buf; buf = buf->next) {
>>> +		memcpy((void *)ctx->daddr, buf->payload, buf->len);
>>> +		ctx->daddr += buf->len;
>>> +		ctx->size += buf->len;
>>> +		if (ctx->size - ctx->prevsize > PROGRESS_PRINT_STEP_BYTES) {
>>> +			printf("#");
>>> +			ctx->prevsize = ctx->size;
>>> +		}
>>> +	}
>>> +
>>> +	altcp_recved(pcb, pbuf->tot_len);
>>> +	pbuf_free(pbuf);
>>> +	return ERR_OK;
>>> +}
>>> +
>>> +static void httpc_result_cb(void *arg, httpc_result_t httpc_result,
>>> +			    u32_t rx_content_len, u32_t srv_res, err_t err)
>>> +{
>>> +	struct wget_ctx *ctx = arg;
>>> +	ulong elapsed;
>>> +
>>> +	if (httpc_result != HTTPC_RESULT_OK) {
>>> +		log_err("\nHTTP client error %d\n", httpc_result);
>>> +		ctx->done = FAILURE;
>>> +		return;
>>> +	}
>>> +
>>> +	elapsed = get_timer(ctx->start_time);
>>> +	if (rx_content_len > PROGRESS_PRINT_STEP_BYTES)
>>> +		printf("\n");
>>> +        printf("%u bytes transferred in %lu ms (", rx_content_len,
>>> +	       get_timer(ctx->start_time));
>>> +        print_size(rx_content_len / elapsed * 1000, "/s)\n");
>>> +
>>> +	if (env_set_hex("filesize", rx_content_len) ||
>>> +	    env_set_hex("fileaddr", ctx->saved_daddr)) {
>>> +		log_err("Could not set filesize or fileaddr\n");
>>> +		ctx->done = FAILURE;
>>> +		return;
>>> +	}
>>> +
>>> +	ctx->done = SUCCESS;
>>> +}
>>> +
>>> +int wget_with_dns(ulong dst_addr, char *uri)
>>> +{
>>> +	char server_name[SERVER_NAME_SIZE];
>>> +	httpc_connection_t conn;
>>> +	httpc_state_t *state;
>>> +	struct netif *netif;
>>> +	struct wget_ctx ctx;
>>> +	char *path;
>>> +	u16 port;
>>> +
>>> +	ctx.daddr = dst_addr;
>>> +	ctx.saved_daddr = dst_addr;
>>> +	ctx.done = NOT_DONE;
>>> +	ctx.size = 0;
>>> +	ctx.prevsize = 0;
>>> +
>>> +	if (parse_url(uri, server_name, &port, &path))
>>> +		return CMD_RET_USAGE;
>>> +
>>> +	netif = net_lwip_new_netif();
>>> +	if (!netif)
>>> +		return -1;
>>> +
>>> +	memset(&conn, 0, sizeof(conn));
>>> +	conn.result_fn = httpc_result_cb;
>>> +	ctx.start_time = get_timer(0);
>>> +	if (httpc_get_file_dns(server_name, port, path, &conn, httpc_recv_cb,
>>> +			       &ctx, &state)) {
>>> +		net_lwip_remove_netif(netif);
>>> +		return CMD_RET_FAILURE;
>>> +	}
>>> +
>>> +	while (!ctx.done) {
>>> +		eth_rx();
>>> +		sys_check_timeouts();
>>> +		if (ctrlc())
>>> +			break;
>>> +	}
>>> +
>>> +	net_lwip_remove_netif(netif);
>>> +
>>> +	if (ctx.done == SUCCESS)
>>> +		return 0;
>>> +
>>> +	return -1;
>>> +}
>>> +
>>> +int do_wget(struct cmd_tbl *cmdtp, int flag, int argc, char * const argv[])
>>> +{
>>> +	char *end;
>>> +	char *url;
>>> +	ulong dst_addr;
>>> +
>>> +	if (argc < 2 || argc > 3)
>>> +		return CMD_RET_USAGE;
>>> +
>>> +	dst_addr = hextoul(argv[1], &end);
>>> +        if (end == (argv[1] + strlen(argv[1]))) {
>>> +		if (argc < 3)
>>> +			return CMD_RET_USAGE;
>>> +		url = argv[2];
>>> +	} else {
>>> +		dst_addr = image_load_addr;
>>> +		url = argv[1];
>>> +	}
>>> +
>>> +	if (wget_with_dns(dst_addr, url))
>>> +		return CMD_RET_FAILURE;
>>> +
>>> +	return CMD_RET_SUCCESS;
>>> +}
>>> +
>>> +/**
>>> + * wget_validate_uri() - validate the uri for wget
>>> + *
>>> + * @uri:	uri string
>>> + *
>>> + * This function follows the current U-Boot wget implementation.
>>> + * scheme: only "http:" is supported
>>> + * authority:
>>> + *   - user information: not supported
>>> + *   - host: supported
>>> + *   - port: not supported(always use the default port)
>>> + *
>>> + * Uri is expected to be correctly percent encoded.
>>> + * This is the minimum check, control codes(0x1-0x19, 0x7F, except '\0')
>>> + * and space character(0x20) are not allowed.
>>> + *
>>> + * TODO: stricter uri conformance check
>>> + *
>>> + * Return:	true on success, false on failure
>>> + */
>>> +bool wget_validate_uri(char *uri)
>>> +{
>>> +	char c;
>>> +	bool ret = true;
>>> +	char *str_copy, *s, *authority;
>>> +
>>> +	for (c = 0x1; c < 0x21; c++) {
>>> +		if (strchr(uri, c)) {
>>> +			log_err("invalid character is used\n");
>>> +			return false;
>>> +		}
>>> +	}
>>> +	if (strchr(uri, 0x7f)) {
>>> +		log_err("invalid character is used\n");
>>> +		return false;
>>> +	}
>>> +
>>> +	if (strncmp(uri, "http://", 7)) {
>>> +		log_err("only http:// is supported\n");
>>> +		return false;
>>> +	}
>>> +	str_copy = strdup(uri);
>>> +	if (!str_copy)
>>> +		return false;
>>> +
>>> +	s = str_copy + strlen("http://");
>>> +	authority = strsep(&s, "/");
>>> +	if (!s) {
>>> +		log_err("invalid uri, no file path\n");
>>> +		ret = false;
>>> +		goto out;
>>> +	}
>>> +	s = strchr(authority, '@');
>>> +	if (s) {
>>> +		log_err("user information is not supported\n");
>>> +		ret = false;
>>> +		goto out;
>>> +	}
>>> +	s = strchr(authority, ':');
>>> +	if (s) {
>>> +		log_err("user defined port is not supported\n");
>>> +		ret = false;
>>> +		goto out;
>>> +	}
>>> +
>>> +out:
>>> +	free(str_copy);
>>> +
>>> +	return ret;
>>> +}
>>> -- 
>>> 2.40.1
>
> Hi Jon,
>
>> Hi Jerome, I am trying out the lwIP stack on TI boards.
>> 
>> For wget, I noticed a bug and a user experience improvement. The bug is an
>> off-by-one array access when parsing the url.
>
> Indeed, someone reported this bug to me off-list and it's already fixed in
> my WIP branch.
>
>> The improvement emits a
>> message if the url isn't an http://, since that is all we are supporting,
>> and without the message, the wget command silently fails, which is
>> confusing to the user.
>
> I agree.
>
>> I will post the 2 patches as a reply to this email. LMK if you want to use
>> another method to collaborate. I can already see that efi http boot support
>> will not work. wget_validate_uri() looks to be copied vertabim from the
>> current implementation, and IMO we should completely remove it.
>
> It is indeed copied. It is needed to avoid an unresolved symbol but I
> understand more work may be needed for EFI HTTP boot support (not tested by
> me).
>
> If you are willing to help with this then you are welcome to post patches as
> replies and I will integrate them in the series.
>
> Thanks,
> -- 
> Jerome

Jerome, here is a patch to allow a port specifier in wget's uri, which lwIP
supports.

thanks
Jon

Subject: [PATCH] net-lwip: lwIP wget supports user defined port in the uri, so
 allow it.

Signed-off-by: Jonathan Humphreys <j-humphreys@ti.com>
---
 net-lwip/wget.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/net-lwip/wget.c b/net-lwip/wget.c
index 6b9c953ef51..d3fdd44a29e 100644
--- a/net-lwip/wget.c
+++ b/net-lwip/wget.c
@@ -257,12 +257,6 @@ bool wget_validate_uri(char *uri)
 		ret = false;
 		goto out;
 	}
-	s = strchr(authority, ':');
-	if (s) {
-		log_err("user defined port is not supported\n");
-		ret = false;
-		goto out;
-	}
 
 out:
 	free(str_copy);

On 6/25/24 05:20, Jon Humphreys wrote:
> Jerome Forissier <jerome.forissier@linaro.org> writes:
> 
>> On 6/24/24 08:21, Jon Humphreys wrote:
>>> Jerome Forissier <jerome.forissier@linaro.org> writes:
>>>
>>>> Add support for the wget command with NET_LWIP.
>>>>
>>>> Based on code initially developed by Maxim U.
>>>>
>>>> Signed-off-by: Jerome Forissier <jerome.forissier@linaro.org>
>>>> Co-developed-by: Maxim Uvarov <muvarov@gmail.com>
>>>> Cc: Maxim Uvarov <muvarov@gmail.com>
>>>> Signed-off-by: Jerome Forissier <jerome.forissier@linaro.org>
>>>> ---
>>>>  cmd/Kconfig        |   7 ++
>>>>  cmd/net-lwip.c     |   8 ++
>>>>  include/net-lwip.h |  18 +++
>>>>  net-lwip/Makefile  |   1 +
>>>>  net-lwip/wget.c    | 269 +++++++++++++++++++++++++++++++++++++++++++++
>>>>  5 files changed, 303 insertions(+)
>>>>  create mode 100644 net-lwip/wget.c
>>>>

<snip>

>>>> +/**
>>>> + * wget_validate_uri() - validate the uri for wget
>>>> + *
>>>> + * @uri:	uri string
>>>> + *
>>>> + * This function follows the current U-Boot wget implementation.
>>>> + * scheme: only "http:" is supported
>>>> + * authority:
>>>> + *   - user information: not supported
>>>> + *   - host: supported
>>>> + *   - port: not supported(always use the default port)
>>>> + *
>>>> + * Uri is expected to be correctly percent encoded.
>>>> + * This is the minimum check, control codes(0x1-0x19, 0x7F, except '\0')
>>>> + * and space character(0x20) are not allowed.
>>>> + *
>>>> + * TODO: stricter uri conformance check
>>>> + *
>>>> + * Return:	true on success, false on failure
>>>> + */
>>>> +bool wget_validate_uri(char *uri)
>>>> +{
>>>> +	char c;
>>>> +	bool ret = true;
>>>> +	char *str_copy, *s, *authority;
>>>> +
>>>> +	for (c = 0x1; c < 0x21; c++) {
>>>> +		if (strchr(uri, c)) {
>>>> +			log_err("invalid character is used\n");
>>>> +			return false;
>>>> +		}
>>>> +	}
>>>> +	if (strchr(uri, 0x7f)) {
>>>> +		log_err("invalid character is used\n");
>>>> +		return false;
>>>> +	}
>>>> +
>>>> +	if (strncmp(uri, "http://", 7)) {
>>>> +		log_err("only http:// is supported\n");
>>>> +		return false;
>>>> +	}
>>>> +	str_copy = strdup(uri);
>>>> +	if (!str_copy)
>>>> +		return false;
>>>> +
>>>> +	s = str_copy + strlen("http://");
>>>> +	authority = strsep(&s, "/");
>>>> +	if (!s) {
>>>> +		log_err("invalid uri, no file path\n");
>>>> +		ret = false;
>>>> +		goto out;
>>>> +	}
>>>> +	s = strchr(authority, '@');
>>>> +	if (s) {
>>>> +		log_err("user information is not supported\n");
>>>> +		ret = false;
>>>> +		goto out;
>>>> +	}
>>>> +	s = strchr(authority, ':');
>>>> +	if (s) {
>>>> +		log_err("user defined port is not supported\n");
>>>> +		ret = false;
>>>> +		goto out;
>>>> +	}
>>>> +
>>>> +out:
>>>> +	free(str_copy);
>>>> +
>>>> +	return ret;
>>>> +}
>>>> -- 
>>>> 2.40.1
>>
>> Hi Jon,
>>
>>> Hi Jerome, I am trying out the lwIP stack on TI boards.
>>>
>>> For wget, I noticed a bug and a user experience improvement. The bug is an
>>> off-by-one array access when parsing the url.
>>
>> Indeed, someone reported this bug to me off-list and it's already fixed in
>> my WIP branch.
>>
>>> The improvement emits a
>>> message if the url isn't an http://, since that is all we are supporting,
>>> and without the message, the wget command silently fails, which is
>>> confusing to the user.
>>
>> I agree.
>>
>>> I will post the 2 patches as a reply to this email. LMK if you want to use
>>> another method to collaborate. I can already see that efi http boot support
>>> will not work. wget_validate_uri() looks to be copied vertabim from the
>>> current implementation, and IMO we should completely remove it.
>>
>> It is indeed copied. It is needed to avoid an unresolved symbol but I
>> understand more work may be needed for EFI HTTP boot support (not tested by
>> me).
>>
>> If you are willing to help with this then you are welcome to post patches as
>> replies and I will integrate them in the series.
>>
>> Thanks,
>> -- 
>> Jerome
> 
> Jerome, here is a patch to allow a port specifier in wget's uri, which lwIP
> supports.
> 
> thanks
> Jon
> 
> Subject: [PATCH] net-lwip: lwIP wget supports user defined port in the uri, so
>  allow it.
> 
> Signed-off-by: Jonathan Humphreys <j-humphreys@ti.com>
> ---
>  net-lwip/wget.c | 6 ------
>  1 file changed, 6 deletions(-)
> 
> diff --git a/net-lwip/wget.c b/net-lwip/wget.c
> index 6b9c953ef51..d3fdd44a29e 100644
> --- a/net-lwip/wget.c
> +++ b/net-lwip/wget.c
> @@ -257,12 +257,6 @@ bool wget_validate_uri(char *uri)
>  		ret = false;
>  		goto out;
>  	}
> -	s = strchr(authority, ':');
> -	if (s) {
> -		log_err("user defined port is not supported\n");
> -		ret = false;
> -		goto out;
> -	}
>  
>  out:
>  	free(str_copy);

Patch added to v5, thanks!