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 -- Linux-audit mailing list [email protected] https://www.redhat.com/mailman/listinfo/linux-audit
