Page MenuHomeFreeBSD
Differential D29524

tcp: Shouldn't drain empty sbuf
ClosedPublic
Actions

Authored by rscheff on Mar 31 2021, 11:01 PM.
Tags
None
Referenced Files
F82166041: D29524.diff
Fri, Apr 26, 3:10 AM
F82138108: D29524.id86691.diff
Thu, Apr 25, 8:55 PM
F82106934: D29524.id86649.diff
Thu, Apr 25, 2:56 PM
Unknown Object (File)
Thu, Apr 25, 12:24 AM
Unknown Object (File)
Thu, Apr 18, 2:37 PM
Unknown Object (File)
Thu, Apr 18, 8:50 AM
Unknown Object (File)
Sun, Apr 7, 4:34 AM
Unknown Object (File)
Sat, Apr 6, 12:45 PM
View All 39 Files
Subscribers
imp
melifaro

Details

Reviewers
tuexen
jhb
jtl
Group Reviewers
transport
Commits
rGcca0fba78e5f: tcp: Shouldn't drain empty sbuf
rG2a9ce2734f8b: tcp: Shouldn't drain empty sbuf
rG9f3835013032: tcp: Shouldn't drain empty sbuf
rG9aef4e7c2bd4: tcp: Shouldn't drain empty sbuf
Summary

Need a bool protection after all, to not hit a KASSERT

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 38230
Build 35119: arc lint + arc unit

Event Timeline

rscheff created this revision.Mar 31 2021, 11:01 PM
Herald added a reviewer: transport. · View Herald TranscriptMar 31 2021, 11:02 PM
Herald added subscribers: melifaro, imp. · View Herald Transcript
rscheff requested review of this revision.Mar 31 2021, 11:02 PM
Harbormaster completed remote builds in B38230: Diff 86649.Mar 31 2021, 11:02 PM
rscheff updated this revision to Diff 86685.Apr 1 2021, 1:58 PM
  • fix typo in assert string
Harbormaster completed remote builds in B38247: Diff 86685.Apr 1 2021, 1:59 PM
tuexen added inline comments.Apr 1 2021, 2:00 PM
sys/netinet/tcp_hostcache.c
696

Maybe add a comment why you are doing this dance. It wasn't obvious for us...

rscheff updated this revision to Diff 86688.Apr 1 2021, 2:07 PM
  • add comment to explain why do_drain is needed
Harbormaster completed remote builds in B38249: Diff 86688.Apr 1 2021, 2:07 PM
tuexen added inline comments.Apr 1 2021, 2:21 PM
sys/netinet/tcp_hostcache.c
698

Not sure coordination is involved. Isn't it more that one should not call sbuf_drain() with an empty buffer. Please state that and finish the sentence with a full stop (.).

rscheff updated this revision to Diff 86691.Apr 1 2021, 2:34 PM
  • update comment
Harbormaster completed remote builds in B38251: Diff 86691.Apr 1 2021, 2:34 PM
tuexen accepted this revision.Apr 1 2021, 2:40 PM
This revision is now accepted and ready to land.Apr 1 2021, 2:40 PM
Closed by commit rG9aef4e7c2bd4: tcp: Shouldn't drain empty sbuf (authored by rscheff). · Explain WhyApr 1 2021, 3:21 PM
This revision was automatically updated to reflect the committed changes.
rscheff added a commit: rG9aef4e7c2bd4: tcp: Shouldn't drain empty sbuf.
jhb added a comment.Apr 1 2021, 5:38 PM

I would probably instead have changed sbuf_drain() to just return instead of asserting. Asserting made sense when it was a private implementation function, as a public API I think just returning instead of asserting is better behavior.

rscheff added a commit: rG9f3835013032: tcp: Shouldn't drain empty sbuf.Apr 16 2021, 8:47 PM
rscheff added a commit: rG2a9ce2734f8b: tcp: Shouldn't drain empty sbuf.Apr 16 2021, 9:34 PM
rscheff added a commit: rGcca0fba78e5f: tcp: Shouldn't drain empty sbuf.Apr 16 2021, 10:12 PM

Revision Contents
Changeset List

PathSize
sys/
netinet/
7 lines

Diff 86649

View Options

sys/netinet/tcp_hostcache.c

Loading...