<div dir="ltr">Hello, <div class="gmail_extra"> <div class="gmail_quote">On Sat, Jul 18, 2015 at 2:23 AM, Christian Boltz <<a href="mailto:apparmor@cboltz.de" target="_blank">apparmor@cboltz.de</a>> wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hello, add_event_to_tree() is a hard-to-test function because it hands over its result to add_to_tree(). This patch converts add_event_to_tree() to a simple wrapper function and moves the main code into parse_event_for_tree() and map_log_type(). These two new functions return their results and are therefore easier to test. [ 77-split-logparser-add_event_to_tree.diff ] diff -ru '--exclude=.bzr' ../HEAD-patches-applied/utils/apparmor/logparser.py ./utils/apparmor/logparser.py --- utils/apparmor/logparser.py 2015-07-17 22:43:21.977879320 +0200 +++ ./utils/apparmor/logparser.py 2015-07-17 22:45:14.380287480 +0200 @@ -180,23 +181,35 @@ #print("log",self.log) def add_event_to_tree(self, e): - aamode = e.get('aamode', 'UNKNOWN') - if e.get('type', False): + e = self.parse_event_for_tree(e) + if e is not None: + (pid, parent, mode, details) = e + self.add_to_tree(pid, parent, mode, details) + + def map_log_type(self, type): </blockquote><div> "type" is a keyword in Python so using it/overriding it in this scope is not a good idea and we can probably avoid it. How about "log_type" instead? </div><div>I'm aware that "type" has been incorrectly used as a variable at other places, but two wrongs don't make a right ;-) </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> - if re.search('(UNKNOWN\[1501\]|APPARMOR_AUDIT|1501)', e['type']): + if re.search('(UNKNOWN\[1501\]|APPARMOR_AUDIT|1501)', type): aamode = 'AUDIT' - elif re.search('(UNKNOWN\[1502\]|APPARMOR_ALLOWED|1502)', e['type']): + elif re.search('(UNKNOWN\[1502\]|APPARMOR_ALLOWED|1502)', type): aamode = 'PERMITTING' - elif re.search('(UNKNOWN\[1503\]|APPARMOR_DENIED|1503)', e['type']): + elif re.search('(UNKNOWN\[1503\]|APPARMOR_DENIED|1503)', type): aamode = 'REJECTING' - elif re.search('(UNKNOWN\[1504\]|APPARMOR_HINT|1504)', e['type']): + elif re.search('(UNKNOWN\[1504\]|APPARMOR_HINT|1504)', type): aamode = 'HINT' - elif re.search('(UNKNOWN\[1505\]|APPARMOR_STATUS|1505)', e['type']): + elif re.search('(UNKNOWN\[1505\]|APPARMOR_STATUS|1505)', type): aamode = 'STATUS' - elif re.search('(UNKNOWN\[1506\]|APPARMOR_ERROR|1506)', e['type']): + elif re.search('(UNKNOWN\[1506\]|APPARMOR_ERROR|1506)', type): aamode = 'ERROR' else: aamode = 'UNKNOWN' + return aamode + + def parse_event_for_tree(self, e): + aamode = e.get('aamode', 'UNKNOWN') + + if e.get('type', False): + aamode = self.map_log_type(e['type']) + if aamode in ['UNKNOWN', 'AUDIT', 'STATUS', 'ERROR']: return None @@ -240,13 +254,13 @@ e['request_mask'], e['name2'] = log_str_to_mode(e['profile'], e['request_mask'], e['name2']) if e.get('info', False) and e['info'] == 'mandatory profile missing': - self.add_to_tree(e['pid'], e['parent'], 'exec', + return(e['pid'], e['parent'], 'exec', [profile, hat, aamode, 'PERMITTING', e['denied_mask'], e['name'], e['name2']]) elif (e.get('name2', False) and '//null-' in e['name2']) or e.get('name', False): - self.add_to_tree(e['pid'], e['parent'], 'exec', + return(e['pid'], e['parent'], 'exec', [profile, hat, prog, aamode, e['denied_mask'], e['name'], '']) else: - self.debug_logger.debug('add_event_to_tree: dropped exec event in %s' % e['profile']) + self.debug_logger.debug('parse_event_for_tree: dropped exec event in %s' % e['profile']) elif ( e['operation'].startswith('file_') or e['operation'].startswith('inode_') or e['operation'] in ['open', 'truncate', 'mkdir', 'mknod', 'chmod', 'rename_src', @@ -286,14 +300,14 @@ self.throw_away_next_log_entry() if is_domain_change: - self.add_to_tree(e['pid'], e['parent'], 'exec', + return(e['pid'], e['parent'], 'exec', [profile, hat, prog, aamode, e['denied_mask'], e['name'], e['name2']]) else: - self.add_to_tree(e['pid'], e['parent'], 'path', + return(e['pid'], e['parent'], 'path', [profile, hat, prog, aamode, e['denied_mask'], e['name'], '']) elif e['operation'] == 'capable': - self.add_to_tree(e['pid'], e['parent'], 'capability', + return(e['pid'], e['parent'], 'capability', [profile, hat, prog, aamode, e['name'], '']) elif e['operation'] == 'clone': @@ -317,10 +331,10 @@ # self.pid[child] = arrayref elif self.op_type(e['operation']) == 'net': - self.add_to_tree(e['pid'], e['parent'], 'netdomain', + return(e['pid'], e['parent'], 'netdomain', [profile, hat, prog, aamode, e['family'], e['sock_type'], e['protocol']]) elif e['operation'] == 'change_hat': - self.add_to_tree(e['pid'], e['parent'], 'unknown_hat', + return(e['pid'], e['parent'], 'unknown_hat', [profile, hat, aamode, hat]) else: self.debug_logger.debug('UNHANDLED: %s' % e) </blockquote><div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> </blockquote><div>Looks fine to me. Haven't tested it though. Thanks for the patch. Acked-by: Kshitij Gupta <<a href="mailto:kgupta8592@gmail.com" target="_blank">kgupta8592@gmail.com</a>>. </div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> Regards, Christian Boltz -- A qualified candidate would display the following characteristics: [...] willing to apply the rules to everybody; primary goal is to safeguard quality, not friendship :) You're even allowed to decline coolo's request! [Craig Gardner in opensuse-packaging] -- AppArmor mailing list <a href="mailto:AppArmor@lists.ubuntu.com">AppArmor@lists.ubuntu.com</a> Modify settings or unsubscribe at: <a href="https://lists.ubuntu.com/mailman/listinfo/apparmor" rel="noreferrer" target="_blank">https://lists.ubuntu.com/mailman/listinfo/apparmor</a> </blockquote></div> -- <div class="gmail_signature"><div dir="ltr"><div>Regards, </div>Kshitij Gupta </div></div> </div></div>