From owner-svn-src-head@FreeBSD.ORG Tue Nov 27 21:21:03 2012 Return-Path: Delivered-To: svn-src-head@FreeBSD.org Received: from mx1.freebsd.org (mx1.freebsd.org [69.147.83.52]) by hub.freebsd.org (Postfix) with ESMTP id 4432F6AA; Tue, 27 Nov 2012 21:21:03 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from fallbackmx07.syd.optusnet.com.au (fallbackmx07.syd.optusnet.com.au [211.29.132.9]) by mx1.freebsd.org (Postfix) with ESMTP id BE6C58FC0C; Tue, 27 Nov 2012 21:21:02 +0000 (UTC) Received: from mail36.syd.optusnet.com.au (mail36.syd.optusnet.com.au [211.29.133.76]) by fallbackmx07.syd.optusnet.com.au (8.13.1/8.13.1) with ESMTP id qARLDeBn016488; Wed, 28 Nov 2012 08:13:40 +1100 Received: from c122-106-175-26.carlnfd1.nsw.optusnet.com.au (c122-106-175-26.carlnfd1.nsw.optusnet.com.au [122.106.175.26]) by mail36.syd.optusnet.com.au (8.13.1/8.13.1) with ESMTP id qARLDMdC027414 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Wed, 28 Nov 2012 08:13:32 +1100 Date: Wed, 28 Nov 2012 08:13:22 +1100 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: Pawel Jakub Dawidek Subject: Re: svn commit: r243613 - head/sys/kern In-Reply-To: <201211271038.qARAcBeX044425@svn.freebsd.org> Message-ID: <20121128072724.A1049@besplex.bde.org> References: <201211271038.qARAcBeX044425@svn.freebsd.org> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed X-Optus-Cloudmark-Score: 0 X-Optus-Cloudmark-Analysis: v=2.0 cv=fbv1UDsF c=1 sm=1 a=baJ7GeqPacgA:10 a=kj9zAlcOel0A:10 a=PO7r1zJSAAAA:8 a=JzwRw_2MAAAA:8 a=M4roAWbnUW4A:10 a=DSn26mRKnKe-ZutKH90A:9 a=CjuIK1q_8ugA:10 a=bxQHXO5Py4tHmhUgaywp5w==:117 Cc: svn-src-head@FreeBSD.org, svn-src-all@FreeBSD.org, src-committers@FreeBSD.org X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 27 Nov 2012 21:21:03 -0000 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