[apparmor] [patch] rule class - split out common parts from is_covered()

[Steve Beattie]

On Thu, Jan 15, 2015 at 01:58:58PM +0100, Christian Boltz wrote:
> Am Dienstag, 13. Januar 2015 schrieb Steve Beattie:
> > On Fri, Dec 19, 2014 at 12:24:02AM +0100, Christian Boltz wrote:
> > > this patch splits is_covered() in capability.py into
> > > - is_covered_localparts() for rule-specific code
> > > - is_covered() for common code - located in __init__.py
> > 
> > I'm not keen on the naming convention xxxxx_localvars() that the code
> > actually uses, the xxxxx_localparts() listed above is makes me cringe
> > slightly less.
> 
> Those (internal) function names are just technical details - I can 
> easily change that to ..._localparts().

Yeah, I know. Anyway, I've voiced a slight preference, but don't have a
strong opinion, so I'll leave it up to you.

> > > +        if check_audit and other_rule.audit != self.audit:
> > > +            return False
> > > +
> > > +        if other_rule.audit and not self.audit:
> > > +            return False
> > 
> > I recognize that you are merely pushing these checks around, but do
> > you recall why we do the second audit test after the first? I don't
> > quite get the reasoning...
> 
> if check_audit and other_rule.audit != self.audit:
>     exact check if audit flags are equal, so you get exactly the same 
>     rule and exactly the same logging. Only checked if the check_audit
>     flag is given.
> 
> if other_rule.audit and not self.audit:
>     released check - only hits if other_rule has audit flag set to make 
>     sure we get audit entries in the log - but also "says" things like
>     "foo is covered by audit foo".

Ah right. Thanks.

> > > +
> > > +        # still here? -> then the common part is covered, check
> > > rule-specific things now +        return
> > > self.is_covered_localvars(other_rule, check_allow_deny,
> > > check_audit) +
> > > +    # @abstractmethod  FIXME - uncomment when python3 only
> > 
> > > +    def is_covered_localvars(self, other_rule, 
> check_allow_deny=True, check_audit=False):
> > If check_allow_deny and check_audit are verified in the BaseRule
> > method, is there a reason to pass them on to the subclass version of
> > the method, since they should be irrelevant to them?
> 
> Good question, in theory they are really superfluous.
> 
> My reason for handing them over is to have the same set of parameters in 
> the parent and child function.

I think I prefer to keep the division of responsibilities explicit, by
dropping the arguments for the "child" method. If the subclass really
needs to examine allow/deny/audit to determine coverage, then I either
we can revisit this decision to pass the arguments along or the subclass
can override the is_covered() method.

With that change, and your choice on the _localxxxx name,
Acked-by: Steve Beattie <steve at nxnw.org>. Thanks.

-- 
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/20150115/09b6b1d1/attachment.pgp>