Date:   Mon, 4 Jun 2018 19:57:17 -0400
From:   Paul Moore <paul@...l-moore.com>
To:     Richard Guy Briggs <rgb@...hat.com>
Cc:     Linux-Audit Mailing List <linux-audit@...hat.com>,
        LKML <linux-kernel@...r.kernel.org>,
        Eric Paris <eparis@...hat.com>, Steve Grubb <sgrubb@...hat.com>
Subject: Re: [RFC PATCH ghak86 V1] audit: use audit_enabled as a boolean where convenient

On Sat, Jun 2, 2018 at 1:53 PM, Richard Guy Briggs <rgb@...hat.com> wrote:
> On 2018-06-01 18:15, Paul Moore wrote:
>> On Thu, May 31, 2018 at 12:38 PM, Richard Guy Briggs <rgb@...hat.com> wrote:
>> > On 2018-05-31 11:48, Paul Moore wrote:
>> >> On Thu, May 31, 2018 at 11:13 AM, Richard Guy Briggs <rgb@...hat.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@...hat.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.
>
> That doesn't really address the original issue though.

To be honest, there really isn't an issue to begin with, at least not
in my mind.  Sure, I understand you think all non-audit users of
audit_enabled should treat audit_enabled as a boolean; at this point
in time, I don't think that is necessary or desirable.

> Without any elaboration, I am not able to guess why you don't like this
> or what possible future subtleties would cause a problem.

As I said previously: "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.".

> Can you explain the problem with "non-audit folks will mistake
> audit_enabled as a true/false value when it is not"?

See the "it might work but ..." part above.

> While I realize people change their opinions given a broader context,
> and the origninal reply was ambiguous, I went ahead with this patch
> based on your "Sounds good." from:
>         https://www.redhat.com/archives/linux-audit/2018-April/msg00089.html

I think the confusion comes from what was meant by "clean them all
up".  We obviously have different understandings of what "cleaning"
meant.

> Would you accept a patch that defines a function by the same name as the
> global variable that returns a boolean (and localizes and renames the
> existing global with a "__" prefix?

At this point I think I've been clear that I don't like treating it as
a boolean, regardless of if it is wrapped in a function or not.  Why?
Well, it's not a boolean for starters.

If you wanted to submit a patch that would swap out 0 for AUDIT_OFF I
would accept that.

> I'm not willing to offer a patch to make the existing boolean usage harder to read to bring it all into similar usage.

Okay ... ?  Patch submission has always been voluntary as far as I can
tell; if you aren't willing to submit a patch, that's fine.

-- 
paul moore
www.paul-moore.com

Powered by blists - more mailing lists