net/tcp: Remove the unused TCP_STOPPED flags#7528
net/tcp: Remove the unused TCP_STOPPED flags#7528xiaoxiang781216 wants to merge 1 commit intoapache:masterfrom
Conversation
report here: apache#4005 Signed-off-by: Xiang Xiao <xiaoxiang@xiaomi.com>
|
@xiaoxiang781216 since this PR touch 6LoWPAN I added @antmerlino because he depends on it for this drone swarm company. |
antmerlino
left a comment
antmerlino
left a comment
There was a problem hiding this comment.
I am not on the latest codebase and it is not trivial for me to bring my code up to the latest, however I seem to recall this code being added to 6lowpan for a legitimate reason. I recall problems during an FTP transfer from a Windows client. I believe this STOPPED flag is a legitimate part of TCP and it's use here is proper.
I see now a conversation with Greg in another issue trying to recall why STOPPED got added. Can't we look this up via Git history? In any case, I would strongly prefer us to leave this code in 6lowpan.
|
@antmerlino the code just set STOPPED bit, but no place check this flag and do the different action. So, it is dead code, please find the right time to check it again. It isnot a critical issue, let wait you. |
| /* Set the TCP window */ | ||
| /* Update the TCP received window based on I/O buffer availability */ | ||
|
|
||
| if (conn->tcpstateflags & TCP_STOPPED) |
There was a problem hiding this comment.
@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?
There was a problem hiding this comment.
Sorry, I mean there isn't code to set STOPPED flag, not the check:(. So, the if block never execute.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Can you elaborate on what doesn't work as expected? Maybe we can just fix it?
There was a problem hiding this comment.
Because nobody set STOPPED flag, one improvement is implement shutdown as @patacongo mention which will set STOPPED definitely.
4 participants
Summary
report here: #4005
Impact
Remove the dead code
Testing
Pass CI