[apparmor] [patch 01/26] Convert mount and dbus to be subclasses of a generic rule class

[John Johansen]

On 03/31/2014 03:59 PM, Steve Beattie wrote:
> On Thu, Mar 27, 2014 at 08:45:14AM -0700, john.johansen at canonical.com wrote:
>> This will simplify add new features as most of the code can reside in
>> its own class. There are still things to improve but its a start.
>>
>> Signed-off-by: John Johansen <john.johansen at canonical.com>
> 
> Alright, I think I've got enough of a handle on this patch (although
> I'll admit I glazed over a bit on the mount processing move to
> the mount_rule class) to go ahead and say
> Acked-by: Steve Beattie <steve at nxnw.org>, with some nits to follow:
> 
thanks, I think I detect seth and tyler are silently thanking you too

<< snip >>

> A stylistic nit: presumably, dbus.c, mount.c and rule.c should get
> renamed to .cc?
> 
heh we could, I don't really care whether it is .c or .cc but it should
be consistent

>> -void free_dbus_entry(struct dbus_entry *ent)
>> +#define _(s) gettext(s)
> 
> Nit: we really ought to move this define to a common header.
> 

yeah, that can be separate patch though

<< snip >>

>> -	list_for_each(entry_list, entry) {
>> -		error = expand_entry_variables(&entry->mnt_point, entry);
>> +	for(RuleList::iterator i = prof.rule_ents.begin(); i != prof.rule_ents.end(); i++) {
> 
> Another nit, can you put a space between the for and the open paren?
> 
yep

<< snip >>

>> +%code requires {
>> +	#include "profile.h"
>> +	#include "mount.h"
>> +	#include "dbus.h"
>> +}
>> +
> 
> Interesting, I'd not seen the %code stuff before.
> 
me neither until I started hit build error because of it, and had to go
digging to figure out. I'm still not sure why having just profile.h
without the requires works but add mount.h or dbus.h just broke things

<<snip>>

>> +
>> +class rule_t {
>> +public:
>> +	int aa_class;
>> +
>> +	rule_t(void): aa_class(AA_CLASS_UNKNOWN) { };
> 
> Another nit: aa_class is defined for the rule_t abstract class as well
> as for the mnt_rule class, but not for the dbus_rule class, nor is it
> used anywhere. So I'm guessing it's something that you initially thought
> you needed, but turned out not to. Can you kill it if that's the case?
> (It can be a separate patch.)
> 
right, I was using it initially but it got dropped in revising. It may
yet show up again but it should have been yanked out of here