Hello,
the attached files contain my review notes for r17..22.
In case you miss the files for r19 and r20: that's intentional, those
commits look so good that I don't need to comment on them ;-)
Regards,
Christian Boltz
--
Jungs. Mit dem Argument kann ich kaputte Autos verkaufen und dann
erklären, daß Fahrradfahren eh viel gesünder ist.
[Peer Heinlein in postfixbuch-users]
=== modified file 'apparmor/aa.py'
--- apparmor/aa.py 2013-07-08 22:16:26 +0000
+++ apparmor/aa.py 2013-07-17 15:08:13 +0000
@@ -619,28 +624,36 @@
def set_profile_flags(prof_filename, newflags):
"""Reads the old profile file and updates the flags accordingly"""
regex_bin_flag = re.compile('^(\s*)(("??\/.+?"??)|(profile\s+("??.+?"??)))\s+(flags=\(.+\)\s+)*\{\s*$/')
- regex_hat_flag = re.compile('^(\s*\^\S+)\s+(flags=\(.+\)\s+)*\{\s*$')
+ regex_hat_flag = re.compile('^([a-z]*)\s+([A-Z]*)((\s+#\S*)*)\s*$')
### this looks superfluous because it's overwritten two lines below
+ a=re.compile('^([a-z]*)\s+([A-Z]*)((\s+#\S*)*)\s*$')
### a is not used anywhere in the function
+ regex_hat_flag = re.compile('^(\s*\^\S+)\s+(flags=\(.+\)\s+)*\{\s*(#*\S*)$')
### (regex_hat_flag overwritten here)
if os.path.isfile(prof_filename):
with open_file_read(prof_filename) as f_in:
- with open_file_write(prof_filename + '.new') as f_out:
+ tempfile = tempfile.NamedTemporaryFile('w', prefix=prof_filename , delete=False, dir='/etc/apparmor.d/')
### Looks quite good. A small detail is missing - please add suffix='~' to make sure
### a profile in a tempfile is not loaded when someone calls "rcapparmor reload"
@@ -654,3 +667,240 @@
+def sync_profile():
+def submit_created_profiles(new_profiles):
+def submit_changed_profiles(changed_profiles):
+def yast_select_and_upload_profiles(title, message, profiles_up):
+def console_select_and_upload_profiles(title, message, profiles_up):
+def set_profile_local_only(profs):
### those functions are about the profile repo and currently unused.
### a comment saying that would be nice
### (or move them to a separate file, maybe profilerepo.py)
+def handle_children(profile, hat, root):
+ entries = root[:]
+ for entry in entries:
+
### looks like the code to handle the children is still missing ;-)
=== added file 'apparmor/ui.py'
--- apparmor/ui.py 1970-01-01 00:00:00 +0000
+++ apparmor/ui.py 2013-07-17 15:08:13 +0000
@@ -0,0 +1,255 @@
+def init_localisation():
+ locale.setlocale(locale.LC_ALL, '')
+ #cur_locale = locale.getlocale()
+ filename = 'res/messages_%s.mo' % locale.getlocale()[0][0:2]
### [0:2] means locales like pt_BR won't be used (only pt, which might be different).
### I doubt this is intentional.
+def UI_Info(text):
+ if DEBUGGING:
+ debug_logger.info(text)
### debug_logger should check DEBUGGING itsself to avoid all the "if DEBUGGING:" conditions
### even if this means you'll need a small wrapper around debug_logger - but on the long term it makes the code more readable
+def UI_YesNo(text, default):
+ if DEBUGGING:
+ debug_logger.debug('UI_YesNo: %s: %s %s' %(UI_mode, text, default))
+ ans = default
+ if UI_mode == 'text':
+ yes = '(Y)es'
+ no = '(N)o'
+ usrmsg = 'PromptUser: Invalid hotkey for'
+ yeskey = 'y'
+ nokey = 'n'
### can you honor translations please?
### for example, in german it should be (j)a / (n)ein
+ sys.stdout.write('\n' + text + '\n')
+ if default == 'y':
### better: if default == yeskey - avoids problems when using translations
+ sys.stdout.write('\n[%s] / %s\n' % (yes, no))
+ else:
+ sys.stdout.write('\n%s / [%s]\n' % (yes, no))
+ ans = readkey()
+ if ans:
+ ans = ans.lower()
+ else:
+ ans = default
+ else:
+ SendDataTooYast({
+ 'type': 'dialog-yesno',
+ 'question': text
+ })
+ ypath, yarg = GetDataFromYast()
+ ans = yarg['answer']
+ if not ans:
+ ans = default
+ return ans
### ans can be anything, not just yeskey and nokey
### if something else (= invalid key) is entered, the function should ask again
### (see UI_YesNoCancel which does exactly that)
+def UI_YesNoCancel(text, default):
+ if DEBUGGING:
+ debug_logger.debug('UI_YesNoCancel: %s: %s %s' % (UI_mode, text, default))
+
+ if UI_mode == 'text':
+ yes = '(Y)es'
+ no = '(N)o'
+ cancel = '(C)ancel'
+ yeskey = 'y'
+ nokey = 'n'
+ cancelkey = 'c'
### should honor translations
+CMDS = {
+ 'CMD_ALLOW': '(A)llow',
+ 'CMD_OTHER': '(M)ore',
### do all those options honor translations?
=== added file 'apparmor/yasti.py'
--- apparmor/yasti.py 1970-01-01 00:00:00 +0000
+++ apparmor/yasti.py 2013-07-17 15:08:13 +0000
@@ -0,0 +1,82 @@
+def SendDataToYast(data):
+ for line in sys.stdin:
[...]
+ Return(data)
+ return True
### two return statements?
+def GetDataFromYast():
[...]
+ Return('true')
+ return ypath, yarg
### again - two return statements?
+def ParseTerm(input):
[...]
+ ycp.y2error('No term parantheses')
### typo: par_e_ntheses
=== modified file 'apparmor/config.py'
--- apparmor/config.py 2013-07-08 22:16:26 +0000
+++ apparmor/config.py 2013-07-17 21:41:05 +0000
+def py2_parser(filename):
+ tmp = tempfile.NamedTemporaryFile('rw')
+ f_out = open(tmp.name, 'w')
+ if os.path.exists(filename):
+ with open_file_read(filename) as f_in:
+ for line in f_in:
+ if line[:2] == ' ':
+ line = line[2:]
### this will fail if someone has for example three spaces ;-)
### why don't you just trim() all lines?
+ f_out.write(line)
+ f_out.flush()
+ return tmp
=== modified file 'Testing/severity_test.py'
--- Testing/severity_test.py 2013-07-17 15:08:13 +0000
+++ Testing/severity_test.py 2013-07-18 13:47:43 +0000
+ # Load all variables for /sbin/klogd and test them
+ s.load_variables('/etc/apparmor.d/sbin.klogd')
### please avoid using system-wide files in tests
### the best option here is to use the profiles in bzr/tarball
### which means something like '../../profiles/apparmor.d/sbin.klogd'
self.assertEqual(s.rank('@{PROC}/sys/vm/overcommit_memory', 'r'), 6, 'Invalid Rank')
self.assertEqual(s.rank('@{HOME}/sys/@{PROC}/overcommit_memory', 'r'), 10, 'Invalid Rank')
self.assertEqual(s.rank('/overco@{multiarch}mmit_memory', 'r'), 10, 'Invalid Rank')
- self.assertEqual(s.rank('@{PROC}/sys/@{TFTP_DIR}/overcommit_memory', 'r'), 6, 'Invalid Rank')
+ #self.assertEqual(s.rank('@{PROC}/sys/@{TFTP_DIR}/overcommit_memory', 'r'), 6, 'Invalid Rank')
### what's the reason for disabling this line?
=== modified file 'apparmor/severity.py'
--- apparmor/severity.py 2013-07-08 22:16:26 +0000
+++ apparmor/severity.py 2013-07-18 13:47:43 +0000
@@ -202,4 +180,34 @@
+ def load_variables(self, prof_path):
+ line = line.strip()
+ # If any includes, load variables from them fitst
+ if '#include' in line:
### line.startswith() would be a better test
### BTW: the # is optional - 'include' is also valid
+ new_path = line.split('<')[1].rstrip('>').strip()
### rstrip('>') will probably fail if there are inline comments
+ else:
+ if line.startswith('@') and '=' in line:
+ if '+=' in line:
+ line = line.split('+=')
+ try:
+ self.severity['VARIABLES'][line[0]] += [i.strip('"') for i in line[1].split()]
### what happens if line contains an inline comment?
+ else:
+ line = line.split('=')
+ if line[0] in self.severity['VARIABLES'].keys():
+ raise AppArmorException("Variable %s was previously declared" % line[0])
+ self.severity['VARIABLES'][line[0]] = [i.strip('"') for i in line[1].split()]
### again - what about inline comments?
=== added file 'Testing/config_test.py'
--- Testing/config_test.py 1970-01-01 00:00:00 +0000
+++ Testing/config_test.py 2013-07-18 19:14:55 +0000
@@ -0,0 +1,42 @@
+class Test(unittest.TestCase):
+
+
+ def test_IniConfig(self):
+ ini_config = config.Config('ini')
+ conf = ini_config.read_config('logprof.conf')
### does this test use the system-wide logprof.conf in /etc/apparmor/ ?
### (if yes: bad idea ;-)
+ def test_ShellConfig(self):
+ ini_config = config.Config('shell')
+ conf = ini_config.read_config('easyprof.conf')
### same here - easyprof.conf from /etc/apparmor/ ?
--
AppArmor mailing list
AppArmor@lists.ubuntu.com
Modify settings or unsubscribe at:
https://lists.ubuntu.com/mailman/listinfo/apparmor
