[apparmor] [utils] proposed redesign for mergeprof
Steve Beattie
steve at nxnw.org
Thu Oct 16 16:55:34 UTC 2014
On Wed, Oct 15, 2014 at 10:14:58PM +0200, Christian Boltz wrote:
> Hello,
>
> Am Freitag, 5. September 2014 schrieb Kshitij Gupta:
> > Seems I sent out that patch a bit early, a secondary consolidated
> > patch which fixes (hopefully) the issue multiple profiles in a file is
> > attached.
> > (Brace a for loop is removed resulting in a huge diff due to the
> > indent change).
>
> As discussed in yesterday's meeting, we'd like to include this patch in
> the 2.9 final release that is planned for this week (!)
>
> I just updated the patch to apply on latest bzr[1] - the result is
> attached as
> mergeprof2-rebased.diff.
>
> I'm also attaching
> mergeprof2-rebased-diff-ignore-whitespace.diff
> which is the same patch made with "diff -w" to ignore whitespace
> changes. It's probably not a surprise that it's much shorter and easier
> to read ;-)
Comments on the latter follow. Nothing that's a blocks me from saying
Acked-by: Steve Beattie <steve at nxnw.org>.
>
>
> Remaining known issues I noticed while testing:
> - aa-mergeprof always asks for includes, even if they are already there
> - CMD_OTHER should work ;-)
>
>
> Regards,
>
> Christian Boltz
>
> [1] actually I applied the patch against r2636, and then re-applied the
> two commits after that
> --- aa-mergeprof 2014-10-15 22:02:03.608738000 +0200
> +++ aa-mergeprof__MERGED 2014-10-15 21:08:12.610435497 +0200
> @@ -28,17 +28,17 @@ _ = init_translation()
>
> parser = argparse.ArgumentParser(description=_('Perform a 2-way or 3-way merge on the given profiles'),
> epilog='WARNING: the arguments will change in a future version!')
> -parser.add_argument('mine', type=str, help=_('your profile'))
> -parser.add_argument('base', type=str, help=_('base profile'))
> -parser.add_argument('other', nargs='?', type=str, help=_('other profile'))
> +parser.add_argument('files', nargs='+', type=str, help=_('base profile'))
> +#parser.add_argument('other', nargs='?', type=str, help=_('other profile'))
> parser.add_argument('-d', '--dir', type=str, help=_('path to profiles'))
> #parser.add_argument('-a', '--auto', action='store_true', help=_('Automatically merge profiles, exits incase of *x conflicts'))
> args = parser.parse_args()
>
> +args.other = None
> # 2-way merge or 3-way merge based on number of params
> -merge_mode = 2 if args.other == None else 3
> +merge_mode = 2 #if args.other == None else 3
>
> -profiles = [args.mine, args.base, args.other]
> +profiles = [args.files, [args.other]]
>
> profiledir = args.dir
> if profiledir:
> @@ -46,25 +46,94 @@ if profiledir:
> if not os.path.isdir(apparmor.aa.profile_dir):
> raise apparmor.AppArmorException(_("%s is not a directory.") %profiledir)
>
> +def reset_aa():
> + apparmor.aa.aa = apparmor.aa.hasher()
> + apparmor.aa.filelist = apparmor.aa.hasher()
> + apparmor.aa.include = dict()
> + apparmor.aa.existing_profiles = apparmor.aa.hasher()
> + apparmor.aa.original_aa = apparmor.aa.hasher()
Not a blocker for this patch, but longer term, it'd be nice if aa was
class (with a name the indicated better what it conceptually
represents), just so that __init__() for the class could do all the
relevant busywork of the above.
Short term, the reset_aa() function might want to live in
apparmor/aa.py.
> +def find_profiles_from_files(files):
> + profile_to_filename = dict()
> + for file_name in files:
> + apparmor.aa.read_profile(file_name, True)
> + for profile_name in apparmor.aa.filelist[file_name]['profiles'].keys():
> + profile_to_filename[profile_name] = file_name
> + reset_aa()
> +
> + return profile_to_filename
> +
> +def find_files_from_profiles(profiles):
> + profile_to_filename = dict()
> + apparmor.aa.read_profiles()
> +
> + for profile_name in profiles:
> + profile_to_filename[profile_name] = apparmor.aa.get_profile_filename(profile_name)
> +
> + reset_aa()
> +
> + return profile_to_filename
These also seem like general utility functions that should live under
apparmor/ somewhere. Thoughts?
> def main():
> - mergeprofiles = Merge(profiles)
> + profiles_to_merge = set()
> +
> + base_files, other_files = profiles
> +
> + base_profile_to_file = find_profiles_from_files(base_files)
> +
> + profiles_to_merge = profiles_to_merge.union(set(base_profile_to_file.keys()))
I'm... confused by the need for the union() of an empty set
(profiles_to_merge) with a newly constructed set. Couldn't
profiles_to_merge be just assigned the result of
'set(base_profile_to_file.keys())'?
> + other_profile_to_file = dict()
> +
> + if merge_mode == 3:
> + other_profile_to_file = find_profiles_from_files(other_files)
> + profiles_to_merge.add(other_profile_to_file.keys())
> +
> + user_profile_to_file = find_files_from_profiles(profiles_to_merge)
> +
> + print(base_files,"\n",other_files)
> + print(base_profile_to_file,"\n",other_profile_to_file,"\n",user_profile_to_file)
> + print(profiles_to_merge)
> +
> + for profile_name in profiles_to_merge:
> + user_file = user_profile_to_file[profile_name]
> + base_file = base_profile_to_file.get(profile_name, None)
> + other_file = None
> +
> + if merge_mode == 3:
> + other_file = other_profile_to_file.get(profile_name, None)
> +
> + if base_file == None:
> + if other_file == None:
> + continue
> +
> + act([user_file, other_file, None], 2, profile_name)
> + else:
> + if other_file == None:
> + act([user_file, base_file, None], 2, profile_name)
> + else:
> + act([user_file, base_file, other_file], 3, profile_name)
> +
> + reset_aa()
> +
> +def act(files, merge_mode, merging_profile):
> + mergeprofiles = Merge(files)
> #Get rid of common/superfluous stuff
> mergeprofiles.clear_common()
>
> # if not args.auto:
> if 1 == 1: # workaround to avoid lots of whitespace changes
> if merge_mode == 3:
> - mergeprofiles.ask_the_questions('other')
> + mergeprofiles.ask_the_questions('other', merging_profile)
>
> mergeprofiles.clear_common()
>
> - mergeprofiles.ask_the_questions('base')
> + mergeprofiles.ask_the_questions('base', merging_profile)
>
> q = aaui.PromptQuestion()
> q.title = _('Changed Local Profiles')
> q.explanation = _('The following local profiles were changed. Would you like to save them?')
> - q.functions = ['CMD_SAVE_CHANGES', 'CMD_VIEW_CHANGES', 'CMD_ABORT']
> + q.functions = ['CMD_SAVE_CHANGES', 'CMD_VIEW_CHANGES', 'CMD_ABORT', 'CMD_IGNORE_ENTRY']
> q.default = 'CMD_VIEW_CHANGES'
> q.options = []
> q.selected = 0
> @@ -84,6 +153,8 @@ def main():
> #oldprofile = apparmor.serialize_profile(apparmor.original_aa[program], program, '')
> newprofile = apparmor.aa.serialize_profile(mergeprofiles.user.aa[program], program, '')
> apparmor.aa.display_changes_with_comments(mergeprofiles.user.filename, newprofile)
> + elif ans == 'CMD_IGNORE_ENTRY':
> + break
>
>
> class Merge(object):
> @@ -94,25 +165,18 @@ class Merge(object):
> apparmor.aa.read_profile(base, True)
> self.base = cleanprofile.Prof(base)
>
> - self.reset()
> + reset_aa()
>
> #Read and parse other profile and save profile data, include data from it and reset them
> if merge_mode == 3:
> apparmor.aa.read_profile(other, True)
> self.other = cleanprofile.Prof(other)
> - self.reset()
> + reset_aa()
>
> #Read and parse user profile
> apparmor.aa.read_profile(user, True)
> self.user = cleanprofile.Prof(user)
>
> - def reset(self):
> - apparmor.aa.aa = apparmor.aa.hasher()
> - apparmor.aa.filelist = apparmor.aa.hasher()
> - apparmor.aa.include = dict()
> - apparmor.aa.existing_profiles = apparmor.aa.hasher()
> - apparmor.aa.original_aa = apparmor.aa.hasher()
> -
> def clear_common(self):
> deleted = 0
>
> @@ -166,7 +230,7 @@ class Merge(object):
> raise apparmor.aa.AppArmorException(_('Unknown selection'))
> done = True
>
> - def ask_the_questions(self, other):
> + def ask_the_questions(self, other, profile):
> if other == 'other':
> other = self.other
> else:
> @@ -199,7 +263,7 @@ class Merge(object):
> sev_db = apparmor.aa.sev_db
> if not sev_db:
> sev_db = apparmor.severity.Severity(apparmor.aa.CONFDIR + '/severity.db', _('unknown'))
> - for profile in sorted(other.aa.keys()):
> +
> for hat in sorted(other.aa[profile].keys()):
> #Add the includes from the other profile to the user profile
> done = False
> That already gave me a mostly working aa-mergeprof, but I soon found out
> why Kshitij said the patch is not final yet ;-)
>
> I did the following additional fixes:
> - remove some debug output (which Kshitij intentionally kept in the
> draft patch)
> - add a UI_Info to display which profile will be merged
> - disable the mergeprofiles.clear_common() call because it crashes
> - disable (M)ore (CMD_OTHER) because it crashes
> - make (F)inish work everywhere
> - change the help text so that it doesn't mention 3-way-merge until we
> implement it
>
> Those changes are in mergeprof-fixes.diff which applies on top of
> mergeprof2-rebased.diff.
This second patch is Acked-by: Steve Beattie <steve at nxnw.org>.
Thanks!
--
Steve Beattie
<sbeattie at ubuntu.com>
http://NxNW.org/~steve/
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <https://lists.ubuntu.com/archives/apparmor/attachments/20141016/e84bd578/attachment-0001.pgp>
More information about the AppArmor
mailing list