Hello, Am Mittwoch, 1. Juni 2016, 23:06:41 CEST schrieb Kshitij Gupta: > On Wed, Jun 1, 2016 at 5:37 AM, Christian Boltz <appar...@cboltz.de> wrote: > > thanks to reading the wrong directory in read_inactive_profiles() > > (profile_dir instead of extra_profile_dir), aa-genprof never asked > > about using a profile from the extra_profile_dir. > > > > Sounds like an easy fix, right? ;-) ... > > I propose this patch for 2.9, 2.10 and trunk > > (test-translations.py is only in trunk, therefore this part of the > > patch is obviously trunk-only.) > > > > > > [ 01-genprof-ask-for-extra-dir.diff ] > > > > === modified file ./utils/apparmor/aa.py > > --- utils/apparmor/aa.py 2016-05-30 23:16:05.713448348 +0200 > > +++ utils/apparmor/aa.py 2016-06-01 01:25:31.242505830 +0200 > > @@ -578,8 +578,11 @@ > > > > inactive_profile[prof_name][prof_name].pop('filename') > > profile_hash[uname]['username'] = uname > > profile_hash[uname]['profile_type'] = 'INACTIVE_LOCAL' > > > > - profile_hash[uname]['profile'] = > > serialize_profile(inactive_profile[prof_name], prof_name) > > + profile_hash[uname]['profile'] = > > serialize_profile(inactive_profile[prof_name], prof_name, None) > > Wouldn't it be better to then make the parameter optional with default > value as None, I have two reasons: > 1. I see the two parameter form littered around in aa.py *hides*
I noticed that and intentionally didn't fix it because it points out dead code ;-) Well, at least to someone who knows about the function parameters, so "me yesterday" ;-) > 2. The function has built in handling for cases when the options are > not present and sort of makes sense to have an option free serialiser > with a default > 3. The None there looks absolutely meaningless and confusing (to me > and explains why I above said two reasonsand not 3) Sounds like good arguments - patches welcome ;-) > > profile_hash[uname]['profile_data'] = inactive_profile > > + > > + existing_profiles.pop(prof_name) # remove profile filename > > from list to force storing in /etc/apparmor.d/ instead of > > extra_profile_dir > force storing in profile_dir you mean? Yes, of course. I just used /etc/apparmor.d to make the command easier to understand ;-) > > + proc.communicate(p['profile'].encode()) > > space before encode [comment ignored as discussed on IRC] > > @@ -683,6 +682,7 @@ > > > > if not profile_data: > > profile_data = create_new_profile(pname) > > > > file = get_profile_filename(pname) > > > > + profile_data[pname][pname]['filename'] = None # will be stored > > in /etc/apparmor.d when saving, so it shouldn't carry the > > profile_dir? see above ;-) > I agree to the sentiment that this is essentially a bugfix for a > long broken feature. > > Acked-by: Kshitij Gupta <kgupta8...@gmail.com> Thanks! Regards, Christian Boltz -- Die drei Todfeinde des Programmieres: Sonnenlicht, frische Luft und das unerträgliche Gebrüll der Vögel.
signature.asc
Description: This is a digitally signed message part.
-- AppArmor mailing list AppArmor@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor