[apparmor] [patch] - performance again... small changes

[Seth Arnold]

On Wed, Nov 26, 2014 at 10:57:13PM +0100, Peter Maloney wrote:
> I was going to test the 2 separately to see if p1b is necessary, but
> cboltz seems to like its readability and prefers it anyway, so here it is.

Wow, nice speedups. The check_for_apparmor() change feel more readable.
I'm scared of touching it though, I seem to remember it was carefully
written to try to handle different cases of /proc mounted or not mounted,
securityfs located in different places, etc. This change seems good to me,
but it'd be nice for someone else to also give it a look.

> diff -ur orig/aa.py p1a/aa.py
> --- orig/aa.py	2014-10-16 22:03:42.000000000 +0200
> +++ p1a/aa.py	2014-11-24 22:55:56.522927379 +0100
> @@ -147,29 +147,27 @@
>      sys.exit(1)
>  
>  def check_for_apparmor():
> -    """Finds and returns the mointpoint for apparmor None otherwise"""
> +    """Finds and returns the mountpoint for apparmor None otherwise"""
>      filesystem = '/proc/filesystems'
>      mounts = '/proc/mounts'
>      support_securityfs = False
>      aa_mountpoint = None
> -    regex_securityfs = re.compile('^\S+\s+(\S+)\s+securityfs\s')
>      if valid_path(filesystem):
>          with open_file_read(filesystem) as f_in:
>              for line in f_in:
>                  if 'securityfs' in line:
>                      support_securityfs = True
> -    if valid_path(mounts):
> -        with open_file_read(mounts) as f_in:
> -            for line in f_in:
> -                if support_securityfs:
> -                    match = regex_securityfs.search(line)
> -                    if match:
> -                        mountpoint = match.groups()[0] + '/apparmor'
> -                        if valid_path(mountpoint):
> +                    break
> +        if support_securityfs:
> +            with open_file_read(mounts) as f_in:
> +                for line in f_in:
> +                    split = line.split()
> +                    if len(split) > 2 and split[2] == 'securityfs':
> +                        mountpoint = split[1] + '/apparmor'
> +                        # Check if apparmor is actually mounted there
> +                        if valid_path(mountpoint) and valid_path(mountpoint + '/profiles'):
>                              aa_mountpoint = mountpoint
> -    # Check if apparmor is actually mounted there
> -    if not valid_path(aa_mountpoint + '/profiles'):
> -        aa_mountpoint = None
> +                            break
>      return aa_mountpoint
>  
>  def which(file):

Acked-by: Seth Arnold <seth.arnold at canonical.com>


> diff -ur apparmor.orig/aamode.py apparmor.p1/aamode.py
> --- apparmor.orig/aamode.py	2014-07-14 20:56:26.000000000 +0200
> +++ apparmor.p1/aamode.py	2014-11-09 22:15:01.875415033 +0100
> @@ -68,7 +68,7 @@
>               }
>  
>  LOG_MODE_RE = re.compile('(r|w|l|m|k|a|x|ix|ux|px|pux|cx|nx|pix|cix|Ux|Px|PUx|Cx|Nx|Pix|Cix)')
> -MODE_MAP_RE = re.compile('(r|w|l|m|k|a|x|i|u|p|c|n|I|U|P|C|N)')
> +MODE_MAP_LIST = ["r", "w", "l", "m", "k", "a", "x", "i", "u", "p", "c", "n", "I", "U", "P", "C", "N"]
>  
>  def str_to_mode(string):
>      if not string:
> @@ -87,27 +87,23 @@
>  
>  def sub_str_to_mode(string):
>      mode = set()
> -
> -    while string:
> -        tmp = MODE_MAP_RE.search(string)
> -        if not tmp:
> +    
> +    for mode_char in string:
> +        if mode_char not in MODE_MAP_LIST:
>              break
> -        string = MODE_MAP_RE.sub('', string, 1)
> -
> -        mode_char = tmp.groups()[0]
>          if MODE_HASH.get(mode_char, False):
>              mode |= MODE_HASH[mode_char]
> -        else:
> -            pass
>  
>      return mode
>  
>  def split_log_mode(mode):
> +    #if the mode has a "::", then the left side is the user mode, and the right side is the other mode
> +    #if not, then the mode is both the user and other mode
>      user = ''
>      other = ''
> -    match = re.search('(.*?)::(.*)', mode)
> -    if match:
> -        user, other = match.groups()
> +    
> +    if "::" in mode:
> +        user, other = mode.split("::")
>      else:
>          user = mode
>          other = mode

The mode.split("::") speedup makes a ton of sense. I'm less convinced of
the MOD_MAP_LIST lookup, that's a long list to iterate through. If you're
up for more testing, I'm curious about the performance difference of only
the :: splitting, only the MODE_MAP_LIST, and perhaps using MOD_MAP_SET =
{ 'r', 'w'. 'l', ...} as well.

For just the split_log_mode() change,
Acked-by: Seth Arnold <seth.arnold at canonical.com>

Thanks
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: Digital signature
URL: <https://lists.ubuntu.com/archives/apparmor/attachments/20141126/f5ed84be/attachment.pgp>