Page MenuHomeFreeBSD

inet.4: Document new experimental sysctls
ClosedPublic

Authored by karels on Jul 10 2022, 4:31 PM.
Tags
None
Referenced Files
Unknown Object (File)
Dec 24 2022, 9:22 PM
Unknown Object (File)
Dec 15 2022, 7:34 AM

Details

Reviewers
pauamma
rgrimes
Group Reviewers
manpages
Summary

Add descriptions of net.inet.ip.allow_net0, net.inet.ip.allow_net240,
and net.inet.ip.loopback_mask sysctls.

These changes are planned to be committed after other missing changes
in inet.4

Test Plan

tested with mandoc

Diff Detail

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

Event Timeline

My thoughts are currently to do the following in inet.4, probably as separate reviews:

  1. Change the oldest entries to use sysctl names rather than IPCTL_*; no one knows what the IPCTL values mean now. The sysctl names are in parens now. Probably also remove the leading "ip." from each sysctl name and change the heading instead.
  2. Sort the entries alphabetically. I think these are in historical order now, which is not very useful.
  3. Add existing undocumented sysctls worthy of documentation.
  4. This review (updated, of course).

Comments on the plan? I'll note that tcp.4, udp.4, and icmp.4 have the same format as inet.4, so I'd plan to tackle those next.

I have strong opinion that documenting changes should go in the same commit as the functional part, thus this change must be committed in a single commit with D35741. The reason for that is working with the repo in the future. We may want to rename the functionality once it becomes official RFC, or we might want to delete it if the draft is abandoned, or other options are possible. When functionality+documentation are committed in a single commit, it easy to reach it no matter where are you coming from - from documentation, or from the source. If they are separate, there is no way to link commits together, except, well, reverting and recommitting again.

Example 1: a user who has no knowledge of our kernel sources (e.g. coming from Linux) reads about support for zero net in man 4 inet. They want to check the implementation. The hard way is to grep the sources, and remember that full sysctl name is not in the sources, so straightforward grep would fail. So they need to grep for pieces of the sysctl OID or try to guess other keywords. In other words - welcome back to the RCS/CVS times. With git however, you can git blame on the documentation and find out the commit that created documentation and the code!

Example 2: the draft changes status to RFC and somebody comes to update this fact. Could be yourself, but after years you forgot all places that need to be touched by that change. Again, single commit will allow to find it easily.

I don't understand why you think that your additions can be committed only after improving inet.4 wrt the other missing sysctls! Why? Just add your sysctls alphabetically sorted and they won't conflict with any future improvements of inet.4

Comments on the plan? I'll note that tcp.4, udp.4, and icmp.4 have the same format as inet.4, so I'd plan to tackle those next.

Sounds great! Thanks!

I think if you add the new sysctls to the top of the list (as they start with allow) they would be alphabetically sorted and no future reformatting and improvement of the page would conflict with them. After any improvements, as long as you don't fully rewrite the description text, git blame would always point at your commit that brought the new text.

I don't understand why you think that your additions can be committed only after improving inet.4 wrt the other missing sysctls! Why? Just add your sysctls alphabetically sorted and they won't conflict with any future improvements of inet.4

They will conflict, in part because I plan to remove the leading "ip." from all the sysctl names. Essentially all of the entries will be reordered; I'm doubting that git would be smart enough to follow along.

However, I accept your point about grouping related changes together. If manpages does not object, I'll move this change into the other review.

As you remove "ip." prefix, only the .It Va ip.foo line would change, not the rest of the text. The paragraph would still be blamed to the original commit.

pauamma added a subscriber: pauamma.

English LGTM. Can't speak to consistency with source.

This revision is now accepted and ready to land.Jul 11 2022, 3:03 AM

Pau Amma, any thoughts on the overall plan in the first comment?

Also, any preference whether similar changes to multiple man pages are grouped into the same review (e.g. removing the old *CTL* labels)?

Pau Amma, any thoughts on the overall plan in the first comment?

As outlined, looks reasonable to me, whether in the implied order or not.

Also, any preference whether similar changes to multiple man pages are grouped into the same review (e.g. removing the old *CTL* labels)?

I think that for mechanical changes like this, grouping them in a single commit (or a few if one would get too long) with no other changes is the norm, but I don't have a personal preference.

Thanks, Pau Amma.

Added this change to D35741 as requested; added one sentence compared to this version.

rgrimes added a subscriber: rgrimes.

Agree that this should be committed at the same time as the functional code changes.

Committed with the code changes in efe58855f3ea; closing review.