Hi, On Thu, Oct 29, 2015 at 6:09 PM, Christian Boltz <appar...@cboltz.de> wrote:
> Hello, > > as Kshitij mentioned, abstract methods should use NotImplementedError > instead of AppArmorBug. > > While changing this, I noticed that __repr__() needs to be robust against > NotImplementedError because get_raw() is not available in BaseRule. > Therefore the patch changes __repr__() to catch NotImplementedError. > > Of course the change to NotImplementedError also needs several > adjustments in the tests. > > Thanks for the short turn-around time with this patch and helping make things uniform and consistent. > > [ 19-baserule-use-NotImplementedError.diff ] > > === modified file ./utils/apparmor/rule/__init__.py > --- utils/apparmor/rule/__init__.py 2015-10-29 13:11:10.731878625 +0100 > +++ utils/apparmor/rule/__init__.py 2015-10-29 13:12:50.523041470 +0100 > @@ -52,7 +52,11 @@ > > def __repr__(self): > classname = self.__class__.__name__ > - return '<%s> ' % classname + self.get_raw() > + try: > + raw_content = self.get_raw() # will fail for BaseRule > + return '<%s> %s' % (classname, raw_content) > + except NotImplementedError: > + return '<%s (NotImplementedError - get_clean() not > implemented?)>' % classname > > @classmethod > def match(cls, raw_rule): > @@ -69,7 +73,7 @@ > @classmethod > def _match(cls, raw_rule): > '''parse raw_rule and return regex match object''' > - raise AppArmorBug("'%s' needs to implement _match(), but didn't" > % (str(cls))) > + raise NotImplementedError("'%s' needs to implement _match(), but > didn't" % (str(cls))) > > @classmethod > def parse(cls, raw_rule): > @@ -83,12 +87,12 @@ > def _parse(cls, raw_rule): > '''returns a Rule object created from parsing the raw rule. > required to be implemented by subclasses; raise exception if > not''' > - raise AppArmorBug("'%s' needs to implement _parse(), but didn't" > % (str(cls))) > + raise NotImplementedError("'%s' needs to implement _parse(), but > didn't" % (str(cls))) > > # @abstractmethod FIXME - uncomment when python3 only > def get_clean(self, depth=0): > '''return clean rule (with default formatting, and leading > whitespace as specified in the depth parameter)''' > - raise AppArmorBug("'%s' needs to implement get_clean(), but > didn't" % (str(self.__class__))) > + raise NotImplementedError("'%s' needs to implement get_clean(), > but didn't" % (str(self.__class__))) > > def get_raw(self, depth=0): > '''return raw rule (with original formatting, and leading > whitespace in the depth parameter)''' > @@ -121,7 +125,7 @@ > # @abstractmethod FIXME - uncomment when python3 only > def is_covered_localvars(self, other_rule): > '''check if the rule-specific parts of other_rule is covered by > this rule object''' > - raise AppArmorBug("'%s' needs to implement > is_covered_localvars(), but didn't" % (str(self))) > + raise NotImplementedError("'%s' needs to implement > is_covered_localvars(), but didn't" % (str(self))) > > def is_equal(self, rule_obj, strict=False): > '''compare if rule_obj == self > @@ -142,7 +146,7 @@ > # @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(self))) > + raise NotImplementedError("'%s' needs to implement > is_equal_localvars(), but didn't" % (str(self))) > > def severity(self, sev_db): > '''return severity of this rule, which can be: > @@ -178,7 +182,7 @@ > def logprof_header_localvars(self): > '''return the headers (human-readable version of the rule) to > display in aa-logprof for this rule object > returns {'label1': 'value1', 'label2': 'value2'} ''' > - raise AppArmorBug("'%s' needs to implement logprof_header(), but > didn't" % (str(self))) > + raise NotImplementedError("'%s' needs to implement > logprof_header(), but didn't" % (str(self))) > > def modifiers_str(self): > '''return the allow/deny and audit keyword as string, including > whitespace''' > @@ -336,7 +340,7 @@ > def get_glob_ext(self, path_or_rule): > '''returns the next possible glob with extension (for file rules > only). > For all other rule types, raise an exception''' > - raise AppArmorBug("get_glob_ext is not available for this rule > type!") > + raise NotImplementedError("get_glob_ext is not available for this > rule type!") > > > def parse_comment(matches): > === modified file ./utils/test/test-baserule.py > --- utils/test/test-baserule.py 2015-10-29 13:11:10.802874470 +0100 > +++ utils/test/test-baserule.py 2015-10-29 12:12:19.971584508 +0100 > @@ -20,34 +20,34 @@ > > class TestBaserule(AATest): > def test_abstract__parse(self): > - with self.assertRaises(AppArmorBug): > + with self.assertRaises(NotImplementedError): > BaseRule._parse('foo') > > def test_abstract__parse_2(self): > - with self.assertRaises(AppArmorBug): > + with self.assertRaises(NotImplementedError): > BaseRule.parse('foo') > > def test_abstract__match(self): > - with self.assertRaises(AppArmorBug): > + with self.assertRaises(NotImplementedError): > BaseRule._match('foo') > > def test_abstract__match2(self): > - with self.assertRaises(AppArmorBug): > + with self.assertRaises(NotImplementedError): > BaseRule.match('foo') > > def test_abstract_get_clean(self): > obj = BaseRule() > - with self.assertRaises(AppArmorBug): > + with self.assertRaises(NotImplementedError): > obj.get_clean() > > def test_is_equal_localvars(self): > obj = BaseRule() > - with self.assertRaises(AppArmorBug): > + with self.assertRaises(NotImplementedError): > obj.is_equal_localvars(BaseRule()) > > def test_is_covered_localvars(self): > obj = BaseRule() > - with self.assertRaises(AppArmorBug): > + with self.assertRaises(NotImplementedError): > obj.is_covered_localvars(None) > > def test_parse_modifiers_invalid(self): > @@ -65,7 +65,7 @@ > > def test_logprof_header_localvars(self): > obj = BaseRule() > - with self.assertRaises(AppArmorBug): > + with self.assertRaises(NotImplementedError): > obj.logprof_header_localvars() > > > === modified file ./utils/test/test-capability.py > --- utils/test/test-capability.py 2015-10-29 13:11:10.802874470 +0100 > +++ utils/test/test-capability.py 2015-10-29 12:21:13.288600004 +0100 > @@ -635,7 +635,7 @@ > self.assertEqual(self.ruleset.get_glob('capability net_raw,'), > 'capability,') > > def test_glob_ext(self): > - with self.assertRaises(AppArmorBug): > + with self.assertRaises(NotImplementedError): > self.ruleset.get_glob_ext('capability net_raw,') > > class CapabilityDeleteTest(AATest): > === modified file ./utils/test/test-change_profile.py > --- utils/test/test-change_profile.py 2015-10-29 13:11:10.803874412 +0100 > +++ utils/test/test-change_profile.py 2015-10-29 12:16:05.183514365 +0100 > @@ -449,7 +449,7 @@ > # self.assertEqual(self.ruleset.get_glob('change_profile /foo -> > /bar,'), 'change_profile -> /bar,') > > def test_glob_ext(self): > - with self.assertRaises(AppArmorBug): > + with self.assertRaises(NotImplementedError): > # get_glob_ext is not available for change_profile rules > self.ruleset.get_glob_ext('change_profile /foo -> /bar,') > > === modified file ./utils/test/test-network.py > --- utils/test/test-network.py 2015-10-29 13:11:10.844872012 +0100 > +++ utils/test/test-network.py 2015-10-29 12:19:48.363533637 +0100 > @@ -441,7 +441,7 @@ > # self.assertEqual(self.ruleset.get_glob('network inet raw,'), > 'network inet,') > > def test_glob_ext(self): > - with self.assertRaises(AppArmorBug): > + with self.assertRaises(NotImplementedError): > # get_glob_ext is not available for network rules > self.ruleset.get_glob_ext('network inet raw,') > > === modified file ./utils/test/test-rlimit.py > --- utils/test/test-rlimit.py 2015-10-29 13:11:10.844872012 +0100 > +++ utils/test/test-rlimit.py 2015-10-29 12:19:13.128581461 +0100 > @@ -411,7 +411,7 @@ > # self.assertEqual(self.ruleset.get_glob('rlimit /foo -> /bar,'), > 'rlimit -> /bar,') > > def test_glob_ext(self): > - with self.assertRaises(AppArmorBug): > + with self.assertRaises(NotImplementedError): > # get_glob_ext is not available for rlimit rules > self.ruleset.get_glob_ext('set rlimit cpu <= 100,') > > === modified file ./utils/test/test-signal.py > --- utils/test/test-signal.py 2015-10-29 13:11:10.844872012 +0100 > +++ utils/test/test-signal.py 2015-10-29 12:20:30.940059830 +0100 > @@ -581,7 +581,7 @@ > # self.assertEqual(self.ruleset.get_glob('signal send raw,'), > 'signal send,') > > def test_glob_ext(self): > - with self.assertRaises(AppArmorBug): > + with self.assertRaises(NotImplementedError): > # get_glob_ext is not available for signal rules > self.ruleset.get_glob_ext('signal send set=int,') > > Looks good. Thanks for the patch. Acked-by: Kshitij Gupta <kgupta8...@gmail.com>. > > > Regards, > > Christian Boltz > -- > > > Gibt's mehr Gehalt? ;) > > Du glaubst doch nicht wirklich, dass ich über solche Dinge öffentlich > > rede, oder? > Du brauchst ja keine Summe nennen. Es reicht wenn Du uns alle hier mal > auf eine Pizza einlädst ;-))) > [>> David Haller, > Philipp Thomas und Thorsten Körner in suse-linux] > > > -- > AppArmor mailing list > AppArmor@lists.ubuntu.com > Modify settings or unsubscribe at: > https://lists.ubuntu.com/mailman/listinfo/apparmor > -- Regards, Kshitij Gupta
-- AppArmor mailing list AppArmor@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor