Looking through the patch, these are my thoughts:

I like that my "splitlog" patch shrunk a lot, way easier. I like that. It also proves something like this is the correct direction.

Shouldn't audit_set_feature() check the version number, granted it doesn't mater now, but shouldn't their be a:

 if(uaf->version <= AUDIT_FEATURE_SUPPORTED_VERSION)

This branchy code could be:

if (new_feature)

af.features |= feature;

else

af.features &= ~feature;

changed to:

af.features = (af.features & ~feature) | feature

Ie just do a clear bit than or in what you want.

Otherwise LGTM

Bill

On Fri, May 24, 2013 at 9:28 AM, Eric Paris <eparis@redhat.com> wrote:

On Fri, 2013-05-24 at 12:11 -0400, 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
> structure should be able to grow in the future while maintaining forward
> and backward compatibility (based loosly on the ideas from capabilities
> and prctl)
>
> This does not actually add any features, but is just infrastructure to
> allow new on/off types of audit system features.
>
> Signed-off-by: Eric Paris <eparis@redhat.com>

Attached you will find the test program I used to check that things were
working correctly.  It should give an idea to Steve how we can program
the features support in userspace.  I believe it fits very nicely to
have a new syntax in audit.rules to set (and lock if needed/wanted)
these features.

netlink.c is just some helper code I stole from the audit tree to get
some functions which weren't exposed externally.  The only part really
interesting is test.c.

You will also need the include/uapi/linux/audit.h file from this patch
to build test.c

-Eric

--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit

--
Respectfully,

William C Roberts