Hello, Am Montag, 8. Dezember 2014 schrieb Steve Beattie: > On Wed, Dec 03, 2014 at 10:39:13AM -0800, Steve Beattie wrote: > > > For readabiliy, it might be a good idea to keep parse_capability() > > > as a separate function inside the CapabilityRule class and call > > > it from __init__. > > > > So, I started down this path, but I really didn't like how > > convoluted > > it was making the logic in __init__; it required verifying that one > > and only one of cap_list and raw_rule were passed and splitting up > > the parse_capability function a bit at the very least. > > > > I came up with another solution, for which I've attached a patch. I > > moved the parse() entry point to the base class, which then calls > > the intended-for-internal-consumption-only class method _parse(). I > > removed the raw_rule argument to both __init__ methods and haver > > BaseRule.parse() set raw_rule directly. > > > > Thus, the two intended entry points are __init__(), where you're > > expected to have already dissected whatever rule into digestible > > components *or* the Rule.parse() class method, where only a raw_rule > > is given. In this way, using the obvious methods of interacting > > with the CapabilityRule class, you can't have a mismatch between > > the stored capabilities and the raw_rule unless there's a bug in the > > parsing function *or* you've gone digging into the internals of the > > object.
Not exactly what I had in mind, but I understand why you do it this way. > > And if you do go around getting grotty with the class fields like > > raw_rule directly, (1) it's not recommended, and (2) you are > > expected > > to know what you're doing (e.g. you're a test script). > > Christian, any comments on this? Yes ;-) - sorry for the delay! > 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. > 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() > @@ -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 With these details fixed, Acked-by: Christian Boltz <[email protected]> Regards, Christian Boltz -- Neue Windows-Tastatur seit heute zu kaufen. Hier ein erstes Bild: ,-----, ,-----, ,------, | Str | | Alt | | Entf | '-----' '-----' '------' -- AppArmor mailing list [email protected] Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor
