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
F154647488: D29524.id86697.diff
Wed, Apr 29, 3:21 AM
F154576637: D29524.id86697.diff
Tue, Apr 28, 11:13 PM
Unknown Object (File)
Thu, Apr 23, 12:10 AM
Unknown Object (File)
Mon, Apr 20, 7:37 AM
Unknown Object (File)
Sat, Apr 18, 6:40 PM
Unknown Object (File)
Sat, Apr 18, 9:39 AM
Unknown Object (File)
Fri, Apr 17, 10:44 AM
Unknown Object (File)
Fri, Apr 17, 9:23 AM
View All 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 38251
Build 35140: 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
709

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
711

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/
12 lines

Diff 86691

View Options

sys/netinet/tcp_hostcache.c

Loading...