Hi Yasuharu,
On Mon, 12 Aug 2024 at 21:50, Yasuharu Shibata
<yasuharu.shib...@gmail.com> wrote:
>
> Dear Simon and Tom,
>
> Thank you for your details of the test and your advice.
> The test has a problem that tcp_ack isn't calculated by the received data
> size.
> sb_ack_handler() in test/cmd/wget.c, tcp_send->tcp_ack calculated by
> following code:
>
> tcp_send->tcp_seq = htonl(ntohl(tcp->tcp_ack));
> tcp_send->tcp_ack = htonl(ntohl(tcp->tcp_seq) + 1);
>
> tcp_ack needs to be calculated by the received TCP payload size.
>
> Also, wget command may have a problem that
> HTTP response from server must be divided into more than two packets.
> wget command receive packets and change internal state in
> wget_handler() as follows:
>
> 1. When current_wget_state is WGET_CONNECTED,
> wget_handler() calls wget_connected().
> It receives HTTP response and moves WGET_TRANSFERRING.
> 2. When current_wget_state is WGET_TRANSFERRING
> and wget_tcp_state is TCP_ESTABLISHED,
> wget_handler() receives HTTP response and
> wget_loop_state sets NETLOOP_SUCCESS.
> 3. When current_wget_state is WGET_TRANSFERRING
> and wget_tcp_state is TCP_CLOSE_WAIT,
> call net_set_state with wget_loop_state.
>
> If there are more than two HTTP response packets,
> wget executes in the order 1 -> 2 -> 3 and terminates properly.
> If there is only one HTTP response packet, wget executes in the order 1 -> 3,
> wget_loop_state remains NETLOOP_CONTINUE and wget doesn't terminate
> successfully.
>
> I wrote the patch at the end of this mail.
> The wget issue is temporarily fixed on the test side.
> This may need to be fixed on the wget side in the future.
> In addition, I fixed the HTTP response returned at the correct timing.
> If the patch is OK, I will send format-patch.
Thank you for looking at this and for all the info. Yes, the patch
looks good to me.
Regards,
Simon
>
> Best regards,
> Yasuharu Shibata
>
> —
> diff --git a/test/cmd/wget.c b/test/cmd/wget.c
> index 356a4dcd8f..32542cdfe2 100644
> --- a/test/cmd/wget.c
> +++ b/test/cmd/wget.c
> @@ -26,6 +26,8 @@
> #define SHIFT_TO_TCPHDRLEN_FIELD(x) ((x) << 4)
> #define LEN_B_TO_DW(x) ((x) >> 2)
>
> +int net_set_ack_options(union tcp_build_pkt *b);
> +
> static int sb_arp_handler(struct udevice *dev, void *packet,
> unsigned int len)
> {
> @@ -105,6 +107,10 @@ static int sb_ack_handler(struct udevice *dev,
> void *packet,
> const char *payload1 = "HTTP/1.1 200 OK\r\n"
> "Content-Length: 30\r\n\r\n\r\n"
> "<html><body>Hi</body></html>\r\n";
> + union tcp_build_pkt *b = (union tcp_build_pkt *)tcp;
> + const int recv_payload_len = len - net_set_ack_options(b) -
> IP_HDR_SIZE - ETHER_HDR_SIZE;
> + static int next_seq = 0;
> + const int bottom_payload_len = 10;
>
> /* Don't allow the buffer to overrun */
> if (priv->recv_packets >= PKTBUFSRX)
> @@ -119,13 +125,31 @@ static int sb_ack_handler(struct udevice *dev,
> void *packet,
> tcp_send->tcp_dst = tcp->tcp_src;
> data = (void *)tcp_send + IP_TCP_HDR_SIZE;
>
> - if (ntohl(tcp->tcp_seq) == 1 && ntohl(tcp->tcp_ack) == 1) {
> + if (ntohl(tcp->tcp_seq) == 1 && ntohl(tcp->tcp_ack) == 1 &&
> recv_payload_len == 0) {
> + // ignore ACK for three-way handshaking
> + return 0;
> + } else if (ntohl(tcp->tcp_seq) == 1 && ntohl(tcp->tcp_ack) == 1) {
> + // recv HTTP request message and reply top half data
> tcp_send->tcp_seq = htonl(ntohl(tcp->tcp_ack));
> - tcp_send->tcp_ack = htonl(ntohl(tcp->tcp_seq) + 1);
> - payload_len = strlen(payload1);
> + tcp_send->tcp_ack = htonl(ntohl(tcp->tcp_seq) +
> recv_payload_len);
> +
> + payload_len = strlen(payload1) - bottom_payload_len;
> memcpy(data, payload1, payload_len);
> tcp_send->tcp_flags = TCP_ACK;
> - } else if (ntohl(tcp->tcp_seq) == 2) {
> +
> + next_seq = ntohl(tcp_send->tcp_seq) + payload_len;
> + } else if (ntohl(tcp->tcp_ack) == next_seq) {
> + // reply bottom half data
> + const int top_payload_len = strlen(payload1) -
> bottom_payload_len;
> +
> + tcp_send->tcp_seq = htonl(next_seq);
> + tcp_send->tcp_ack = htonl(ntohl(tcp->tcp_seq) +
> recv_payload_len);
> +
> + payload_len = bottom_payload_len;
> + memcpy(data, payload1 + top_payload_len, payload_len);
> + tcp_send->tcp_flags = TCP_ACK;
> + } else {
> + // close connection
> tcp_send->tcp_seq = htonl(ntohl(tcp->tcp_ack));
> tcp_send->tcp_ack = htonl(ntohl(tcp->tcp_seq) + 1);
> payload_len = 0;
> @@ -148,11 +172,9 @@ static int sb_ack_handler(struct udevice *dev,
> void *packet,
> pkt_len,
> IPPROTO_TCP);
>
> - if (ntohl(tcp->tcp_seq) == 1 || ntohl(tcp->tcp_seq) == 2) {
> - priv->recv_packet_length[priv->recv_packets] =
> - ETHER_HDR_SIZE + IP_TCP_HDR_SIZE + payload_len;
> - ++priv->recv_packets;
> - }
> + priv->recv_packet_length[priv->recv_packets] =
> + ETHER_HDR_SIZE + IP_TCP_HDR_SIZE + payload_len;
> + ++priv->recv_packets;
>
> return 0;
>
>
> }
>
> On Tue, 13 Aug 2024 at 03:46, Tom Rini <tr...@konsulko.com> wrote:
> >
> > On Sun, Aug 11, 2024 at 08:50:18AM -0600, Simon Glass wrote:
> > > +Ying-Chun Liu (PaulLiu) <paul....@linaro.org>
> > >
> > > Hi Yasuharu,
> > >
> > > On Sat, 10 Aug 2024 at 20:48, Yasuharu Shibata
> > > <yasuharu.shib...@gmail.com> wrote:
> > > >
> > > > Dear Simon,
> > > >
> > > > Could you inform me how the wget test was broken?
> > > > As I explained in the commit log, I fixed the bug in specific
> > > > conditions.
> > > > Without the details of how the break happened,
> > > > it is difficult to find a proper patch.
> > > >
> > > > Best regards,
> > > > Yasuharu Shibata
> > >
> > > Yes, I should have mentioned that. It is test/cmd/wget.c - see also [1].
> > >
> > > This is a good run:
> > > Test: net_test_wget: wget.c
> > > HTTP/1.1 200 OK
> > > Packets received 5, Transfer Successful
> > > Bytes transferred = 32 (20 hex)
> > > md5 for 00020000 ... 0002001f ==> 234af48e94b0085060249ecb5942ab57
> > > Failures: 0
> > >
> > > With your patch it hangs:
> > >
> > > Test: net_test_wget: wget.c
> > > HTTP/1.1 200 OKT
> >
> > Can you please figure out what's going on with the sandbox test then?
> > The change in question fixes wget support on real hardware, and also the
> > other wget tests (as part of the lwIP series) work fine as well. This
> > seems like something specific to that test.
> >
> > --
> > Tom
