Re: [PATCH 1/7] audit: implement generic feature setting and retrieving
On Mon, 2013-07-08 at 16:28 -0400, Steve Grubb wrote:
On Friday, May 24, 2013 12:11:44 PM Eric Paris wrote:
> The audit_status structure was not designed with extensibility in mind.
> Define a new AUDIT_SET_FEATURE message type which takes a new structure
> of bits where things can be enabled/disabled/locked one at a time.
This changes how we have been doing things. The way that the audit system
settings have been done is to use the AUDIT_SET and AUDIT_GET commands. It
takes a bit map as the function to perform. We have only used 5 of the 32
bits.
Do we really need another of the same thing?
It's not the same thing. This is an interface designed for options
which have 4 states. On/Off and Locked/Unlocked. It is certainly the
right solution for that problem if we want to solve it generically.
(look at what it did to the other code who wanted an on/off option)
AUDIT_SET/GET was designed around setting a kernel variable to a single
value. It does an ok job at this (although I'd argue that there could
be a better design here as well, but we have this, so we live with it.)
It certainly does not form naturally to the 4 states of the new
interface.
I can certainly shoehorn a 4 state interface into AUDIT_SET/GET. But I
believe it will be less obvious and less efficient code. Maybe that
doesn't matter too much since only you have to deal with it and it isn't
run particularly often but I don't see a reason to solve a problem
inefficiently. No matter what we are going to have a new design
pattern, so why not a natural pattern rather than one we are forcing to
needlessly conform?
Let's say we do want to force ourselves to use the AUDIT_SET/GET netlink
message. What do you think the interface should look like?
I can just do the exact same thing inside AUDIT_SET/GET where I
implement a usable bitmask of on/off/lockable options. It'd be
basically the exact same thing as I have now, just more multiplexing
over the GET/SET message. Certainly no better.
I could add two new audit status bits, AUDIT_STATUS_LOGINUID_IMMUTABLE
and AUDIT_STATUS_LOGINUID_IMMUTABLE_LOCKED, along with two __u8's to
struct audit_status. One __u8 could be loginuid_immutable and the other
loginuid_immutable_locked. I guess that keeps us in line with the
GET/SET mentality. I don't see a benefit, but I am biased towards my
own code.
I could also do it as one __u8 and have magic meaning to the bits inside
the __u8, aka,
0 == off unlocked
1 == on unlocked
2 == off locked
3 == on locked
but that seems to needlessly make the code more indecipherable to
humans. So I won't do it (I see audit_panic and audit_enabled as
examples of this poor design pattern)
What do you think it should look like and why is it better than mine
which has proven quite nice for multiple people?
-Eric