Jerome Forissier <jerome.foriss...@linaro.org> writes:
> On 6/24/24 08:21, Jon Humphreys wrote:
>> Jerome Forissier <jerome.foriss...@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.foriss...@linaro.org>
>>> Co-developed-by: Maxim Uvarov <muva...@gmail.com>
>>> Cc: Maxim Uvarov <muva...@gmail.com>
>>> Signed-off-by: Jerome Forissier <jerome.foriss...@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-humphr...@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);
--
2.34.1
