[apparmor] [patch 4/3] hide raw_rule within parse class method

[Steve Beattie]

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 <steve at nxnw.org>
> 
> > 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 <apparmor at cboltz.de>

Thanks. All patches in the series are committed to trunk now.

-- 
Steve Beattie
<sbeattie at ubuntu.com>
http://NxNW.org/~steve/
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <https://lists.ubuntu.com/archives/apparmor/attachments/20141216/83ed5099/attachment.pgp>