[apparmor] GSoC review r40

[Christian Boltz]

Hello,

the review for r40 is attached. I also included the r34 [v2] comments, 
so you can skip the mail with the updated r34 review ;-)

@John: it also includes a question for you (the same I asked on IRC, but 
you didn't respond yet ;-)


Regards,

Christian Boltz
-- 
Sagt mal ehrlich: Ist mein Rechner geisteskrank????
[Harald Katzer in suse-linux]
-------------- next part --------------
------------------------------------------------------------
revno: 40
committer: Kshitij Gupta <kgupta8592 at gmail.com
branch nick: apparmor-profile-tools
timestamp: Sat 2013-08-10 12:46:22 +0530
message:
  fixes from rev 32..39 and fixed(?) flags=(..)thing


=== modified file 'Testing/common_test.py'
--- Testing/common_test.py	2013-08-05 13:25:34 +0000
+++ Testing/common_test.py	2013-08-10 07:16:22 +0000
@@ -4,72 +4,23 @@
+        tests=apparmor.config.Config('ini')
+        tests.CONF_DIR = '.'
+        regex_tests = tests.read_config('regex_tests.ini')
+        for regex in regex_tests.sections():
+            parsed_regex = re.compile(apparmor.common.convert_regexp(regex))
+            for regex_testcase in regex_tests.options(regex):
+                self.assertEqual(bool(parsed_regex.search(regex_testcase)), eval(regex_tests[regex][regex_testcase]), 'Incorrectly Parsed regex')

# the error message should print out which regex failed

# besides that, this + regex_tests.ini are much better readable :-) 


=== modified file 'Tools/aa-logprof.py'
--- Tools/aa-logprof.py	2013-08-07 09:13:17 +0000
+++ Tools/aa-logprof.py	2013-08-10 07:16:22 +0000
@@ -7,9 +7,9 @@
 import argparse
 
 if sys.version_info < (3,0):
-    os.environ['PATH'] = '/bin/:/sbin/:/usr/bin/:/usr/sbin'
+    os.environ['AAPATH'] = '/bin/:/sbin/:/usr/bin/:/usr/sbin'
 else:
-    os.environb.putenv('PATH', '/bin/:/sbin/:/usr/bin/:/usr/sbin')
+    os.environb.putenv('AAPATH', '/bin/:/sbin/:/usr/bin/:/usr/sbin')
 
=== modified file 'apparmor/aa.py'
--- apparmor/aa.py	2013-08-09 11:19:01 +0000
+++ apparmor/aa.py	2013-08-10 07:16:22 +0000
@@ -215,7 +215,7 @@
 
 def which(file):
     """Returns the executable fullpath for the file, None otherwise"""
-    env_dirs = os.getenv('PATH').split(':')
+    env_dirs = os.getenv('AAPATH').split(':')

# renaming the variable doesn't change much ;-)
# the question is if we want to override PATH or if we should use the existing dirs in PATH
# (which users might called expected behaviour, but could also include a small security risk)

# John?

     for env_dir in env_dirs:
         env_path = env_dir + '/' + file
         # Test if the path is executable or not

@@ -1751,6 +1748,7 @@
 
 def ask_the_questions():
     found = None
+    print(log_dict)

# debug code?

     for aamode in sorted(log_dict.keys()):
         # Describe the type of changes
         if aamode == 'PERMITTING':
@@ -2955,10 +2953,9 @@
     repo_data = None
     parsed_profiles = []
     initial_comment = ''
-    RE_PROFILE_START = re.compile('^(("??\/.+?"??)|(profile\s+("??.+?"??)))\s+((flags=)?\((.+)\)\s+)*\{\s*(#.*)?$')
+    RE_PROFILE_START = re.compile('^(("??\/.+?"??)|(profile\s+("??.+?"??)))\s+((flags=)?\((.+)\)\s+)?\{\s*(#.*)?$')
                                          ^
# regex_bin_flag does not have this \ - which of them is correct? ;-)
# (it would be even better to share the regex)

@@ -3373,12 +3361,12 @@
         else:
             print('Ignored: New definition for variable for:',list_var,'=', value, 'operation was:',var_operation,'old value=', var[list_var])
             pass
-            #raise AppArmorException('An existing variable redefined')
+            #raise AppArmorException('An existing variable redefined: %s' %list_var)

# (r35 comment)
# variable redefinition should be an error, not a warning
# (apparmor_parser also errors out IIRC)


     else:

# (r34 [v2] comment)
# a comment saying "else" means "+=" would be nice
# or, better, explicitely check for "+=" and add an else: that errors out saying "invalid var_operation"


         if var.get(list_var, False):
             var[list_var] = set(var[list_var] + vlist)
         else:
-            raise AppArmorException('An existing variable redefined')
+            raise AppArmorException('An existing variable redefined: %s' %list_var)
 
# (r34 [v2] comment)
# wrong error message, should be something like "attemped to append to non-existing variable"


=== modified file 'apparmor/common.py'
--- apparmor/common.py	2013-08-09 19:47:00 +0000
+++ apparmor/common.py	2013-08-10 07:16:22 +0000
@@ -165,13 +165,12 @@
     #    new_reg = new_reg.replace('}', '}')
     #    new_reg = new_reg.replace(',', '|')
     
-    while re.search('{.*,.*}', new_reg):
-        match = re.search('(.*){(.*),(.*)}(.*)', new_reg).groups()
+    while re.search('{[^}]*,[^}]*}', new_reg):
+        match = re.search('(.*){([^}]*)}(.*)', new_reg).groups()

# I'd use the same regex for while and match (hint: use the regex from match) and store it in a variable

# I'd also use ^...$ to be on the safe side

@@ -190,18 +189,25 @@
 
 class DebugLogger:
+    def __init__(self, module_name=__name__):       
         self.debugging = False
+        self.debug_level = logging.DEBUG   
         if os.getenv('LOGPROF_DEBUG', False):
-            self.debugging = True
+            self.debugging = os.getenv('LOGPROF_DEBUG')

# what happens if LOGPROF_DEBUG contains '12 monkeys' or 'I hate logs' ?
# hint: convert LOGPROF_DEBUG to integer to be on the safe side, and check for valid values (0..3)

+            if self.debugging == 1:
+                debug_level = logging.ERROR
+            elif self.debug_level == 2:
+                debug_level = logging.INFO

# what about loglevel == 3 for DEBUG ?

+        #logging.basicConfig(filename='/var/log/apparmor/logprof.log', level=self.debug_level, format='%(asctime)s - %(name)s - %(message)s\n')
+        logging.basicConfig(filename='/home/kshitij/logprof.log', level=self.debug_level, format='%(asctime)s - %(name)s - %(message)s\n')   

# golden rules of bad programming, rule 10 *SCNR*


vim:ft=diff