Skip to content

net/tcp: Remove the unused TCP_STOPPED flags #7528

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
xiaoxiang781216 wants to merge 1 commit into from
+21 −53
Closed

net/tcp: Remove the unused TCP_STOPPED flags #7528

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions include/nuttx/net/tcp.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,8 +95,7 @@
# define TCP_CLOSING 0x07
# define TCP_TIME_WAIT 0x08
# define TCP_LAST_ACK 0x09
# define TCP_STOPPED 0x10 /* Bit 4: stopped */
/* Bit 5-7: Unused, but not available */
/* Bit 4-7: Unused, but not available */

/* TCP header sizes
*
Expand Down
30 changes: 8 additions & 22 deletions net/sixlowpan/sixlowpan_tcpsend.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -242,33 +242,19 @@ static int sixlowpan_tcp_header(FAR struct tcp_conn_s *conn,
memcpy(ipv6tcp->tcp.ackno, conn->rcvseq, 4); /* ACK number */
memcpy(ipv6tcp->tcp.seqno, conn->sndseq, 4); /* Sequence number */

/* Set the TCP window */
/* Update the TCP received window based on I/O buffer availability */

if (conn->tcpstateflags & TCP_STOPPED)
Copy link
Copy Markdown
Contributor

@antmerlino antmerlino Nov 7, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@xiaoxiang781216 The flag is checked right here. It sets the window to zero so that the host stops sending data. Why do you say that the code is dead?

Copy link
Copy Markdown
Contributor Author

@xiaoxiang781216 xiaoxiang781216 Nov 7, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I mean there isn't code to set STOPPED flag, not the check:(. So, the if block never execute.

Copy link
Copy Markdown
Contributor

@antmerlino antmerlino Nov 7, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. I can't seem to find whether it was ever set or not. Alot has changed. I can try and dig into it later, but not sure when I'll have time. If it wasn't ever fully supported, then it likely came from the Contiki port that Greg originally did.

In either case - why get rid of this logic? It seems like the right behavior, even if we don't look for the STOPPED flag right now. If we were to look for the STOPPED flag, then this would be right. What's the rationale for removing it?

The STOPPED flag is a legitimate TCP flag. This logic, likely copied from Contiki, is correct. I think we should either set the STOPPED flag and allow this logic to do it's job, or not remove it and leave it as is.

Copy link
Copy Markdown
Contributor Author

@xiaoxiang781216 xiaoxiang781216 Nov 7, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem is that the code doesn't work as expect. If someone plan to implement shutdown function in the future, he will bring back the similar code.
I am fine to keep the dead code, @patacongo what your opinion?

Copy link
Copy Markdown
Contributor

@antmerlino antmerlino Nov 7, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you elaborate on what doesn't work as expected? Maybe we can just fix it?

Copy link
Copy Markdown
Contributor Author

@xiaoxiang781216 xiaoxiang781216 Nov 7, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because nobody set STOPPED flag, one improvement is implement shutdown as @patacongo mention which will set STOPPED definitely.

{
/* If the connection has issued TCP_STOPPED, we advertise a zero
* window so that the remote host will stop sending data.
*/

ipv6tcp->tcp.wnd[0] = 0;
ipv6tcp->tcp.wnd[1] = 0;
}
else
{
/* Update the TCP received window based on I/O buffer availability */
uint32_t rcvseq = tcp_getsequence(conn->rcvseq);
uint32_t recvwndo = tcp_get_recvwindow(dev, conn);

uint32_t rcvseq = tcp_getsequence(conn->rcvseq);
uint32_t recvwndo = tcp_get_recvwindow(dev, conn);
/* Set the TCP Window */

/* Set the TCP Window */
ipv6tcp->tcp.wnd[0] = recvwndo >> 8;
ipv6tcp->tcp.wnd[1] = recvwndo & 0xff;

ipv6tcp->tcp.wnd[0] = recvwndo >> 8;
ipv6tcp->tcp.wnd[1] = recvwndo & 0xff;
/* Update the Receiver Window */

/* Update the Receiver Window */

conn->rcv_adv = rcvseq + recvwndo;
}
conn->rcv_adv = rcvseq + recvwndo;

/* Calculate TCP checksum. */

Expand Down
9 changes: 3 additions & 6 deletions net/tcp/tcp_input.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1016,8 +1016,7 @@ static void tcp_input(FAR struct net_driver_s *dev, uint8_t domain,
* the sequence numbers will be screwed up.
*/

if ((tcp->flags & TCP_FIN) != 0 &&
(conn->tcpstateflags & TCP_STOPPED) == 0)
if ((tcp->flags & TCP_FIN) != 0)
{
/* Needs to be investigated further.
* Windows often sends FIN packets together with the last ACK for
Expand Down Expand Up @@ -1150,12 +1149,10 @@ static void tcp_input(FAR struct net_driver_s *dev, uint8_t domain,
#endif

/* If d_len > 0 we have TCP data in the packet, and we flag this
* by setting the TCP_NEWDATA flag. If the application has stopped
* the data flow using TCP_STOPPED, we must not accept any data
* packets from the remote host.
* by setting the TCP_NEWDATA flag.
*/

if (dev->d_len > 0 && (conn->tcpstateflags & TCP_STOPPED) == 0)
if (dev->d_len > 0)
{
flags |= TCP_NEWDATA;
}
Expand Down
32 changes: 9 additions & 23 deletions net/tcp/tcp_send.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -337,37 +337,23 @@ static void tcp_sendcommon(FAR struct net_driver_s *dev,
tcp->srcport = conn->lport;
tcp->destport = conn->rport;

/* Set the TCP window */
/* Update the TCP received window based on I/O buffer availability */

if (conn->tcpstateflags & TCP_STOPPED)
{
/* If the connection has issued TCP_STOPPED, we advertise a zero
* window so that the remote host will stop sending data.
*/

tcp->wnd[0] = 0;
tcp->wnd[1] = 0;
}
else
{
/* Update the TCP received window based on I/O buffer availability */
uint32_t rcvseq = tcp_getsequence(conn->rcvseq);
uint32_t recvwndo = tcp_get_recvwindow(dev, conn);

uint32_t rcvseq = tcp_getsequence(conn->rcvseq);
uint32_t recvwndo = tcp_get_recvwindow(dev, conn);
/* Update the Receiver Window */

/* Update the Receiver Window */

conn->rcv_adv = rcvseq + recvwndo;
conn->rcv_adv = rcvseq + recvwndo;

#ifdef CONFIG_NET_TCP_WINDOW_SCALE
recvwndo >>= conn->rcv_scale;
recvwndo >>= conn->rcv_scale;
#endif

/* Set the TCP Window */
/* Set the TCP Window */

tcp->wnd[0] = recvwndo >> 8;
tcp->wnd[1] = recvwndo & 0xff;
}
tcp->wnd[0] = recvwndo >> 8;
tcp->wnd[1] = recvwndo & 0xff;

/* Finish the IP portion of the message and calculate checksums */

Expand Down