[apparmor] GSoC review r33

[Christian Boltz]

Hello,

the review for r33 is attached.

The comments I initially had for r32 are included in r33 because you 
moved the convert_regexp function around ;-)


Regards,

Christian Boltz
-- 
Zu meiner Entschuldigung: Ich konnte es nicht nochmal durchlesen,
weil meine Kippenschachtel  leer war und ich also schnell das Haus
verlassen musste. Das neue Jahr - keine 11 Tage alt und die (guten)
Vorsätze schon alle über Bord....      [Rüdiger Meier in suse-linux]
-------------- next part --------------
------------------------------------------------------------
revno: 33
committer: Kshitij Gupta <kgupta8592 at gmail.com
branch nick: apparmor-profile-tools
timestamp: Mon 2013-08-05 18:55:34 +0530
message:
  Added some tests for common module and fixed a few minor bugs in regex parser

=== added file 'Testing/common_test.py'
--- Testing/common_test.py	1970-01-01 00:00:00 +0000
+++ Testing/common_test.py	2013-08-05 13:25:34 +0000
@@ -0,0 +1,77 @@
+    def test_RegexParser(self):
...
+        regex_3 = '/foo/{foo,bar,user,other}/bar/'

# please also add some similar tests for
#        regex_3a = '/foo/{foo,bar,user,other}/{asdf,qwer,yxcv}/bar/'


...        
+        regex_4 = '/foo/user/ba?/'
+        parsed_regex_4 = apparmor.common.convert_regexp(regex_4)
+        compiled_regex_4 = re.compile(parsed_regex_4)
+        #print(parsed_regex_4)
+        
+        self.assertEqual(bool(compiled_regex_4.search('/foo/user/bar/')), True, 'Incorrectly Parsed regex')
+        
+        self.assertEqual(bool(compiled_regex_4.search('/foo/user/bar/apparmor/')), False, 'Incorrectly Parsed regex')
+        self.assertEqual(bool(compiled_regex_4.search('/foo/user/ba/')), False, 'Incorrectly Parsed regex')

# I'd propose an additional test to make sure ? does not match /
#        self.assertEqual(bool(compiled_regex_4.search('/foo/user/ba//')), False, 'Incorrectly Parsed regex')

...        
+        regex_6 = '/foo/user/bar/*'
+        parsed_regex_6 = apparmor.common.convert_regexp(regex_6)
+        compiled_regex_6 = re.compile(parsed_regex_6)
+        #print(parsed_regex_6)
+        
+        self.assertEqual(bool(compiled_regex_6.search('/foo/user/bar/apparmor')), True, 'Incorrectly Parsed regex')
+        
+        self.assertEqual(bool(compiled_regex_6.search('/foo/user/bar/apparmor/tools')), False, 'Incorrectly Parsed regex')
+        self.assertEqual(bool(compiled_regex_6.search('/foo/user/bar/')), False, 'Incorrectly Parsed regex')

# additional (/ added):
#        self.assertEqual(bool(compiled_regex_6.search('/foo/user/bar/apparmor/')), False, 'Incorrectly Parsed regex')

# BTW: storing the regex and the tests (including expected value) in a big array (and then looping over it) will make the code easier readable
# You could even store the tests in an INI-style file:
# [/foo/user/bar/*]
#   /foo/user/bar/apparmor = True
#   /foo/user/bar/apparmor/tools = false



+    def test_readkey(self):
+        print("Please press the Y button on the keyboard.")
+        self.assertEqual(apparmor.common.readkey().lower(), 'y', 'Error reading key from shell!')

# Asking for a key press makes automated testing hard ;-)
# I'd disable this test by default, and enable it only in (to be created) interactive mode


=== modified file 'apparmor/common.py'
--- apparmor/common.py	2013-07-17 15:08:13 +0000
+++ apparmor/common.py	2013-08-05 13:25:34 +0000
@@ -151,4 +152,39 @@

+def convert_regexp(regexp):
+    regexp = regexp.strip()
+    new_reg = re.sub(r'(?<!\\)(\.|\+|\$)',r'\\\1',regexp)
+    # below will fail if { or } or , are part of a path too?   
+    #if re.search('({.*,.*)}', new_reg):
+    #    new_reg = new_reg.replace('{', '(')
+    #    new_reg = new_reg.replace('}', '}')
+    #    new_reg = new_reg.replace(',', '|')
+    
+    while re.search('{.*,.*}', new_reg):
+        match = re.search('(.*){(.*),(.*)}(.*)', new_reg).groups()

# does this work correctly if a line contains multiple {...} ?
# (see also 3a testcase proposal above)
# (using [^}]* instead of .* might make sense)

# you can also save one match part because you replace the , anyway
# so the following regex could work (also added ^...$):
#        match = re.search('(.*){([^}]*)}(.*)', new_reg).groups()
# (you'll need to adjust the match[] numbers)

+        prev = match[0]
+        after = match[3]
+        p1 = match[1].replace(',','|')
+        p2 = match[2].replace(',','|')
+        new_reg = prev+'('+p1+'|'+p2+')'+after


=== modified file 'apparmor/severity.py'
--- apparmor/severity.py	2013-07-22 23:05:51 +0000
+++ apparmor/severity.py	2013-08-05 13:25:34 +0000
@@ -65,24 +65,24 @@
         
-    def convert_regexp(self, path):
+#     def convert_regexp(self, path):

# feel free to delete (re)moved functions
# if you really need them again later, bzr history is only a command away ;-)


vim:ft=diff