[PATCH 07/21] Revert "net: wget: Support retransmission a dropped packet"
Yasuharu Shibata
yasuharu.shibata at gmail.com
Tue Aug 13 05:48:00 CEST 2024
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.
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 <trini at konsulko.com> wrote:
>
> On Sun, Aug 11, 2024 at 08:50:18AM -0600, Simon Glass wrote:
> > +Ying-Chun Liu (PaulLiu) <paul.liu at linaro.org>
> >
> > Hi Yasuharu,
> >
> > On Sat, 10 Aug 2024 at 20:48, Yasuharu Shibata
> > <yasuharu.shibata at 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
More information about the U-Boot
mailing list