Hello,
On Mon, Jun 15, 2015 at 12:44 AM, Christian Boltz <appar...@cboltz.de>
wrote:
> Hello,
>
> this patch moves re_match_include() to regex.py. The function is
> basically a wrapper around a regex, so regex.py is a much better home.
>
> While on it, rename the regex to RE_INCLUDE and compile it outside the
> function, which should result in a (small) performance improvement.
>
> Also adjust code calling it to the new location.
>
> Finally, add some tests for re_match_include()
>
>
>
> [ 49-move-re_match_include.diff ]
>
> === modified file utils/aa-mergeprof
> --- utils/aa-mergeprof 2015-06-14 20:33:26.203498911 +0200
> +++ utils/aa-mergeprof 2015-06-14 20:13:24.204740154 +0200
> @@ -17,9 +17,10 @@
> import os
>
> import apparmor.aa
> -from apparmor.aa import available_buttons, combine_name,
> delete_duplicates, is_known_rule, match_includes, re_match_include
> +from apparmor.aa import available_buttons, combine_name,
> delete_duplicates, is_known_rule, match_includes
> import apparmor.aamode
> from apparmor.common import AppArmorException
> +from apparmor.regex import re_match_include
> import apparmor.severity
> import apparmor.cleanprofile as cleanprofile
> import apparmor.ui as aaui
> @@ -267,7 +268,7 @@
> done = True
> elif ans == 'CMD_ALLOW':
> selection = options[selected]
> - inc = apparmor.aa.re_match_include(selection)
> + inc = re_match_include(selection)
> self.user.filelist[self.user.filename]['include'][inc] =
> True
> options.pop(selected)
> aaui.UI_Info(_('Adding %s to the file.') % selection)
> @@ -302,7 +303,7 @@
> done = True
> elif ans == 'CMD_ALLOW':
> selection = options[selected]
> - inc = apparmor.aa.re_match_include(selection)
> + inc = re_match_include(selection)
> deleted =
> apparmor.aa.delete_duplicates(aa[profile][hat], inc)
> aa[profile][hat]['include'][inc] = True
> options.pop(selected)
> @@ -524,7 +525,7 @@
> elif ans == 'CMD_ALLOW':
> path = options[selected]
> done = True
> - match = apparmor.aa.re_match_include(path)
> + match = re_match_include(path)
> if match:
> inc = match
> deleted =
> apparmor.aa.delete_duplicates(aa[profile][hat], inc)
> @@ -589,7 +590,7 @@
>
> elif ans == 'CMD_NEW':
> arg = options[selected]
> - if not apparmor.aa.re_match_include(arg):
> + if not re_match_include(arg):
> ans = aaui.UI_GetString(_('Enter new
> path: '), arg)
> # if ans:
> # if not matchliteral(ans,
> path):
> @@ -603,7 +604,7 @@
>
> elif ans == 'CMD_GLOB':
> newpath = options[selected].strip()
> - if not
> apparmor.aa.re_match_include(newpath):
> + if not re_match_include(newpath):
> newpath =
> apparmor.aa.glob_path(newpath)
>
> if newpath not in options:
> @@ -614,7 +615,7 @@
>
> elif ans == 'CMD_GLOBEXT':
> newpath = options[selected].strip()
> - if not
> apparmor.aa.re_match_include(newpath):
> + if not re_match_include(newpath):
> newpath =
> apparmor.aa.glob_path_withext(newpath)
>
> if newpath not in options:
> === modified file utils/apparmor/aa.py
> --- utils/apparmor/aa.py 2015-06-14 20:33:26.205498791 +0200
> +++ utils/apparmor/aa.py 2015-06-14 20:32:10.987010769 +0200
> @@ -49,7 +49,7 @@
> RE_PROFILE_HAT_DEF, RE_PROFILE_DBUS,
> RE_PROFILE_MOUNT,
> RE_PROFILE_SIGNAL, RE_PROFILE_PTRACE,
> RE_PROFILE_PIVOT_ROOT,
> RE_PROFILE_UNIX, RE_RULE_HAS_COMMA,
> RE_HAS_COMMENT_SPLIT,
> - strip_quotes, parse_profile_start_line )
> + strip_quotes, parse_profile_start_line,
> re_match_include )
>
> import apparmor.rules as aarules
>
> @@ -2110,15 +2110,6 @@
>
> return newincludes
>
> -def re_match_include(path):
> - """Matches the path for include and returns the include path"""
> - regex_include = re.compile('^\s*#?include\s*<(.*)>\s*(#.*)?$')
> - match = regex_include.search(path)
> - if match:
> - return match.groups()[0]
> - else:
> - return None
> -
> def valid_include(profile, incname):
> if profile and profile['include'].get(incname, False):
> return False
> === modified file utils/apparmor/cleanprofile.py
> --- utils/apparmor/cleanprofile.py 2015-06-08 22:23:14.887806000 +0200
> +++ utils/apparmor/cleanprofile.py 2015-06-14 20:29:20.957207783 +0200
> @@ -12,6 +12,7 @@
> #
> # ----------------------------------------------------------------------
> import apparmor.aa as apparmor
> +from apparmor.regex import re_match_include
>
> class Prof(object):
> def __init__(self, filename):
> @@ -90,7 +91,7 @@
> if not same_profile:
> deleted.append(entry)
> continue
> - if apparmor.re_match_include(rule) or
> apparmor.re_match_include(entry):
> + if re_match_include(rule) or re_match_include(entry):
> continue
> # Check if the rule implies entry
> if apparmor.matchliteral(rule, entry):
> === modified file utils/apparmor/regex.py
> --- utils/apparmor/regex.py 2015-06-14 20:33:26.205498791 +0200
> +++ utils/apparmor/regex.py 2015-06-14 20:14:39.423202789 +0200
> @@ -112,6 +112,19 @@
> return result
>
>
> +
> +RE_INCLUDE = re.compile('^\s*#?include\s*<(.*)>\s*(#.*)?$')
>
How about replacing the end part using RE_EOL?
Also the .* for include name means we can have #include<>? I would believe
the check happens somewhere for valid paths this but why not deal with the
easy case in regex itself?
Also, about should the regex be placed directly above the function using it
or along with all the other regexes?
> +
> +def re_match_include(path):
> + """Matches the path for include and returns the include path"""
> + match = RE_INCLUDE.search(path)
> + if match:
> + return match.groups()[0]
>
hmm... for the matters of this function the second comment group in regex
is wasted.
+ else:
> + return None
> +
> +
> +
>
Bikeshedding: 3 newlines or 2 newlines for separation?
> def strip_quotes(data):
> if data[0] + data[-1] == '""':
> return data[1:-1]
> === modified file utils/test/test-regex_matches.py
> --- utils/test/test-regex_matches.py 2015-06-14 20:33:26.206498731 +0200
> +++ utils/test/test-regex_matches.py 2015-06-14 20:14:57.120135248 +0200
> @@ -14,7 +14,7 @@
> from common_test import AATest, setup_all_loops
> from apparmor.common import AppArmorBug
>
> -from apparmor.regex import strip_quotes, parse_profile_start_line,
> RE_PROFILE_START, RE_PROFILE_CAP
> +from apparmor.regex import strip_quotes, parse_profile_start_line,
> re_match_include, RE_PROFILE_START, RE_PROFILE_CAP
>
>
> class AARegexTest(AATest):
> @@ -465,6 +465,23 @@
> with self.assertRaises(AppArmorBug):
> parse_profile_start_line(line, 'somefile')
>
> +class Test_re_match_include(AATest):
> + tests = [
> + ('#include <abstractions/base>', 'abstractions/base'
> ),
> + ('#include <abstractions/base> # comment', 'abstractions/base'
> ),
> + (' #include <abstractions/base> ', 'abstractions/base'
> ),
>
could probably also use #include<abstractions/base> the no space version?
> + ('include <abstractions/base>', 'abstractions/base'
> ), # not supported by parser
> + # ('include foo', 'foo'
> ), # XXX not supported in tools yet
> + # ('include /foo/bar', '/foo/bar'
> ), # XXX not supported in tools yet
> + # ('include "foo"', 'foo'
> ), # XXX not supported in tools yet
> + # ('include "/foo/bar"', '/foo/bar'
> ), # XXX not supported in tools yet
> + (' some #include <abstractions/base>', None,
> ),
> + (' /etc/fstab r,', None,
> ),
> + ]
> +
> + def _run_test(self, params, expected):
> + self.assertEqual(re_match_include(params), expected)
> +
>
You could probably add just copy-paste (with little tweaking) the above
tests with the regex instead of the function and cover the comment part.
> class TestStripQuotes(AATest):
> def test_strip_quotes_01(self):
>
>
> With suggestions for regex and tests addressed.
Acked-by: Kshitij Gupta <kgupta8...@gmail.com>.
Thanks.
Regards,
>
>
>
> Regards,
>
> Christian Boltz
> --
> > > [OOo splash] kroete should turn their eyes during load ;-)
> > It should roll with his eyes if user is doing something completely
> > stupid.
> ...like touching the mouse, or do you know even better heuristics?
> [> Robert Schiele and Hans-Peter Jansen]
>
>
> --
> AppArmor mailing list
> AppArmor@lists.ubuntu.com
> Modify settings or unsubscribe at:
> https://lists.ubuntu.com/mailman/listinfo/apparmor
>
--
AppArmor mailing list
AppArmor@lists.ubuntu.com
Modify settings or unsubscribe at:
https://lists.ubuntu.com/mailman/listinfo/apparmor
