Page MenuHomeFreeBSD

Enable RACCT/RCTL in GENERIC.
ClosedPublic

Authored by trasz on Apr 30 2015, 12:57 PM.
Tags
None
Referenced Files
Unknown Object (File)
Thu, Mar 14, 12:34 AM
Unknown Object (File)
Jan 23 2024, 1:26 AM
Unknown Object (File)
Jan 14 2024, 1:09 AM
Unknown Object (File)
Jan 8 2024, 10:08 PM
Unknown Object (File)
Jan 8 2024, 10:08 PM
Unknown Object (File)
Jan 8 2024, 10:08 PM
Unknown Object (File)
Jan 8 2024, 10:08 PM
Unknown Object (File)
Jan 8 2024, 10:08 PM

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

trasz retitled this revision from to Enable RACCT/RCTL in GENERIC..
trasz updated this object.
trasz edited the test plan for this revision. (Show Details)
sys/amd64/conf/GENERIC
77 ↗(On Diff #5100)

I find this name (and negative options in general) rather confusing.

Maybe RACCT_DEFAULT_DISABLE is a bit more clear if we don't want to flip the sense.

sys/amd64/conf/GENERIC
77 ↗(On Diff #5100)

The idea is to make upgrade seamless - this way, people who already have RACCT and RCTL in their config will have it working as before, without needing to tweak the config or the tunable.

sys/amd64/conf/GENERIC
77 ↗(On Diff #5100)

Yes, that makes sense and is why I'm not strongly in favour of changing the sense. But I do think that indicating the option applies specifically to the default is valuable.

rctl(8) needs updated. It references the kernel config options required and not yet the sysctl either.

sys/amd64/conf/GENERIC
77 ↗(On Diff #5100)

I've got mixed feelings about this. RACCT_DEFAULT_DISABLE looks not very grammatical, RACCT_DEFAULT_TO_DISABLE (like IPFW_DEFAULT_TO_DISABLE) sounds ok, but it seems too long and moves the comment to the right. And the fact that the option is the default is documented in the comment, which would become completely redundant.

sys/amd64/conf/GENERIC
77 ↗(On Diff #5100)

Consider IPFIREWALL_DEFAULT_TO_ACCEPT and PF_DEFAULT_TO_DROP for example, which set defaults for settings that may be changed later by the user, and SC_DISABLE_REBOOT and SC_DISABLE_KDBKEY, which compile out the corresponding capability.

Use better define name.

Erm, actually do update the manual page.

wblock added inline comments.
usr.bin/rctl/rctl.8
237 ↗(On Diff #5187)

Either add a "the" ("can be set in the loader prompt") or remove "prompt" ("can be set in loader(8) or...").

LGTM except one nit in the manpage. Also should likely have an UPDATING entry (for both the option name change and feature itself), and don't forget the Relnotes: yes when committing.

usr.bin/rctl/rctl.8
237 ↗(On Diff #5196)

shouldn't it be "can be set at the loader prompt"?

usr.bin/rctl/rctl.8
237 ↗(On Diff #5196)

That is probably better, yes: "at the loader prompt" or "in the loader".

emaste edited edge metadata.

Looks good with either one of @wblock's suggestions incorporated.

This revision is now accepted and ready to land.May 7 2015, 2:50 PM
trasz edited edge metadata.

One more grammar fix.

This revision now requires review to proceed.May 10 2015, 8:11 AM

Do we actually need the UPDATING entry? The old name (RACCT_DISABLED) was just added two weeks ago, and not documented either.

I like this, but have one comment about the man page.

usr.bin/rctl/rctl.8
247 ↗(On Diff #5312)

Seems confusing to me to say this without noting that it is set that way in GENERIC. Would it be OK to add something along the lines of "The GENERIC kernel config sets this.", with the assumption that the man page would be updated if the config changes in GENERIC? Or is it expected that the user reads the GENERIC config file?

In D2407#46078, @trasz wrote:

Do we actually need the UPDATING entry? The old name (RACCT_DISABLED) was just added two weeks ago, and not documented either.

I was thinking primarily of advertising that the feature is now available in GENERIC; perhaps the release notes are sufficient.

usr.bin/rctl/rctl.8
247 ↗(On Diff #5312)

Assuming the man page will be updated when the config file defaults change is not safe. The person who changes the config file might forget, or more likely, not even know that a man page needs to be updated.

247 ↗(On Diff #5312)

Passive to active:
s/was set/is set/

usr.bin/rctl/rctl.8
247 ↗(On Diff #5312)

Note that rctl utility makes it very obvious - through the error message - whether the user needs to tweak the tunable, or rebuild the kernel. I'm not sure if we need to document any better what's available in GENERIC; the correct course of action is described in rctl(8) output.

trasz edited edge metadata.

s/was set/is set/

Ok, last call for you folks to mark this revision "Accepted", to get on the "Reviewed by:" list. :-)

wblock added a reviewer: wblock.

Man page looks good to me.

This revision is now accepted and ready to land.May 13 2015, 1:55 PM
emaste edited edge metadata.
This revision was automatically updated to reflect the committed changes.