Date: Wed, 28 Nov 2012 08:13:22 +1100 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Pawel Jakub Dawidek <pjd@FreeBSD.org> Cc: svn-src-head@FreeBSD.org, svn-src-all@FreeBSD.org, src-committers@FreeBSD.org Subject: Re: svn commit: r243613 - head/sys/kern Message-ID: <20121128072724.A1049@besplex.bde.org> In-Reply-To: <201211271038.qARAcBeX044425@svn.freebsd.org> References: <201211271038.qARAcBeX044425@svn.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Tue, 27 Nov 2012, Pawel Jakub Dawidek wrote:
> Log:
> Add kern.capmode_coredump sysctl/tunable to allow processes in capability mode
> to dump core.
You missed fixing some style bugs before touching this.
> Modified: head/sys/kern/kern_sig.c
> ==============================================================================
> --- head/sys/kern/kern_sig.c Tue Nov 27 10:32:35 2012 (r243612)
> +++ head/sys/kern/kern_sig.c Tue Nov 27 10:38:11 2012 (r243613)
> @@ -175,6 +175,11 @@ TUNABLE_INT("kern.sugid_coredump", &sugi
> SYSCTL_INT(_kern, OID_AUTO, sugid_coredump, CTLFLAG_RW,
> &sugid_coredump, 0, "Allow setuid and setgid processes to dump core");
Normal indentation. Abnormal description of a flag.
>
> +static int capmode_coredump;
> +TUNABLE_INT("kern.capmode_coredump", &capmode_coredump);
> +SYSCTL_INT(_kern, OID_AUTO, capmode_coredump, CTLFLAG_RW,
> + &capmode_coredump, 0, "Allow processes in capability mode to dump core");
> +
Normal indentation. Abnormal description of a flag.
> static int do_coredump = 1;
> SYSCTL_INT(_kern, OID_AUTO, coredump, CTLFLAG_RW,
> &do_coredump, 0, "Enable/Disable coredumps");
Abnormal indentation. Normal description of a flag as Enable/Disable (but
better if it leaves out "Disable"). I'm not sure if the new flag is
sufficiently different to deserve an "Allow" in it.
The first one certainly doesn't. It is just another flag that gates
the main one; when it is set, set*id processes aren't "Allowed" to
dump core, but are forced to. It used to be described more or less
correctly as "Enable coredumping set user/group ID processes", but
this has been broken. The description was verbose and had bad grammar.
Now it is verbose and has correct grammar, but is not obviously just
a flag. My version doesn't fix this, but fixes the indentation and
the order of the declarations (the basic do_coredump flag should be
first, since it is first alphabetically, historically (?) and
logically).
The 2 newer flag also have a namespace error (they don't have a do_
prefix like the others). Among other bugs, this makes it sort
incorrectly with the others. My version adds a do_ prefix to
the second-newest one.
Suggested rewordings for the old descriptions, and other style fixes
(indentation, namespace):
&do_coredump, 0, "Enable core dumps");
Statistics: on freefall now, sysctl -nda | grep -i ... shows 115 lines
matching "enable", 38 matching "disable", and only 11 matching both;
zgrep -ir on /usr/share/man shows 338 lines matching "core dump" and
168 matching "coredump", with most of the compressed spellings being
for proper names like the coredumpsize rlimit and the wait flag
WCOREDUMP.
&do_sugid_coredump, 0, "Enable core dumps for set*id processes");
Now I don't like the "Enable" wording either. This flag is in addition
to the other one, and neither "Enable" nor "Allow" fully describes the
logic. A full verbose description is "Enable core dumps for set*id
processes (if core dumps are not disabled for all processes)".
> @@ -3134,12 +3139,17 @@ nomem:
> int error, n;
> int flags = O_CREAT | O_EXCL | FWRITE | O_NOFOLLOW;
> int cmode = S_IRUSR | S_IWUSR | S_IRGRP | S_IWGRP;
Old style bugs:
- nested declarations
- unsorted declarations
- int variables not all in 1 declaration
- initialization in declarations.
> + int oflags = 0;
> +
> + if (capmode_coredump)
> + oflags = VN_OPEN_NOCAPCHECK;
New style bugs:
- extend the previous bugs. Half of the initialization of oflags is in
its declaration and half in 2 new lines.
- extra blank line
- other verboseness. The initialization of oflags can be written more
concisely and clearly as
oflags = capmode_coredump ? VN_OPEN_NOCAPCHECK : 0;
This could be squeezed into its declaration line, but shouldn't be.
Unless you make it slighly clearer by declaring oflags as const --
then its initialization must be in its declaration. This often
applies for variables that hold translations of flags from one set
to another. But I wouldn't go that far.
Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20121128072724.A1049>