Re: [lwip-devel] [PATCHv6 06/14] net/lwip: implement wget cmd

[Ilias Apalodimas]

On Mon, Aug 14, 2023 at 07:32:45PM +0600, Maxim Uvarov wrote:
> Implement function for wget command with lwIP variant. Usage and output is
> the same as the original command. This code called by compatibility code
>  between U-Boot and lwIP.
> 


Similar comments to previous patches adding commands

> Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org>
> +*
> +*
> +* @addr  start address to download result
> +* @url   url in format http://host[:port]/url
> +* Return: 0 for success
> +*         !0 if error
> +*/

Same here, sphinx needs a different syntax

> +int ulwip_wget(ulong addr, char *url);
> diff --git a/net/lwip/Makefile b/net/lwip/Makefile
> index 0337d82cf5..4c6df94807 100644
> --- a/net/lwip/Makefile
> +++ b/net/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/net/lwip/apps/http/Makefile b/net/lwip/apps/http/Makefile
> new file mode 100644
> index 0000000000..3e92b0ef1b
> --- /dev/null
> +++ b/net/lwip/apps/http/Makefile
> @@ -0,0 +1,13 @@
> +ccflags-y += -I$(srctree)/net/lwip/port/include
> +ccflags-y += -I$(srctree)/net/lwip/lwip-external/src/include 
> -I$(srctree)/net/lwip
> +ccflags-y += -I$(obj)
> +
> +$(obj)/http_clinet.o: $(obj)/http_client.c
> +.PHONY: $(obj)/http_client.c
> +$(obj)/http_client.c:
> +     cp $(srctree)/net/lwip/lwip-external/src/apps/http/http_client.c 
> $(obj)/http_client.c
> +     cp 
> $(srctree)/net/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/net/lwip/apps/http/lwip-wget.c b/net/lwip/apps/http/lwip-wget.c
> new file mode 100644
> index 0000000000..73d82225a2
> --- /dev/null
> +++ b/net/lwip/apps/http/lwip-wget.c
> @@ -0,0 +1,131 @@
> +// 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 <vsprintf.h>
> +
> +#include "http_client.h"
> +#include <net/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 unused_err)
> +{
> +     struct pbuf *q;

Please change the p/q stuff to something that's actually readable and hints
on what those variables represent 

> +
> +     if (!p)
> +             return ERR_BUF;
> +
> +     for (q = p; q != NULL; q = q->next) {
> +             memcpy((void *)daddr, q->payload, q->len);
> +             log_debug("downloaded chunk size %d, to addr 0x%lx\n", q->len, 
> daddr);
> +             daddr += q->len;
> +     }
> +     altcp_recved(pcb, p->tot_len);
> +     pbuf_free(p);

Why is this freed here?
If the caller allocates this, he should also be responsible for freeing it 

> +     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) {
> +             log_info("\n%d bytes successfully downloaded.\n", 
> rx_content_len);
> +             env_set_ulong("filesize", rx_content_len);
> +             ulwip_exit(0);
> +     } else {
> +             log_err("\nhttp eroror: %d\n", httpc_result);
> +             ulwip_exit(-1);
> +     }
> +}
> +
> +/* http://hostname:port/url */
> +static int parse_url(char *url, char *host, u16 *port)

I think we discussed this already.  If the existing wget application
doesn't support configurable ports, just skip it on the initial patch and 
add it in the future 

> +{
> +     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
> +             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 = (u16)dectoul(portstr, NULL);
> +     } 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 ulwip_wget(ulong addr, char *url)
> +{
> +     err_t err;
> +     u16 port;
> +     char server_name[SERVER_NAME_SIZE];
> +     httpc_state_t *connection;
> +
> +     daddr = addr;
> +
> +     err = parse_url(url, server_name, &port);
> +     if (err) {
> +             log_err("error parse_url\n");
> +             return -1;
> +     }
> +
> +     log_info("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) {
> +             log_err("httpc_init_connection failed\n");
> +             return err;
> +     }
> +
> +     env_set_hex("fileaddr", addr);
> +     return 0;
> +}
> -- 
> 2.30.2
> 

Thanks
/Ilias