On Tue, Dec 09, 2014 at 05:20:39PM +0100, Christian Boltz wrote: > > I've updated the patch to add some comments to the base class about > > what rule subclasses need to implement. > > > > Signed-off-by: Steve Beattie <[email protected]> > > > Index: b/utils/apparmor/rule/__init__.py > > =================================================================== > > --- a/utils/apparmor/rule/__init__.py > > +++ b/utils/apparmor/rule/__init__.py > ... > > + # @abstractmethod FIXME - uncomment when python3 only > > + def is_covered(self, other_rule, check_allow_deny=True, > > check_audit=False): > > + '''check if other_rule is covered by this rule object''' > > + raise AppArmorBug("'%s' needs to > > implement is_covered(), but didn't" % (str(cls))) > > You'll get an exception here, but not the one you expect - cls is not > defined here ;-) > > > + # @abstractmethod FIXME - uncomment when python3 only > > + def is_equal_localvars(self, other_rule): > > + '''compare if rule-specific variables are equal''' > > + raise AppArmorBug("'%s' needs to implement > > is_equal_localvars(), but didn't" % (str(cls))) > > Same here - cls is undefined.
Gack, fixed.
> > Index: b/utils/apparmor/rule/capability.py
> > ===================================================================
> > --- a/utils/apparmor/rule/capability.py
> > +++ b/utils/apparmor/rule/capability.py
> ...
> > @@ -61,8 +60,8 @@ class CapabilityRule(BaseRule):
> > if len(cap.strip()) == 0:
> > raise AppArmorBug('Passed empty capability to
> > CapabilityRule: %s' % str(cap_list))
> >
> > - @staticmethod
> > - def parse(raw_rule):
> > + @classmethod
> > + def _parse(cls, raw_rule):
> > return parse_capability(raw_rule)
>
> parse_capability() is (IIRC) only called here - feel free to integrate
> it directly in _parse()
Yeah. I was of two minds as to whether to have it directly in the class
or not, which is why I had the code indented too deep in a prior
iteration of the patch. I've moved it into the class.
> > @@ -147,4 +144,4 @@ def parse_capability(raw_rule):
> >
> > return CapabilityRule(capability, audit=audit, deny=deny,
> > allow_keyword=allow_keyword,
> > - comment=comment, raw_rule=raw_rule)
> > + comment=comment)
>
> This change means you no longer set raw_rule. Please change this to
>
> rule = CapabilityRule(......)
> rule.raw_rule = raw_rule
> return rule
That gets set in the BaseRule.parse() class method. I had thought
you would prefer this coding style, such that subclasses wouldn't
need to bother setting the raw_rule.
> With these details fixed,
> Acked-by: Christian Boltz <[email protected]>
Thanks. All patches in the series are committed to trunk now.
--
Steve Beattie
<[email protected]>
http://NxNW.org/~steve/
signature.asc
Description: Digital signature
-- AppArmor mailing list [email protected] Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor
