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 <[email protected]>. Thanks. -- 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
