Re: [RFC PATCH ghak86 V1] audit: use audit_enabled as a boolean where convenient

From: Paul Moore <paul@paul-moore.com>
To: Richard Guy Briggs <rgb@redhat.com>
Cc: Linux-Audit Mailing List <linux-audit@redhat.com>,
	LKML <linux-kernel@vger.kernel.org>,
	Eric Paris <eparis@redhat.com>, Steve Grubb <sgrubb@redhat.com>
Subject: Re: [RFC PATCH ghak86 V1] audit: use audit_enabled as a boolean where convenient
Date: Fri, 1 Jun 2018 18:15:49 -0400	[thread overview]
Message-ID: <CAHC9VhTABDT0FKb=vY4G06aY9Ky+NctArCRo8-iUQF6=eWKy1g@mail.gmail.com> (raw)
In-Reply-To: <20180531163811.7m7c3q75duqkicl4@madcap2.tricolour.ca>

On Thu, May 31, 2018 at 12:38 PM, Richard Guy Briggs <rgb@redhat.com> wrote:
> On 2018-05-31 11:48, Paul Moore wrote:
>> On Thu, May 31, 2018 at 11:13 AM, Richard Guy Briggs <rgb@redhat.com> wrote:
>> > Most uses of audit_enabled don't care about the distinction between
>> > AUDIT_ON and AUDIT_LOCKED, so using audit_enabled as a boolean makes
>> > more sense and is easier to read. Most uses of audit_enabled treat it as
>> > a boolean, so switch the remaining AUDIT_OFF usage to simply use
>> > audit_enabled as a boolean where applicable.
>> >
>> > See: https://github.com/linux-audit/audit-kernel/issues/86
>> >
>> > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
>> > ---
>> >  drivers/tty/tty_audit.c      | 2 +-
>> >  include/net/xfrm.h           | 2 +-
>> >  kernel/audit.c               | 8 ++++----
>> >  net/netfilter/xt_AUDIT.c     | 2 +-
>> >  net/netlabel/netlabel_user.c | 2 +-
>> >  5 files changed, 8 insertions(+), 8 deletions(-)
>>
>> I'm not sure I like this idea.  Yes, technically this change is
>> functionally equivalent but I worry that this will increase the chance
>> that non-audit folks will mistake audit_enabled as a true/false value
>> when it is not.  It might work now, but I worry about some subtle
>> problem in the future.
>
> Would you prefer a patch to change the majority (18) of uses of
> audit_enabled to be of the form "audit_enabled == AUDIT_OFF" (or
> "audit_enabled != AUDIT_OFF")?
>
> I prefer the approach in this patch because it makes the code smaller
> and significantly easier to read, but either way, I'd like all uses to
> be consistent so that it is easier to read all the code similarly.
>
>> If you are bothered by the comparison to 0 (magic numbers), you could
>> move the AUDIT_OFF/AUDIT_ON/AUDIT_LOCKED definitions into
>> include/linux/audit.h and convert the "audit_enabled == 0" to
>> "audit_enabled == AUDIT_OFF".
>
> I'd be fine doing that if you really dislike this patch's approach.

Like I said, I'm don't really care for the boolean-like approach of
this first patch.

-- 
paul moore
www.paul-moore.com