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

[John Johansen]

On 12/30/2011 06:58 PM, Kees Cook wrote:
> 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. ;)
>
It for the VFS_CAP_FLAGS_MASK define


>>> +#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
>