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 <[email protected]>. > > > 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 <[email protected]>. Thanks! -- Steve Beattie <[email protected]> http://NxNW.org/~steve/
signature.asc
Description: Digital signature
-- AppArmor mailing list [email protected] Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor
