Page MenuHomeFreeBSD

rc.subr.8: document argument_cmd override pitfalls
Needs ReviewPublic

Authored by a.wolk_fudosecurity.com on May 18 2021, 3:16 PM.
Referenced Files
Unknown Object (File)
Fri, Oct 18, 12:42 AM
Unknown Object (File)
Fri, Oct 18, 12:34 AM
Unknown Object (File)
Oct 4 2024, 11:49 PM
Unknown Object (File)
Oct 1 2024, 4:14 PM
Unknown Object (File)
Sep 30 2024, 6:11 PM
Unknown Object (File)
Sep 30 2024, 2:41 PM
Unknown Object (File)
Sep 24 2024, 11:05 AM
Unknown Object (File)
Sep 20 2024, 8:35 PM

Details

Reviewers
debdrup
Summary

Document argument_cmd override pitfall as discussed in https://reviews.freebsd.org/D30330

Suggested commit message:

commit a00b20aae5db0d79e26e6926c6de5b5c13a580fd (HEAD -> main)
Author: Adam Wolk <a.wolk@fudosecurity.com>
Date:   Tue May 18 17:06:38 2021 +0200

    rc.subr.8: document argument_cmd override pitfalls
    
    Overriding argument_cmd may lead to some functionality silently not being
    triggered. This includes _oomprotect, _nice and several other mechanisms.
    
    One example of a port that overrides start_cmd is PostgreSQL. This port is also
    a natural target for _oomprotect="ALL" which would turn into a no-op and being
    surprising for users.
    
    Sponsored by:   Fudo Security

Diff Detail

Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

Mixed up diffs between the reviews. Sorry for the noise.

The current diff you've uploaded lacks context for Phabricator to tell which part of the file it applies to (I don't know why Phabricator can't always figure out the context), which is why the "Context not available" message appears.

Can I ask you to either use the method described here. or git-arc.sh from src/tools/tools/git/ (via make -C /usr/src/tools/tools/git install) to upload diffs. The latter has a few requirements from ports/packages - though it's likely you already have them.

This lets anyone make in-line comments and suggestions for changing the text, which I need because I have a bit of wordsmithing I'd like to suggest. :)

This also applies to D30334.

Updated the raw diff using the git show -U999999 <commit-hash> > change.diff method. Hope that helps! :)

Updated the raw diff using the git show -U999999 <commit-hash> > change.diff method. Hope that helps! :)

Much better, thank you. :)

share/man/man8/rc.subr.8
782–795 ↗(On Diff #89431)

I think this wording is better, but I suggest we run it by some more people, just in case.

Updated the diff to your suggestion, which also fixes the mandoc/igor lint.

I think this wording is better, but I suggest we run it by some more people, just in case.

I'm OK with waiting :)

Picked the wrong file after regenerating. Sorry, this one has your change.

Language LGTM. Whether what this says is correct is beyond my ken.

I ended up documenting this in bd6dce978c1, I believe - can you confirm?