Re: [PATCHv5 06/13] net/lwip: implement wget cmd

Maxim Uvarov
2023-08-03T14:59:42Z

On Thu, 3 Aug 2023 at 12:48, Ilias Apalodimas <ilias.apalodimas@linaro.org>
wrote:

> On Wed, Aug 02, 2023 at 08:06:51PM +0600, Maxim Uvarov wrote:
> > Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org>
> > ---
> > lib/lwip/Makefile | 1 +
> > lib/lwip/apps/http/Makefile | 13 ++++
> > lib/lwip/apps/http/lwip-wget.c | 130 +++++++++++++++++++++++++++++++++
> > 3 files changed, 144 insertions(+)
> > create mode 100644 lib/lwip/apps/http/Makefile
> > create mode 100644 lib/lwip/apps/http/lwip-wget.c
> >
> > diff --git a/lib/lwip/Makefile b/lib/lwip/Makefile
> > index 1893162c1a..ec6b728c8e 100644
> > --- a/lib/lwip/Makefile
> > +++ b/lib/lwip/Makefile
> > @@ -68,3 +68,4 @@ obj-$(CONFIG_NET) += port/sys-arch.o
> > obj-$(CONFIG_CMD_DHCP) += apps/dhcp/lwip-dhcp.o
> > obj-$(CONFIG_CMD_DNS) += apps/dns/lwip-dns.o
> > obj-$(CONFIG_CMD_TFTPBOOT) += apps/tftp/
> > +obj-$(CONFIG_CMD_WGET) += apps/http/
> > diff --git a/lib/lwip/apps/http/Makefile b/lib/lwip/apps/http/Makefile
> > new file mode 100644
> > index 0000000000..7d22817e50
> > --- /dev/null
> > +++ b/lib/lwip/apps/http/Makefile
> > @@ -0,0 +1,13 @@
> > +ccflags-y += -I$(srctree)/lib/lwip/port/include
> > +ccflags-y += -I$(srctree)/lib/lwip/lwip-external/src/include
> -I$(srctree)/lib/lwip
> > +ccflags-y += -I$(obj)
> > +
> > +$(obj)/http_clinet.o: $(obj)/http_client.c
> > +.PHONY: $(obj)/http_client.c
> > +$(obj)/http_client.c:
> > + cp $(srctree)/lib/lwip/lwip-external/src/apps/http/http_client.c
> $(obj)/http_client.c
> > + cp
> $(srctree)/lib/lwip/lwip-external/src/include/lwip/apps/http_client.h
> $(obj)/http_client.h
> > +
> > +obj-$(CONFIG_CMD_WGET) += http_client.o
> > +obj-$(CONFIG_CMD_WGET) += lwip-wget.o
> > +
> > diff --git a/lib/lwip/apps/http/lwip-wget.c
> b/lib/lwip/apps/http/lwip-wget.c
> > new file mode 100644
> > index 0000000000..47a77250c5
> > --- /dev/null
> > +++ b/lib/lwip/apps/http/lwip-wget.c
> > @@ -0,0 +1,130 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +
> > +/*
> > + * (C) Copyright 2023 Linaro Ltd. <maxim.uvarov@linaro.org>
> > + */
> > +
> > +#include <common.h>
> > +#include <command.h>
> > +#include <console.h>
> > +
> > +#include "http_client.h"
> > +#include <ulwip.h>
> > +
> > +static ulong daddr;
> > +static httpc_connection_t settings;
> > +
> > +#define SERVER_NAME_SIZE 200
> > +
> > +static err_t httpc_recv(void *arg, struct altcp_pcb *pcb, struct pbuf
> *p, err_t err)
> > +{
> > + struct pbuf *q;
> > + LWIP_UNUSED_ARG(err);
>
> I think it's better to use standard C instead of LWIP defined macros for
> things like that
>

agree no need to use lwip macro here. But what is the common practice for
U-Boot for unused function args?
I think compilation does not even warn about it. So naming like unused_err
has to be fine here.

>
> > +
> > + if (!p)
> > + return ERR_BUF;
> > +
> > + for (q = p; q != NULL; q = q->next) {
> > + memcpy((void *)daddr, q->payload, q->len);
> > + printf("downloaded chunk size %d, to addr 0x%lx\n",
> q->len, daddr);
> > + daddr += q->len;
> > + }
> > + altcp_recved(pcb, p->tot_len);
> > + pbuf_free(p);
> > + return ERR_OK;
> > +}
> > +
> > +static void httpc_result(void *arg, httpc_result_t httpc_result, u32_t
> rx_content_len,
> > + u32_t srv_res, err_t err)
> > +{
> > + if (httpc_result == HTTPC_RESULT_OK) {
> > + printf("\n%d bytes successfully downloaded.\n",
> rx_content_len);
>
> Switch those to a log_XXXX that makes sense
>
>
yep.

> > + env_set_ulong("filesize", rx_content_len);
> > + ulwip_exit(0);
> > + } else {
> > + printf("\nhttp eroror: %d\n", httpc_result);
> > + ulwip_exit(-1);
> > + }
> > +}
> > +
> > +/* http://hostname:port/url */
> > +static int parse_url(char *url, char *host, int *port)
> > +{
> > + char *p, *pp;
> > +
> > + p = strstr(url, "http://");
> > + if (!p) {
> > + printf("err: no http://!\n");
> > + return -1;
> > + }
> > +
> > + p += strlen("http://");
> > +
> > + /* parse hostname */
> > + pp = strchr(p, ':');
> > + if (pp) {
> > +#define PORT_STR_SIZE 5
>
> https://lore.kernel.org/u-boot/ZMJqIxmXJD4S72Ig@hades/
> Do we currently support a configurable port for wget?
>
>
I might have to reply to the previous email, then update the patch. Old
wget has port inside header file and it's binded to 80.
Old wget does not parse url script and uses serverip variable for the
server. Old wget does not use DNS. Because of lwip variant
can use a url (including DNS and IPv6) there is no need to use the server
ip variable and bind to a specific port. So here we parse port,
host and url and pass it to lwip http_client.c example.

> > + char portstr[PORT_STR_SIZE];
> > +
> > + if (pp - p >= SERVER_NAME_SIZE)
> > + return -2;
> > + memcpy(host, p, pp - p);
> > + host[pp - p + 1] = '\0';
> > +
> > + p = pp + 1;
> > + pp = strchr(p, '/');
> > + if (!pp) {
> > + printf("wrong url\n");
> > + return -3;
> > + }
> > +
> > + if (pp - p >= PORT_STR_SIZE)
> > + return -4;
> > + memcpy(portstr, p, pp - p);
> > + portstr[pp - p] = '\0';
> > + *port = atoi(portstr);
> > + } else {
> > + pp = strchr(p, '/');
> > + if (!pp) {
> > + printf("wrong url\n");
> > + return -5;
> > + }
> > +
> > + if (pp - p >= SERVER_NAME_SIZE)
> > + return -6;
> > + memcpy(host, p, pp - p);
> > + host[pp - p + 1] = '\0';
> > + *port = 80; /* default */
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +int lwip_wget(ulong addr, char *url)
> > +{
> > + err_t err;
> > + int port;
> > + char server_name[SERVER_NAME_SIZE];
> > + httpc_state_t *connection;
> > +
> > + daddr = addr;
> > +
> > + err = parse_url(url, server_name, &port);
> > + if (err) {
> > + printf("error parse_url\n");
> > + return -1;
> > + }
> > +
> > + printf("downloading %s to addr 0x%lx\n", url, addr);
> > + memset(&settings, 0, sizeof(settings));
> > + settings.result_fn = httpc_result;
> > + err = httpc_get_file_dns(server_name, port, url, &settings,
> > + httpc_recv, NULL, &connection);
> > + if (err != ERR_OK) {
> > + printf("httpc_init_connection failed\n");
> > + return err;
> > + }
> > +
> > + env_set_hex("fileaddr", addr);
> > + return 0;
> > +}
> > --
> > 2.30.2
> >
>