[U-Boot,v10,3/3] Adding wget

From: Duncan Hare <DuncanCHare@yahoo.com>

Why http and wget:

HTTP is the most efficient file retrieval protocol in common
use. The client send a single request, after TCP connection,
to receive a file of any length.

WGET is the application which implements http file transfer
outside browsers as a file transfer protocol. Versions of
wget exists on many operating systems.
END

Signed-off-by: Duncan Hare <DuncanCHare@yahoo.com>
---

Changes in v10: None

 cmd/Kconfig        |   5 +
 cmd/net.c          |  13 ++
 include/net.h      |  16 +-
 include/net/wget.h |  17 +++
 net/Kconfig        |   7 +-
 net/Makefile       |   1 +
 net/wget.c         | 420 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 7 files changed, 470 insertions(+), 9 deletions(-)
 create mode 100644 include/net/wget.h
 create mode 100644 net/wget.c

Hi Duncan,

On 14 April 2018 at 17:43,  <DH@synoia.com> wrote:
> From: Duncan Hare <DuncanCHare@yahoo.com>
>
> Why http and wget:
>
> HTTP is the most efficient file retrieval protocol in common
> use. The client send a single request, after TCP connection,
> to receive a file of any length.
>
> WGET is the application which implements http file transfer
> outside browsers as a file transfer protocol. Versions of
> wget exists on many operating systems.
> END

Why END?

>
> Signed-off-by: Duncan Hare <DuncanCHare@yahoo.com>
> ---
>
> Changes in v10: None

There is no change log here. Has nothing changed since v1?

This code looks OK to me, but please can you run it through patman? I
see what look like some style errors.

How can we create a test for this? Can we add something to test_net.py ?

Regards,
Simon

Hi Duncan,

On 14 April 2018 at 17:43,  <DH@synoia.com> wrote:
>> From: Duncan Hare <DuncanCHare@yahoo.com>
>>
> Why http and wget:
>> END

Why END?
Don't know.  will delete.

>
> Signed-off-by: Duncan Hare <DuncanCHare@yahoo.com>
> ---
>
> Changes in v10: None

There is no change log here. Has nothing changed since v1?
changed c to C for cover meno tag.

This code looks OK to me, but please can you run it through patman? I
see what look like some style errors.
Has passed patman. Only error messages are for packed structures.

How can we create a test for this? Can we add something to test_net.py ?
Do not see why not. It's only an app and has no hardware dependencies. 
One needs a web server and test file, and code the verify the integrity of the test file in local memory.I used a file with line numbers and a pattern repeated on every "line". Make it easy to findinvalid offsets mis ordered packets.
We use nginx as the web server. We used wbox for a while, but then mvbed to nginx. We could not get Apacheconfigured.
For internet test we use a Ubuntu image & nginx on the Amazon cloud. 
The internet path to Amazon does a nice job of scrambling the order of packets.

Regards,
Duncan

>From: Simon Glass <sjg@chromium.org>
 >To: Duncan Hare <dh@synoia.com> 
 >Sent: Sunday, April 22, 2018 1:10 PM
 >Subject: Re: [PATCH v10 3/3] Adding wget
   >
>Hi Duncan,

>On 17 April 2018 at 15:58, Duncan Hare <dh@synoia.com> wrote:
>> From: Simon Glass <sjg@chromium.org>
>> It has been through patman, and the only errors flagged a packed structures,
>> necessary for packed headers.

>It should be possible in the Python test to enable an http server on localhost with a few lines of code, and then connect to it in the test?
Yes server at port 80. The server can be tested with the wget command which can be installed on linux.I doubt that loop-back like this will produce the scrambling of packet order which is a feature of push down stacks for packet queuesin the internet.
The pi is easy to overrun when testing on a low latency network, because the TCP send window size is defined by the number ofnet_rx_buffers, which is controlled the CONFIG_SYS_RX_ETH_BUFFER in include.net.h,  which sets PKTBUFSRX at the beginning of include/net.h, 
which in-turn defines a buffer pool in net/net.c, array named  net_pkt_buf.
Hence my comment in a different thread about buffering on the pi. Few of the socs appear to use net_pkt_buf  buffers for net traffic.

If there are too many transmission errors the sending tcp drops the connection. My solution to this is to halve the size of 
CONFIG_SYS_RX_ETH_BUFFER until transmission works.
If I was thinking about a buffer pool for in the drivers, I'd implement it in net/net.c. Interface "getbuffer," returns a pointer, 
and increments an index to the next net_rx_buffer with no protection for overrun. Overrun is managed by ack numbers in TCPand timeout in UDP.

Possibly CONFIG_SYS_RX_ETH_BUFFER could come under Kconfig.

>Regards,
>Simon
RegardsDuncan

Hi Duncan,

On 22 April 2018 at 21:22, Duncan Hare <dh@synoia.com> wrote:
>
>
> ________________________________
>>From: Simon Glass <sjg@chromium.org>
>>To: Duncan Hare <dh@synoia.com>
>>Sent: Sunday, April 22, 2018 1:10 PM
>>Subject: Re: [PATCH v10 3/3] Adding wget
>>
>>Hi Duncan,
>
>>On 17 April 2018 at 15:58, Duncan Hare <dh@synoia.com> wrote:
>>> From: Simon Glass <sjg@chromium.org>
>
>>> It has been through patman, and the only errors flagged a packed
>>> structures,
>>> necessary for packed headers.
>
>>It should be possible in the Python test to enable an http server on
>> localhost with a few lines of code, and then connect to it in the test?
>
> Yes server at port 80. The server can be tested with the wget command
which
> can be installed on linux.
> I doubt that loop-back like this will produce the scrambling of packet
order
> which is a feature of push down stacks for packet queues
> in the internet.
>
> The pi is easy to overrun when testing on a low latency network, because
the
> TCP send window size is defined by the number of
> net_rx_buffers, which is controlled the CONFIG_SYS_RX_ETH_BUFFER in
> include.net.h,  which sets PKTBUFSRX at the beginning of include/net.h,
> which in-turn defines a buffer pool in net/net.c, array named
net_pkt_buf.
>
> Hence my comment in a different thread about buffering on the pi. Few of
the
> socs appear to use net_pkt_buf  buffers for net traffic.
>
> If there are too many transmission errors the sending tcp drops the
> connection. My solution to this is to halve the size of
> CONFIG_SYS_RX_ETH_BUFFER until transmission works.
>
> If I was thinking about a buffer pool for in the drivers, I'd implement it
> in net/net.c. Interface "getbuffer," returns a pointer,
> and increments an index to the next net_rx_buffer with no protection for
> overrun. Overrun is managed by ack numbers in TCP
> and timeout in UDP.
>
> Possibly CONFIG_SYS_RX_ETH_BUFFER could come under Kconfig.

Just to be clear, I was wondering about having an automated test. Manual
tests are not very useful since people won't do them. See 'make tests' for
all the test that we currently run. I'm pretty sure you could standard up a
little server, run your wget, then shut it down, all within a pytest test.

Regards,
Simon

From: Simon Glass <sjg@chromium.org>
 To: Duncan Hare <dh@synoia.com> 
Cc: U-Boot Mailing List <u-boot@lists.denx.de>; Joe Hershberger <joe.hershberger@ni.com>
 Sent: Tuesday, April 24, 2018 10:01 PM
 Subject: Re: [PATCH v10 3/3] Adding wget
   
Hi Duncan,

On 22 April 2018 at 21:22, Duncan Hare <dh@synoia.com> wrote:
>
>>The server can be tested with the wget command which
>> can be installed on linux.
>> I doubt that loop-back like this will produce the scrambling of packet order
>> which is a feature of push down stacks for packet queues
>> in the internet.
>>
>> Hence my comment in a different thread about buffering on the pi. Few of the
>> socs appear to use net_pkt_buf  buffers for net traffic.
>>
>> If there are too many transmission errors the sending tcp drops the
>> connection. My solution to this is to halve the size of
>> CONFIG_SYS_RX_ETH_BUFFER until transmission works.
>>
 > 
>> Possibly CONFIG_SYS_RX_ETH_BUFFER could come under Kconfig.
>
>>Just to be clear, I was wondering about having an automated test. Manual tests are not very useful since people won't do them. See 'make tests' for all the test that we >currently >run. I'm pretty sure you could standard up a little server, run your wget, then shut it down, all within a pytest test.

>>Regards,
>>Simon

Hi Wolfgang. Simon
Can we put a test 4 Mbyte kernel on the u-boot website for an automated test for other users of TCP & Wget in u-boot?
Then I can produce a standard u-boot script for testing.
RegardsDuncan Hare

Hi Duncan,

On 25 April 2018 at 08:33, Duncan Hare <dh@synoia.com> wrote:
>
>
> ________________________________
> From: Simon Glass <sjg@chromium.org>
> To: Duncan Hare <dh@synoia.com>
> Cc: U-Boot Mailing List <u-boot@lists.denx.de>; Joe Hershberger
> <joe.hershberger@ni.com>
> Sent: Tuesday, April 24, 2018 10:01 PM
> Subject: Re: [PATCH v10 3/3] Adding wget
>
> Hi Duncan,
>
> On 22 April 2018 at 21:22, Duncan Hare <dh@synoia.com> wrote:
>>
>>>The server can be tested with the wget command which
>>> can be installed on linux.
>>> I doubt that loop-back like this will produce the scrambling of packet
>>> order
>>> which is a feature of push down stacks for packet queues
>>> in the internet.
>>>
>>> Hence my comment in a different thread about buffering on the pi. Few of
>>> the
>>> socs appear to use net_pkt_buf  buffers for net traffic.
>>>
>>> If there are too many transmission errors the sending tcp drops the
>>> connection. My solution to this is to halve the size of
>>> CONFIG_SYS_RX_ETH_BUFFER until transmission works.
>>>
>  >
>>> Possibly CONFIG_SYS_RX_ETH_BUFFER could come under Kconfig.
>>
>>>Just to be clear, I was wondering about having an automated test. Manual
>>> tests are not very useful since people won't do them. See 'make tests' for
>>> all the test that we >currently >run. I'm pretty sure you could standard up
>>> a little server, run your wget, then shut it down, all within a pytest test.
>
>
>>>Regards,
>>>Simon
>
> Hi Wolfgang. Simon
>
> Can we put a test 4 Mbyte kernel on the u-boot website for an automated test
> for other users of TCP & Wget in u-boot?
>
> Then I can produce a standard u-boot script for testing.

How about the test just creates a little (4KB) file. We don't want the
tests to access a real network, if possible, just use localhost.

Regards,
Simon

From: Simon Glass <sjg@chromium.org>
 To: Duncan Hare <dh@synoia.com> 
Cc: Wolfgang Denk <wd@denx.de>; U-Boot Mailing List <u-boot@lists.denx.de>; Joe Hershberger <joe.hershberger@ni.com>
 Sent: Wednesday, April 25, 2018 4:44 PM
 Subject: Re: [PATCH v10 3/3] Adding wget
   
Hi Duncan,

On 25 April 2018 at 08:33, Duncan Hare <dh@synoia.com> wrote:

On Tue, Apr 17, 2018 at 10:10 AM, Simon Glass <sjg@chromium.org> wrote:
> Hi Duncan,
>
> On 14 April 2018 at 17:43,  <DH@synoia.com> wrote:
>> From: Duncan Hare <DuncanCHare@yahoo.com>
>>
>> Why http and wget:
>>
>> HTTP is the most efficient file retrieval protocol in common
>> use. The client send a single request, after TCP connection,
>> to receive a file of any length.
>>
>> WGET is the application which implements http file transfer
>> outside browsers as a file transfer protocol. Versions of
>> wget exists on many operating systems.
>> END
>
> Why END?

I would assume that (from reading this blurb) this is really a series
cover letter and the END would terminate that... so the Cover-letter:
tag is probably malformed.

Which begs the question, where the actual log is for the wget patch?

>>
>> Signed-off-by: Duncan Hare <DuncanCHare@yahoo.com>
>> ---
>>
>> Changes in v10: None
>
> There is no change log here. Has nothing changed since v1?

Duncan has been struggling to use patman effectively.

>
> This code looks OK to me, but please can you run it through patman? I
> see what look like some style errors.

This is after using patman since v2 or so.

>
> How can we create a test for this? Can we add something to test_net.py ?
>
> Regards,
> Simon
> _______________________________________________
> U-Boot mailing list
> U-Boot@lists.denx.de
> https://lists.denx.de/listinfo/u-boot

On Wed, Apr 25, 2018 at 6:52 PM, Duncan Hare <dh@synoia.com> wrote:
>
>
>    From: Simon Glass <sjg@chromium.org>
>  To: Duncan Hare <dh@synoia.com>
> Cc: Wolfgang Denk <wd@denx.de>; U-Boot Mailing List <u-boot@lists.denx.de>; Joe Hershberger <joe.hershberger@ni.com>
>  Sent: Wednesday, April 25, 2018 4:44 PM
>  Subject: Re: [PATCH v10 3/3] Adding wget
>
> Hi Duncan,
>
> On 25 April 2018 at 08:33, Duncan Hare <dh@synoia.com> wrote:
> ____________________
>>> From: Simon Glass <sjg@chromium.org>
>>> To: Duncan Hare <dh@synoia.com>
>>> Cc: U-Boot Mailing List <u-boot@lists.denx.de>; Joe Hershberger
>>> <joe.hershberger@ni.com>
>>> Sent: Tuesday, April 24, 2018 10:01 PM
>>> Subject: Re: [PATCH v10 3/3] Adding wget
>>>
>>> Hi Duncan,
>>>
>>>> On 22 April 2018 at 21:22, Duncan Hare <dh@synoia.com> wrote:
>>>>
>>>>>The server can be tested with the wget command which
>>>>> can be installed on linux.
>>>>> I doubt that loop-back like this will produce the scrambling of packet
>>>>> order
>>>>> which is a feature of push down stacks for packet queues
>>>>> in the internet.
>>>>>
>>>>> Hence my comment in a different thread about buffering on the pi. Few of
>>>>> the
>>>>> socs appear to use net_pkt_buf  buffers for net traffic.
>>>>>
>>>>> If there are too many transmission errors the sending tcp drops the
>>>>> connection. My solution to this is to halve the size of
>>>>> CONFIG_SYS_RX_ETH_BUFFER until transmission works.
>>>>>
>>>> >
>>>>> Possibly CONFIG_SYS_RX_ETH_BUFFER could come under Kconfig.
>>>>
>>>>Just to be clear, I was wondering about having an automated test. Manual
>>>>> tests are not very useful since people won't do them. See 'make tests' for
>>>>> all the test that we >currently >run. I'm pretty sure you could standard up
>>>>> a little server, run your wget, then shut it down, all within a pytest test.
>>>
>>>
>>>>>Regards,
>>>>>Simon
>>>
>>> Hi Wolfgang. Simon
>>>
>>> Can we put a test 4 Mbyte kernel on the u-boot website for an automated test
>>> for other users of TCP & Wget in u-boot?
>>>
>>> Then I can produce a standard u-boot script for testing.
>
>>How about the test just creates a little (4KB) file. We don't want the
>>tests to access a real network, if possible, just use localhost.

This makes it portable to where ever the test is run.

>
>>Regards,
>>Simon
> 4k is 4 packets. I believe most kernels are larger.
> I was think of a static server set up with a known dns name.
> Thta's what I've got.
>
> Do the test setup once.

This assume accessibility to this Internet server and that this server
is up /still configured when the test runs.

Please setup a test that can run in an environment without the
Internet. That is critical for unit tests.

Hand tests for Internet usage and the environmental effects are great,
but that can't be what we include in the auto tests for repeat-ability
reasons. Simon is asking for a separate type of test.

>
>
>
> _______________________________________________
> U-Boot mailing list
> U-Boot@lists.denx.de
> https://lists.denx.de/listinfo/u-boot

On Sat, Apr 14, 2018 at 6:43 PM,  <DH@synoia.com> wrote:
> From: Duncan Hare <DuncanCHare@yahoo.com>
>
> Why http and wget:
>
> HTTP is the most efficient file retrieval protocol in common
> use. The client send a single request, after TCP connection,
> to receive a file of any length.
>
> WGET is the application which implements http file transfer
> outside browsers as a file transfer protocol. Versions of
> wget exists on many operating systems.
> END
>
> Signed-off-by: Duncan Hare <DuncanCHare@yahoo.com>
> ---
>
> Changes in v10: None
>
>  cmd/Kconfig        |   5 +
>  cmd/net.c          |  13 ++
>  include/net.h      |  16 +-
>  include/net/wget.h |  17 +++
>  net/Kconfig        |   7 +-
>  net/Makefile       |   1 +
>  net/wget.c         | 420 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>  7 files changed, 470 insertions(+), 9 deletions(-)
>  create mode 100644 include/net/wget.h
>  create mode 100644 net/wget.c
>
> diff --git a/cmd/Kconfig b/cmd/Kconfig
> index 136836d146..1113d69950 100644
> --- a/cmd/Kconfig
> +++ b/cmd/Kconfig
> @@ -1094,6 +1094,11 @@ config CMD_ETHSW
>           operations such as enabling / disabling a port and
>           viewing/maintaining the filtering database (FDB)
>
> +config CMD_WGET
> +       bool "wget"
> +       select TCP
> +       help
> +         Download a kernel, or other files, from a web server over TCP.
>  endif
>
>  endmenu
> diff --git a/cmd/net.c b/cmd/net.c
> index d7c776aacf..6a7a51f357 100644
> --- a/cmd/net.c
> +++ b/cmd/net.c
> @@ -110,6 +110,19 @@ U_BOOT_CMD(
>  );
>  #endif
>
> +#if defined(CONFIG_CMD_WGET)
> +static int do_wget(cmd_tbl_t *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 e29d804a23..ec44bf2bec 100644
> --- a/include/net.h
> +++ b/include/net.h
> @@ -15,10 +15,14 @@
>  #include <asm/cache.h>
>  #include <asm/byteorder.h>     /* for nton* / ntoh* stuff */
>
> -#define DEBUG_LL_STATE  0      /* Link local state machine changes */
> -#define DEBUG_DEV_PKT   0      /* Packets or info directed to the device */
> +#define DEBUG_LL_STATE  0      /* Link local state machine changes        */
> +#define DEBUG_DEV_PKT   0      /* Packets or info directed to the device  */
>  #define DEBUG_NET_PKT   0      /* Packets on info on the network at large */
> -#define DEBUG_INT_STATE 0      /* Internal network state changes */
> +#define DEBUG_INT_STATE 0      /* Internal network state change           */

Random formatting change... also changed differently in a former patch
if I recall.

> +
> +#if defined(CONFIG_TCP)

Why would a #define need to be protected?

> +#define CONFIG_SYS_RX_ETH_BUFFER 12     /* For TCP */
> +#endif
>
>  /*
>   *     The number of receive packet buffers, and the required packet buffer
> @@ -31,10 +35,6 @@
>   *     data, the sending TCP will stop sending.
>   */
>
> -#if defined(CONFIG_TCP)
> -#define CONFIG_SYS_RX_ETH_BUFFER 12    /* For TCP */
> -#endif

Why not just put it in the proper place in the TCP patch instead of
moving it later?

> -
>  #ifdef CONFIG_SYS_RX_ETH_BUFFER
>  # define PKTBUFSRX     CONFIG_SYS_RX_ETH_BUFFER
>  #else
> @@ -362,8 +362,8 @@ struct vlan_ethernet_hdr {
>  #define PROT_PPP_SES   0x8864          /* PPPoE session messages       */
>
>  #define IPPROTO_ICMP    1      /* Internet Control Message Protocol    */
> +#define IPPROTO_TCP      6      /* Transmission Control Protocol        */
>  #define IPPROTO_UDP    17      /* User Datagram Protocol               */
> -#define IPPROTO_TCP     6      /* Transmission Control Protocol        */

Yeah... same thing... Looks like you didn't ignore me when I made this
in an earlier version, you just applied the change to the wrong patch.

>
>  /*
>   *     Internet Protocol (IP) header.
> diff --git a/include/net/wget.h b/include/net/wget.h
> new file mode 100644
> index 0000000000..dce16c573d
> --- /dev/null
> +++ b/include/net/wget.h
> @@ -0,0 +1,17 @@
> +/*
> + * Duncan Hare Copyright 2017
> + */
> +
> +void wget_start(void);                 /* Begin wget */
> +
> +enum WGET_STATE {
> +       WGET_CLOSED,
> +       WGET_CONNECTING,
> +       WGET_CONNECTED,
> +       WGET_TRANSFERRING,
> +       WGET_TRANSFERRED};

Closing brace on its own line.

> +
> +#define        DEBUG_WGET              0       /* Set to 1 for debug messges */
> +#define        SERVER_PORT             80
> +#define        WGET_RETRY_COUNT        30
> +#define        WGET_TIMEOUT            2000UL
> diff --git a/net/Kconfig b/net/Kconfig
> index cc6dd83fc4..c973def3b8 100644
> --- a/net/Kconfig
> +++ b/net/Kconfig
> @@ -36,7 +36,12 @@ config NET_TFTP_VARS
>  config TCP
>         bool "Include Subset TCP stack for wget"
>         help
> -         TCP protocol support for wget.
> +         Selecting TCP protocol support adds TCP for receiving
> +         streams of data. The TCP packets are presented
> +         as received (possibly out of order). The application
> +         receiving the TCP stream places the date by sequence number
> +         as an offset from the kernel address. TCP transmission is
> +         not supported.

This should be in the TCP patch, not modified after-the-fact in the wget patch.

>
>  config BOOTP_BOOTPATH
>         bool "Enable BOOTP BOOTPATH"
> diff --git a/net/Makefile b/net/Makefile
> index 25729ebeeb..f83df5b728 100644
> --- a/net/Makefile
> +++ b/net/Makefile
> @@ -26,6 +26,7 @@ obj-$(CONFIG_CMD_RARP) += rarp.o
>  obj-$(CONFIG_CMD_SNTP) += sntp.o
>  obj-$(CONFIG_CMD_NET)  += tftp.o
>  obj-$(CONFIG_TCP)      += tcp.o
> +obj-$(CONFIG_CMD_WGET) += wget.o
>  # Disable this warning as it is triggered by:
>  # sprintf(buf, index ? "foo%d" : "foo", index)
>  # and this is intentional usage.
> diff --git a/net/wget.c b/net/wget.c
> new file mode 100644
> index 0000000000..5429a1f7b1
> --- /dev/null
> +++ b/net/wget.c
> @@ -0,0 +1,420 @@
> +/*
> + * WGET/HTTP support driver based on U-BOOT's nfs.c
> + * Copyright Duncan Hare <dh@synoia.com> 2017
> + * SPDX-License-Identifier:GPL-2.0+
> + */
> +
> +#include <common.h>
> +#include <command.h>
> +#include <mapmem.h>
> +#include <net.h>
> +#include <net/wget.h>
> +#include <net/tcp.h>
> +
> +const char bootfile1[]   = "GET ";
> +const  char bootfile3[]   = " HTTP/1.0\r\n\r\n";
> +const char http_eom[]     = "\r\n\r\n";
> +const char http_ok[]      = "200";
> +const char content_len[]  = "Content-Length";
> +const char linefeed[]     = "\r\n";
> +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

bufers -> buffers

> + * 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);
> +
> +static enum net_loop_state wget_loop_state;
> +
> +/* Timeout retry parameters */
> +static u8 r_action;
> +static unsigned int r_tcp_ack_num;
> +static unsigned int r_tcp_seq_num;
> +static int r_len;

"r_" is not clear enough... please use "retry_"

> +
> +static inline int store_block(uchar *src, unsigned int offset, unsigned int len)
> +{
> +       ulong newsize = offset + len;
> +       uchar *ptr;
> +
> +#ifdef CONFIG_SYS_DIRECT_FLASH_WGET
> +       int i, rc = 0;
> +
> +       for (i = 0; i < CONFIG_SYS_MAX_FLASH_BANKS; i++) {
> +               /* start address in flash? */
> +               if (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)(load_addr + offset), len);
> +               if (rc) {
> +                       flash_perror(rc);
> +                       return -1;
> +               }
> +       } else {
> +#endif /* CONFIG_SYS_DIRECT_FLASH_WGET */
> +
> +               ptr = map_sysmem(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;
> +       return 0;
> +}
> +
> +/*
> + * wget response dispatcher
> + * WARNING, This, and only this, is the place in wget,c where

wget,c -> wget.c

> + * 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                  = r_action;
> +       unsigned int tcp_ack_num   = r_tcp_ack_num;
> +       unsigned int tcp_seq_num   = r_tcp_seq_num;
> +       int len                    = r_len;
> +       uchar *ptr;
> +       uchar *offset;
> +
> +       tcp_ack_num = tcp_ack_num + len;
> +
> +       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;

Fix indent.

> +       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;
> +               offset = ptr;
> +
> +               memcpy(offset, &bootfile1, strlen(bootfile1));
> +               offset = offset + strlen(bootfile1);
> +
> +               memcpy(offset, image_url, strlen(image_url));
> +               offset = offset + strlen(image_url);
> +
> +               memcpy(offset, &bootfile3, strlen(bootfile3));
> +               offset = offset + strlen(bootfile3);
> +               net_send_tcp_packet((offset - ptr), SERVER_PORT, our_port,
> +                                   TCP_PUSH, tcp_seq_num, tcp_ack_num);
> +               wget_state = WGET_CONNECTED;
> +       break;

Fix indent.

> +       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;

Fix indent... and everywhere else that you add a case structure.

> +       }
> +}
> +
> +static void wget_send(u8 action, unsigned int tcp_ack_num,
> +                     unsigned int tcp_seq_num, int len)
> +{
> +       r_action        = action;
> +       r_tcp_ack_num   = tcp_ack_num;
> +       r_tcp_seq_num   = tcp_seq_num;
> +       r_len           = len;
> +       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");
> +       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;
> +
> +       pkt[len] = '\0';
> +       pos = strstr((char *)pkt, http_eom);
> +
> +       if (pos == 0) {
> +               debug_cond(DEBUG_WGET,
> +                          "wget: Connected, data before Header %p\n", pkt);
> +               pkt_in_q = (void *)load_addr + 0x20000 + (pkt_q_idx * 0x800);
> +               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;
> +               pos  = strstr((char *)pkt, linefeed);
> +               if (pos > 0)
> +                       i = pos - (char *)pkt;
> +               else
> +                       i = hlen;
> +               tcp_print_buffer(pkt, i, i, -1, 0);
> +
> +               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;
> +                               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);
> +                       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);
> +                       }
> +               }
> +       }
> +       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.
> +        */
> +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) {
> +                       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);

Fix indent.

> +                       } else {
> +                               tcp_print_buffer(pkt, len, len, -1, 0);
> +                               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) {
> +                       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);

Just return here, then you don't need the next block to be in the else
case and you can reduce the indentation.

> +               } else {
> +                       switch (wget_tcp_state) {
> +                       case TCP_FIN_WAIT_2:
> +                               wget_send(TCP_ACK, tcp_seq_num, tcp_ack_num,
> +                                         len);
> +                       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;
> +       }
> +}
> +
> +void wget_start(void)
> +{
> +       debug_cond(DEBUG_WGET, "%s\n", __func__);
> +
> +       image_url = strchr(net_boot_file_name, ':');
> +       if (!image_url) {
> +               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);
> +               print_size(net_boot_file_expected_size_in_blocks << 9, "");
> +       }
> +       debug_cond(DEBUG_WGET,
> +                  "\nwget:Load address: 0x%lx\nLoading: *\b", 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 in case the server ip has changed */
> +       memset(net_server_ethaddr, 0, 6);

Why would we expect that to be the case?

> +
> +       wget_send(TCP_SYN, 0, 0, 0);
> +}
> --
> 2.11.0
>
> _______________________________________________
> U-Boot mailing list
> U-Boot@lists.denx.de
> https://lists.denx.de/listinfo/u-boot

>>
>>Please setup a test that can run in an environment without the
>>Internet. That is critical for unit tests.
>>
>>Hand tests for Internet usage and the environmental effects are great,
>>but that can't be what we include in the auto tests for repeat-ability
>>reasons. Simon is asking for a separate type of test.

Is the test environment another patch in the series or an addition to the wget patch?
Thanks Duncan

Hi,

On 14 May 2018 at 07:05, Duncan Hare <dh@synoia.com> wrote:
>
>
>
> ________________________________
>>>
>>>Please setup a test that can run in an environment without the
>>>Internet. That is critical for unit tests.
>>>
>>>Hand tests for Internet usage and the environmental effects are great,
>>>but that can't be what we include in the auto tests for repeat-ability
>>>reasons. Simon is asking for a separate type of test.
>
> Is the test environment another patch in the series or an addition to the
> wget patch?

I suggest a separate patch.

Regards,
Simon

From: Simon Glass <sjg@chromium.org>


 To: Duncan Hare <dh@synoia.com> 
Cc: "joe.hershberger@ni.com" <joe.hershberger@ni.com>; U-Boot Mailing List <u-boot@lists.denx.de>
 Sent: Sunday, May 13, 2018 3:00 PM
 Subject: Re: net: [U-Boot] [PATCH v10 3/3] Adding wget
   
>>>>
>>>>Please setup a test that can run in an environment without the
>>>>Internet. That is critical for unit tests.
>>>>
>>>>Hand tests for Internet usage and the environmental effects are great,
>>>>but that can't be what we include in the auto tests for repeat-ability
>>>>reasons. Simon is asking for a separate type of test.
>>>
>>> Is the test environment another patch in the series or an addition to the
>>> wget patch?

>I suggest a separate patch.
>>Regards,>>Simon
Separate patch, patch 4 of the series or a completely new patch?
Conceptual approach:

apt-get install nginx (in debian)
I'll provide a config file pointing to test kernel a a specified file location.

Test kernel will be numbered lines of printable characters. Printable, not binary, easy to expand in a spreadsheet.

Can the u-boot print command print sections of memory? That is
print $loadaddr $length? 

That will work for small test kernels, or 

I tested by using tftp to download a kernel, then used a modified wget to download a seconf time and verify thedownloads were identical.

compare $loadaddr1 $loadaddr2 $length

Is that a preferred approach?
 Duncan Hare

714 931 7952

Hi Duncan,

On 14 May 2018 at 09:26, Duncan Hare <dh@synoia.com> wrote:
> From: Simon Glass <sjg@chromium.org>
>
>
> To: Duncan Hare <dh@synoia.com>
> Cc: "joe.hershberger@ni.com" <joe.hershberger@ni.com>; U-Boot Mailing List
> <u-boot@lists.denx.de>
> Sent: Sunday, May 13, 2018 3:00 PM
> Subject: Re: net: [U-Boot] [PATCH v10 3/3] Adding wget
>
>>>>>
>>>>>Please setup a test that can run in an environment without the
>>>>>Internet. That is critical for unit tests.
>>>>>
>>>>>Hand tests for Internet usage and the environmental effects are great,
>>>>>but that can't be what we include in the auto tests for repeat-ability
>>>>>reasons. Simon is asking for a separate type of test.
>>>>
>>>> Is the test environment another patch in the series or an addition to
>>>> the
>>>> wget patch?
>
>>I suggest a separate patch.
>>
>>Regards,
>>>Simon
>
> Separate patch, patch 4 of the series or a completely new patch?

Patch 4 in the series.

>
> Conceptual approach:
>
> apt-get install nginx (in debian)
>
> I'll provide a config file pointing to test kernel a a specified file
> location.
>
> Test kernel will be numbered lines of printable characters. Printable, not
> binary, easy to expand in a spreadsheet.
>
> Can the u-boot print command print sections of memory? That is
>
> print $loadaddr $length?
>
> That will work for small test kernels, or
>
> I tested by using tftp to download a kernel, then used a modified wget to
> download a seconf time and verify the
> downloads were identical.
>
> compare $loadaddr1 $loadaddr2 $length
>
> Is that a preferred approach?

You don't need to download a kernel. Just download a small file (say
10KB) that you generate in your test. You might find test_vboot.py and
test_net.py helpful. Something like:

- start an http server on localhost (in Python)
- put a single file in the server that can be read (generate 10KB of
known data?)
- run U-Boot with the commands to wget from that server
- check that U-Boot gets the expected file

Regards,
Simon