Document sysctl MIBs in sctp(4)
ClosedPublic
Actions

Authored by marieheleneka_gmail.com on Sep 7 2015, 8:42 AM.

Details

Reviewers

allanjude
wblock
tuexen

Group Reviewers

manpages

Commits

rS339867: MFC r339791:
rS339791: Add initial descriptions for SCTP related MIB variable.

Summary

PR: https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=184110

This is a first attempt at documenting the sysctl(3) MIBs belonging to sctp(4) in the sctp(4) man page. The documentation is a copy-paste of sysctl -d net.inet.sctp, with some mandoc formatting. I can't find additional documentation on these MIBs, but I figure it's better to have them in the manpage than to have the manpage pretend they don't exist at all.

Additionally:
Added subsection headers for socket options (which were already described), and aforementioned MIBs.
Added a reference to the blackhole(4) man page under the SEE ALSO section.

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

marieheleneka_gmail.com updated this revision to Diff 8541.Sep 7 2015, 8:42 AM

marieheleneka_gmail.com retitled this revision from to Document sysctl MIBs in sctp(4).

marieheleneka_gmail.com updated this object.

marieheleneka_gmail.com edited the test plan for this revision. (Show Details)

marieheleneka_gmail.com added a reviewer: allanjude.

marieheleneka_gmail.com set the repository for this revision to rS FreeBSD src repository - subversion.

Herald added a reviewer: manpages. · View Herald TranscriptSep 7 2015, 8:42 AM

Herald added a subscriber: imp. · View Herald Transcript

Please include context as shown here: https://wiki.freebsd.org/CodeReview#Create_a_Revision_via_Web_Interface

share/man/man4/sctp.4
430 ↗	(On Diff #8541)	Please start new sentences on a new line. Be consistent about whether these entries are sentences ending with a period. To match the rest, this can be Enable SCTP blackholing .Pq Xr blackhole 4
436 ↗	(On Diff #8541)	s/the sames/duplicates/, but "it takes to try" is difficult to interpret, and this sentence can probably be rewritten. "cwnd" is used several times, be consistent with the capitalization.
443 ↗	(On Diff #8541)	Be consistent about capitalization of "cwnd".
448 ↗	(On Diff #8541)	"time wait time"? Should that just be "wait time"? Be consistent about whether these entries are sentences ending with a period. To match the rest, this can be ...seconds, 0 to disable
450 ↗	(On Diff #8541)	Probably needs s/nat/NAT/
452 ↗	(On Diff #8541)	Be consistent about whether these entries are sentences ending with a period. Probably does not need the dash before "bit": Enable sending of the SACK-IMMEDIATELY bit
468 ↗	(On Diff #8541)	"retran'd" is questionable, so is "assoc". Please use full words when abbreviations are ambiguous.
472 ↗	(On Diff #8541)	Can "inside data" just be "in data"?
474 ↗	(On Diff #8541)	s/one-2-one/one-to-one/ The sentence is also ambiguous. Is it a value for the limit? Is it a Boolean about whether the abort is enabled?
476 ↗	(On Diff #8541)	Should Heartbeat be capitalized here? Other terms are not, mostly.
478 ↗	(On Diff #8541)	Possibly Whether SCTP should respond to drain calls
496 ↗	(On Diff #8541)	Please clarify.
504 ↗	(On Diff #8541)	s/retransmission/retransmissions/
508 ↗	(On Diff #8541)	Maybe this should be s/ms/mS/ for milliseconds. That would also be needed elsewhere. But maybe "ms" is the standard for this page.
512 ↗	(On Diff #8541)	Is this really just the default value, or the current value? A lot of these would need to be changed if these are current values.
522 ↗	(On Diff #8541)	Heartbeat was capitalized before. They should be consistent, whether capitalized or not.
532 ↗	(On Diff #8541)	Why capitalize "Scaling"?
540 ↗	(On Diff #8541)	s/asoc/association/
542 ↗	(On Diff #8541)	Please expand "fr".
544 ↗	(On Diff #8541)	Be consistent about SCTP capitalization.
546 ↗	(On Diff #8541)	Should "rwnd" be capitalized?

I'm sorry about not including context; will do that with next revision.

I've been digging through source code and RFC's the past 8 hours or so to figure out what each MIB actually does. Will keep on doing this until I've documented most if not all the MIBs, then update revision.

I think it's outside the scope of this patch to change MIB names, as this would require non-superficial code changes, and possibly configuration changes for affected parties. Please let me know if you disagree. :)

share/man/man4/sctp.4
430 ↗	(On Diff #8541)	New line: Will do. I'll consistently use proper grammar on these, to stay in line with how it's done in tcp(4)
436 ↗	(On Diff #8541)	I had to dive deep into the SCTP implementation, but managed to figure out what this actually does. Will be completely reworded with next revision.
443 ↗	(On Diff #8541)	Will attempt to be consistent. I won't be changing the MIB names, but will be describing it with 'congestion window' instead of 'cwnd'/'CWND'.
450 ↗	(On Diff #8541)	Should probably also be 'setting' not 'sending'. Will dive into source to figure it out.
508 ↗	(On Diff #8541)	The international standard of units states "ms" (lower case) is correct for milliseconds. Is it abbreviated differently elsewhere?

I'm submitting this revision to have the current state of the work on this in the review. I've researched and documented more MIBs, grouped under the 'Congestion Control' sub-header. The MIBs listed under the Misc sub-header have not been updated yet. MIB names will be formatted with underline to match the TCP(4) man page. MIBs will gain the underline formatting as their descriptions are updated or considered OK.

I'll be working more on this soon.

Herald edited edge metadata. · View Herald TranscriptFeb 18 2016, 10:03 PM

marieheleneka_gmail.com added a reviewer: wblock.Feb 19 2016, 7:14 PM

I've not cycled back to this diff, and probably won't get to do the research necessary to document the remaining MIBs. Can this revision be committed as-is?

Adding Tuexen for review and (hopefully) commit.

In D3583#368663, @bcr wrote:

Adding Tuexen for review and (hopefully) commit.

I can review it. I think we can always improve it over time...

Clarification question: Are you asking me to commit it also or do you "just" want me to review it.

@tuexen: Ideally you'd do both review and commit, since I don't have a src commit bit. It might fall under the blanket commit rule of "comment change only" for doc committers in src. Having the SCTP maintainer commit it would give it more credibility I think. I can't really comment on whether these are correct, aside from grammar and typos.

@bcr Thanks for the clarification. I'll review and commit (assuming an approval from re@) to get it in 12.0. I prefer commits SCTP source code changes done by myself, since they have to be upstreamed, and that is easier to get things in upstream and then commit it into FreeBSD.
Once 12.0 is out, I might improve the descriptions which then is simple, since I only have to change text. Once done with that, I'll out it up for review and will add you as a reviewer. But this is post 12.0.

@tuexen: Sounds good to me. Marie-Helene did all the work on this.
Once you have a review post 12.0, I'd be happy to help out.

This revision was not accepted when it landed; it landed in state Needs Review.Oct 26 2018, 9:04 PM

Closed by commit rS339791: Add initial descriptions for SCTP related MIB variable. (authored by tuexen). · Explain Why

This revision was automatically updated to reflect the committed changes.