-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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.
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
net/tcp: Remove the unused TCP_STOPPED flags #7528
Conversation
report here: apache#4005 Signed-off-by: Xiang Xiao <[email protected]>
@xiaoxiang781216 since this PR touch 6LoWPAN I added @antmerlino because he depends on it for this drone swarm company. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. |
|
||
if (conn->tcpstateflags & TCP_STOPPED) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
Summary
report here: #4005
Impact
Remove the dead code
Testing
Pass CI