Re: [PATCH v14 2/2] net: Add wget application

On 4/21/22 18:54, Ying-Chun Liu wrote:

From: "Ying-Chun Liu (PaulLiu)" paul.liu@linaro.org

This commit adds a simple wget command that can download files from http server.

I think description can be much bigger. I was running just wget and it did something.

Signed-off-by: Duncan Hare DuncanCHare@yahoo.com Signed-off-by: Ying-Chun Liu (PaulLiu) paul.liu@linaro.org Cc: Christian Gmeiner christian.gmeiner@gmail.com Cc: Joe Hershberger joe.hershberger@ni.com Cc: Ramon Fried rfried.dev@gmail.com

---

v13: Fix some issues which is reviewed by Christian

cmd/Kconfig | 7 + cmd/net.c | 13 ++ include/net.h | 2 +- include/net/wget.h | 19 ++ net/Makefile | 1 + net/net.c | 6 + net/wget.c | 436 +++++++++++++++++++++++++++++++++++++++++++++ 7 files changed, 483 insertions(+), 1 deletion(-) create mode 100644 include/net/wget.h create mode 100644 net/wget.c

diff --git a/cmd/Kconfig b/cmd/Kconfig index d3abe3a06b..6eff068feb 100644 --- a/cmd/Kconfig +++ b/cmd/Kconfig @@ -1667,6 +1667,13 @@ config NFS_TIMEOUT "ERROR: Cannot umount" in nfs command, try longer timeout such as 10000.

+config CMD_WGET

• bool "wget"
• select TCP
• help

•  Download kernel, or other files, from a web server over TCP.
  

•  Fast file transfer over networks with latenc
  

latenc?

• config CMD_MII bool "mii" imply CMD_MDIO

diff --git a/cmd/net.c b/cmd/net.c index 3619c843d8..60fd785061 100644 --- a/cmd/net.c +++ b/cmd/net.c @@ -124,6 +124,19 @@ U_BOOT_CMD( ); #endif

+#if defined(CONFIG_CMD_WGET) +static int do_wget(struct cmd_tbl *cmdtp, int flag, int argc, char * const argv[]) +{

• return netboot_common(WGET, cmdtp, argc, argv);

+}

+U_BOOT_CMD(

• wget, 3, 1, do_wget,
• "boot image via network using HTTP protocol",
• "[loadAddress] [[hostIPaddr:]path and image name]"

+); +#endif

• static void netboot_update_env(void) { char tmp[22];

diff --git a/include/net.h b/include/net.h index 220eba2e21..3553f17a8e 100644 --- a/include/net.h +++ b/include/net.h @@ -557,7 +557,7 @@ extern int net_restart_wrap; /* Tried all network devices */

enum proto_t { BOOTP, RARP, ARP, TFTPGET, DHCP, PING, DNS, NFS, CDP, NETCONS, SNTP,

• TFTPSRV, TFTPPUT, LINKLOCAL, FASTBOOT, WOL, UDP

• TFTPSRV, TFTPPUT, LINKLOCAL, FASTBOOT, WOL, UDP, WGET };

  extern char net_boot_file_name[1024];/* Boot File name */

diff --git a/include/net/wget.h b/include/net/wget.h new file mode 100644 index 0000000000..61bdd851f9 --- /dev/null +++ b/include/net/wget.h @@ -0,0 +1,19 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +/*

• • Duncan Hare Copyright 2017
• */

+void wget_start(void); /* Begin wget */

comment on previous line.

+enum WGET_STATE {

• WGET_CLOSED,
• WGET_CONNECTING,
• WGET_CONNECTED,
• WGET_TRANSFERRING,
• WGET_TRANSFERRED

+};

Please initialize them.

+#define DEBUG_WGET 0 /* Set to 1 for debug messges */

comment has typo there.

+#define SERVER_PORT 80 +#define WGET_RETRY_COUNT 30 +#define WGET_TIMEOUT 2000UL

fix that indenatation.

diff --git a/net/Makefile b/net/Makefile index f68ad94767..54ef337e80 100644 --- a/net/Makefile +++ b/net/Makefile @@ -30,6 +30,7 @@ obj-$(CONFIG_UDP_FUNCTION_FASTBOOT) += fastboot.o obj-$(CONFIG_CMD_WOL) += wol.o obj-$(CONFIG_PROT_UDP) += udp.o obj-$(CONFIG_PROT_TCP) += tcp.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/net.c b/net/net.c index 28d9fbd227..5044c42add 100644 --- a/net/net.c +++ b/net/net.c @@ -117,6 +117,7 @@ #include "wol.h" #endif #include <net/tcp.h> +#include <net/wget.h>

/** BOOTP EXTENTIONS **/

@@ -505,6 +506,11 @@ restart: nfs_start(); break; #endif +#if defined(CONFIG_CMD_WGET)

• case WGET:
  

• 	wget_start();
  

• 	break;
  

+#endif #if defined(CONFIG_CMD_CDP) case CDP: cdp_start(); diff --git a/net/wget.c b/net/wget.c new file mode 100644 index 0000000000..e3d1c8cc28 --- /dev/null +++ b/net/wget.c @@ -0,0 +1,436 @@ +// SPDX-License-Identifier: GPL-2.0 +/*

• • WGET/HTTP support driver based on U-BOOT's nfs.c
• • Copyright Duncan Hare dh@synoia.com 2017
• */

+#include <common.h> +#include <command.h> +#include <mapmem.h> +#include <env.h>

please sort it.

+#include <image.h> +#include <net.h> +#include <net/wget.h> +#include <net/tcp.h>

+static const char bootfile1[] = "GET "; +static const char bootfile3[] = " HTTP/1.0\r\n\r\n"; +static const char http_eom[] = "\r\n\r\n"; +static const char http_ok[] = "200"; +static const char content_len[] = "Content-Length"; +static const char linefeed[] = "\r\n";

fix indentation

+static struct in_addr web_server_ip; +static int our_port; +static int wget_timeout_count;

+struct pkt_qd {

• uchar *pkt;
• unsigned int tcp_seq_num;
• unsigned int len;

+};

+/*

• • This is a control structure for out of order packets received.
• • The actual packet bufers are in the kernel space, and are
• • expected to be overwritten by the downloaded image.
• */

+static struct pkt_qd pkt_q[PKTBUFSRX / 4]; +static int pkt_q_idx; +static unsigned long content_length; +static unsigned int packets;

+static unsigned int initial_data_seq_num;

+static enum WGET_STATE wget_state;

+static char *image_url; +static unsigned int wget_timeout = WGET_TIMEOUT;

+static void wget_timeout_handler(void);

Can't you resort that functions to remove this line?

+static enum net_loop_state wget_loop_state;

+/* Timeout retry parameters */ +static u8 retry_action; +static unsigned int retry_tcp_ack_num; +static unsigned int retry_tcp_seq_num; +static int retry_len;

So many variables without any description. Please add it.

+static inline int store_block(uchar *src, unsigned int offset, unsigned int len)

kernel-doc for all these functions would help with reviewing.

+{

• ulong newsize = offset + len;
• uchar *ptr;

+#ifdef CONFIG_SYS_DIRECT_FLASH_WGET

add it to the code. checkpatch reports it too.

This macro should be in Kconfig.

And there is no single word anywhere that you can use wget and save data directly to SPI. That should be visible at lease in commit message.

• int i, rc = 0;
• for (i = 0; i < CONFIG_SYS_MAX_FLASH_BANKS; i++) {

• /* start address in flash? */
  

• if (image_load_addr + offset >= flash_info[i].start[0]) {
  

• 	rc = 1;
  

• 	break;
  

• }
  

• }
• if (rc) { /* Flash is destination for this packet */

• rc = flash_write((uchar *)src,
  

• 		 (ulong)(image_load_addr + offset), len);
  

• if (rc) {
  

• 	flash_perror(rc);
  

• 	return -1;
  

• }
  

• } else {

+#endif /* CONFIG_SYS_DIRECT_FLASH_WGET */

• ptr = map_sysmem(image_load_addr + offset, len);
  

• memcpy(ptr, src, len);
  

• unmap_sysmem(ptr);
  

+#ifdef CONFIG_SYS_DIRECT_FLASH_WGET

• }

+#endif

• if (net_boot_file_size < (offset + len))

• net_boot_file_size = newsize;
  

newline here.

• return 0;

+}

+/*

• • wget response dispatcher
• • WARNING, This, and only this, is the place in wget.c where
• • SEQUENCE NUMBERS are swapped between incoming (RX)
• • and outgoing (TX).
• • Procedure wget_handler() is correct for RX traffic.
• */

+static void wget_send_stored(void) +{

• u8 action = retry_action;
• unsigned int tcp_ack_num = retry_tcp_ack_num;
• unsigned int tcp_seq_num = retry_tcp_seq_num;
• int len = retry_len;
• uchar *ptr;
• uchar *offset;

put it on single line and fix that indentation.

• tcp_ack_num = tcp_ack_num + len;

isn't it easier to add it directly above?

If not tcp_ack_num += len; but I prefer to add it directly above.

• switch (wget_state) {
• case WGET_CLOSED:

• debug_cond(DEBUG_WGET, "wget: send SYN\n");
  

• wget_state = WGET_CONNECTING;
  

• net_send_tcp_packet(0, SERVER_PORT, our_port, action,
  

• 		    tcp_seq_num, tcp_ack_num);
  

• packets = 0;
  

• break;
  

• case WGET_CONNECTING:

• pkt_q_idx = 0;
  

• net_send_tcp_packet(0, SERVER_PORT, our_port, action,
  

• 		    tcp_seq_num, tcp_ack_num);
  

• ptr = net_tx_packet + net_eth_hdr_size()
  

• 	+ IP_TCP_HDR_SIZE + TCP_TSOPT_SIZE + 2;
  

indentation is not correct and + should be on previous line.

• offset = ptr;
  

• memcpy(offset, &bootfile1, strlen(bootfile1));
  

• offset = offset + strlen(bootfile1);
  

offset +=

• memcpy(offset, image_url, strlen(image_url));
  

• offset = offset + strlen(image_url);
  

ditto

• memcpy(offset, &bootfile3, strlen(bootfile3));
  

• offset = offset + strlen(bootfile3);
  

ditto

• net_send_tcp_packet((offset - ptr), SERVER_PORT, our_port,
  

• 		    TCP_PUSH, tcp_seq_num, tcp_ack_num);
  

• wget_state = WGET_CONNECTED;
  

• break;
  

• case WGET_CONNECTED:
• case WGET_TRANSFERRING:
• case WGET_TRANSFERRED:

• net_send_tcp_packet(0, SERVER_PORT, our_port, action,
  

• 		    tcp_seq_num, tcp_ack_num);
  

• break;
  

• }

+}

+static void wget_send(u8 action, unsigned int tcp_ack_num,

•       unsigned int tcp_seq_num, int len)
  

+{

• retry_action = action;
• retry_tcp_ack_num = tcp_ack_num;
• retry_tcp_seq_num = tcp_seq_num;
• retry_len = len;

newline here and if indenation above.

• wget_send_stored();

+}

+void wget_fail(char *error_message, unsigned int tcp_seq_num,

•       unsigned int tcp_ack_num, u8 action)
  

+{

• printf("%s", error_message);
• printf("%s", "wget: Transfer Fail\n");

Why to use this style? Just print it directly with single line.

• net_set_timeout_handler(0, NULL);
• wget_send(action, tcp_seq_num, tcp_ack_num, 0);

+}

+void wget_success(u8 action, unsigned int tcp_seq_num,

•   unsigned int tcp_ack_num, int len, int packets)
  

+{

• printf("Packets received %d, Transfer Successful\n", packets);
• wget_send(action, tcp_seq_num, tcp_ack_num, len);

+}

+/*

• • Interfaces of U-BOOT
• */

+static void wget_timeout_handler(void) +{

• if (++wget_timeout_count > WGET_RETRY_COUNT) {

• puts("\nRetry count exceeded; starting again\n");
  

• wget_send(TCP_RST, 0, 0, 0);
  

• net_start_again();
  

• } else {

• puts("T ");
  

• net_set_timeout_handler(wget_timeout +
  

• 			WGET_TIMEOUT * wget_timeout_count,
  

• 			wget_timeout_handler);
  

• wget_send_stored();
  

• }

+}

+static void wget_connected(uchar *pkt, unsigned int tcp_seq_num,

• 	   struct in_addr action_and_state,
  

• 	   unsigned int tcp_ack_num, unsigned int len)
  

+{

• u8 action = action_and_state.s_addr;
• uchar *pkt_in_q;
• char *pos;
• int hlen;
• int i;

pack it.

• pkt[len] = '\0';
• pos = strstr((char *)pkt, http_eom);
• if (pos == 0) {

if (!pos) {

• debug_cond(DEBUG_WGET,
  

• 	   "wget: Connected, data before Header %p\n", pkt);
  

• pkt_in_q = (void *)image_load_addr + 0x20000 + (pkt_q_idx * 0x800);
  

here are 2 magic values - create macros for them.

• memcpy(pkt_in_q, pkt, len);
  

• pkt_q[pkt_q_idx].pkt              = pkt_in_q;
  

• pkt_q[pkt_q_idx].tcp_seq_num      = tcp_seq_num;
  

• pkt_q[pkt_q_idx].len              = len;
  

• pkt_q_idx++;
  

• } else {

• debug_cond(DEBUG_WGET, "wget: Connected HTTP Header %p\n", pkt);
  

• hlen = pos - (char *)pkt + sizeof(http_eom) - 1;
  

this is magic 1

• pos  = strstr((char *)pkt, linefeed);
  

• if (pos > 0)
  

• 	i = pos - (char *)pkt;
  

• else
  

• 	i = hlen;
  

• printf("%.*s", i,  pkt);
  

• wget_state = WGET_TRANSFERRING;
  

• if (strstr((char *)pkt, http_ok) == 0) {
  

• 	debug_cond(DEBUG_WGET,
  

• 		   "wget: Connected Bad Xfer\n");
  

• 	wget_loop_state = NETLOOP_FAIL;
  

• 	wget_send(action, tcp_seq_num, tcp_ack_num, len);
  

• } else {
  

• 	debug_cond(DEBUG_WGET,
  

• 		   "wget: Connctd pkt %p  hlen %x\n",
  

• 		   pkt, hlen);
  

• 	initial_data_seq_num = tcp_seq_num + hlen;
  

• 	pos  = strstr((char *)pkt, content_len);
  

• 	if (!pos) {
  

• 		content_length = -1;
  

• 	} else {
  

• 		pos = pos + sizeof(content_len) + 2;
  

pos +=

• 		strict_strtoul(pos, 10, &content_length);
  

• 		debug_cond(DEBUG_WGET,
  

• 			   "wget: Connected Len %lu\n",
  

• 			   content_length);
  

• 	}
  

• 	net_boot_file_size = 0;
  

• 	if (len > hlen)
  

• 		store_block(pkt + hlen, 0, len - hlen);
  

newline.

• 	debug_cond(DEBUG_WGET,
  

• 		   "wget: Connected Pkt %p hlen %x\n",
  

• 		   pkt, hlen);
  

• 	for (i = 0; i < pkt_q_idx; i++) {
  

• 		store_block(pkt_q[i].pkt,
  

• 			    pkt_q[i].tcp_seq_num -
  

• 				initial_data_seq_num,
  

• 			    pkt_q[i].len);
  

• 	debug_cond(DEBUG_WGET,
  

• 		   "wget: Connctd pkt Q %p len %x\n",
  

• 		   pkt_q[i].pkt, pkt_q[i].len);
  

weird indenation.

• 	}
  

• }
  

• }
• wget_send(action, tcp_seq_num, tcp_ack_num, len);

+}

+/*

• • In the "application push" invocation, the TCP header with all
• • its information is pointed to by the packet pointer.
• • in the typedef

• •  void rxhand_tcp(uchar *pkt, unsigned int dport,
    

• •                  struct in_addr sip, unsigned int sport,
    

• •                  unsigned int len);
    

• • *pkt is the pointer to the payload
• • dport is used for tcp_seg_num
• • action_and_state.s_addr is used for TCP state
• • sport is used for tcp_ack_num (which is unused by the app)
• • pkt_ length is the payload length.
• */

use kernel-doc here and aligned it with function below.

+static void wget_handler(uchar *pkt, unsigned int tcp_seq_num,

• 	 struct in_addr action_and_state,
  

• 	 unsigned int tcp_ack_num, unsigned int len)
  

+{

• enum TCP_STATE wget_tcp_state = tcp_get_tcp_state();
• u8 action = action_and_state.s_addr;
• net_set_timeout_handler(wget_timeout, wget_timeout_handler);
• packets++;
• switch (wget_state) {
• case WGET_CLOSED:

• debug_cond(DEBUG_WGET, "wget: Handler: Error!, State wrong\n");
  

• break;
  

• case WGET_CONNECTING:

• debug_cond(DEBUG_WGET,
  

• 	   "wget: Connecting In len=%x, Seq=%x, Ack=%x\n",
  

• 	   len, tcp_seq_num, tcp_ack_num);
  

• if (len == 0) {
  

!len

• 	if (wget_tcp_state == TCP_ESTABLISHED) {
  

• 		debug_cond(DEBUG_WGET,
  

• 			   "wget: Cting, send, len=%x\n", len);
  

• 		wget_send(action, tcp_seq_num, tcp_ack_num,
  

• 			  len);
  

• 	} else {
  

• 		printf("%.*s", len,  pkt);
  

• 		wget_fail("wget: Handler Connected Fail\n",
  

• 			  tcp_seq_num, tcp_ack_num, action);
  

• 	}
  

• }
  

• break;
  

• case WGET_CONNECTED:

• debug_cond(DEBUG_WGET, "wget: Connected seq=%x, len=%x\n",
  

• 	   tcp_seq_num, len);
  

• if (len == 0) {
  

!len

• 	wget_fail("Image not found, no data returned\n",
  

• 		  tcp_seq_num, tcp_ack_num, action);
  

• } else {
  

• 	wget_connected(pkt, tcp_seq_num, action_and_state,
  

• 		       tcp_ack_num, len);
  

• }
  

• break;
  

• case WGET_TRANSFERRING:

• debug_cond(DEBUG_WGET,
  

• 	   "wget: Transferring, seq=%x, ack=%x,len=%x\n",
  

• 	   tcp_seq_num, tcp_ack_num, len);
  

• if (store_block(pkt,
  

• 		tcp_seq_num - initial_data_seq_num, len) != 0) {
  

• 	wget_fail("wget: store error\n",
  

• 		  tcp_seq_num, tcp_ack_num, action);
  

• 	return;
  

• }
  

• switch (wget_tcp_state) {
  

• case TCP_FIN_WAIT_2:
  

• 	wget_send(TCP_ACK, tcp_seq_num, tcp_ack_num, len);
  

do you really want to fallthrough here? If yes label it.

• case TCP_SYN_SENT:
  

• case TCP_CLOSING:
  

• case TCP_FIN_WAIT_1:
  

• case TCP_CLOSED:
  

• 	net_set_state(NETLOOP_FAIL);
  

• 	break;
  

• case TCP_ESTABLISHED:
  

• 	wget_send(TCP_ACK, tcp_seq_num, tcp_ack_num,
  

• 		  len);
  

• 	wget_loop_state = NETLOOP_SUCCESS;
  

• 	break;
  

• case TCP_CLOSE_WAIT:     /* End of transfer */
  

• 	wget_state = WGET_TRANSFERRED;
  

• 	wget_send(action | TCP_ACK | TCP_FIN,
  

• 		  tcp_seq_num, tcp_ack_num, len);
  

• 	break;
  

• }
  

• break;
  

• case WGET_TRANSFERRED:

• printf("Packets received %d, Transfer Successful\n", packets);
  

• net_set_state(wget_loop_state);
  

• break;
  

• }

+}

+/*

• • make port a little random (1024-17407)
• • This keeps the math somewhat trivial to compute, and seems to work with
• • all supported protocols/clients/servers
• */

kernel-doc please.

+static unsigned int random_port(void) +{

• return 1024 + (get_timer(0) % 0x4000);

And magic values.

+}

+void wget_start(void) +{

• image_url = strchr(net_boot_file_name, ':');
• if (image_url > 0) {

• web_server_ip = string_to_ip(net_boot_file_name);
  

• ++image_url;
  

• } else {

• web_server_ip = net_server_ip;
  

• image_url = net_boot_file_name;
  

• }
• debug_cond(DEBUG_WGET,

•    "wget: Transfer HTTP Server %pI4; our IP %pI4\n",
  

•    &web_server_ip, &net_ip);
  

• /* Check if we need to send across this subnet */
• if (net_gateway.s_addr && net_netmask.s_addr) {

• struct in_addr our_net;
  

• struct in_addr server_net;
  

• our_net.s_addr = net_ip.s_addr & net_netmask.s_addr;
  

• server_net.s_addr = net_server_ip.s_addr & net_netmask.s_addr;
  

• if (our_net.s_addr != server_net.s_addr)
  

• 	debug_cond(DEBUG_WGET,
  

• 		   "wget: sending through gateway %pI4",
  

• 		   &net_gateway);
  

• }
• debug_cond(DEBUG_WGET, "URL '%s'\n", image_url);
• if (net_boot_file_expected_size_in_blocks) {

• debug_cond(DEBUG_WGET, "wget: Size is 0x%x Bytes = ",
  

• 	   net_boot_file_expected_size_in_blocks << 9);
  

magic value 9.

• print_size(net_boot_file_expected_size_in_blocks << 9, "");
  

• }
• debug_cond(DEBUG_WGET,

•    "\nwget:Load address: 0x%lx\nLoading: *\b", image_load_addr);
  

• net_set_timeout_handler(wget_timeout, wget_timeout_handler);
• tcp_set_tcp_handler(wget_handler);
• wget_timeout_count = 0;
• wget_state = WGET_CLOSED;
• our_port = random_port();
• /*

• * Zero out server ether to forece apr resolution in case
  

• * the server ip for the previous u-boot commsnd, for exanple dns
  

• * is not the same as the web server ip.
  

• */
  

• memset(net_server_ethaddr, 0, 6);
• wget_send(TCP_SYN, 0, 0, 0);

+}

M