[apparmor] [RFC v2] "features" directory for version/capability information

[Kees Cook]

Hi Seth,

On Fri, Dec 30, 2011 at 06:44:16PM -0800, Seth Arnold wrote:
> On Thu, Dec 29, 2011 at 4:46 PM, Kees Cook <kees at ubuntu.com> wrote:
> > +#include <linux/capability.h>
> 
> Why is this header now required? I didn't spot it in the code.

Heh, yeah, I have no idea. I was wondering that myself while reviewing the
code today. ;)

> > +#define AA_FS_U64_FILE(name, value) \
> > +       { name, \
> > +         .v_type = AA_FS_U64, .v.u64 = value, \
> > +         &aa_fs_seq_file_ops }
> 
> I would rather the name and value have () around them in the expansion:

Good idea, yeah. I'll do that.

> > +#undef AA_FS_STRING_FILE
> 
> Only AA_FS_STRING_FILE is undefed here -- why not also include
> AA_FS_U64_FILE and AA_FS_BOOLEAN_FILE?

It was my intention to undef them all; I'll fix this.

> > -static void __init aafs_remove(const char *name)
> > ...
> > +static void __init aafs_remove(const char *name, struct dentry *dir_dentry)
> >  {
> >        struct dentry *dentry;
> >
> > -       dentry = lookup_one_len(name, aa_fs_dentry, strlen(name));
> > +       if (!dir_dentry)
> > +               return;
> > +
> > +       dentry = lookup_one_len(name, dir_dentry, strlen(name));
> 
> This looks like a bugfix; should that be rolled in separately?

It's actually not, but "diff" made this hard to review by separating the
old function declaration. I added the dir_dentry argument (it was
aa_fs_dentry always before), so it was part of the refactor.

I've actually just finished a version that includes the rlimit directory,
and the patch has gotten large enough now that I'm going to break up into a
few pieces to make it easier to review.

> Thanks Kees!

You bet! Thanks for the review. :) I'll have another set up soon.

-Kees

-- 
Kees Cook