Hello everyone, Sorry for the delayed response. I wasn't well and have been barely crawling in and out of bed.
I'm starting to catch up all the mails and patches. Please bear with me. Btw @cboltz, you have the commit rights I suppose. Always feel free to commit patches. (They can always be reverted ;) ) Regards, Kshitij Gupta On Feb 15, 2014 12:16 PM, "Steve Beattie" <[email protected]> wrote: > Hey Christian, > > On Sat, Feb 15, 2014 at 12:36:03AM +0100, Christian Boltz wrote: > > the attached files contain my review notes for the merging branch > > lp:~sbeattie/apparmor/apparmor-new-pyutils-branch/ > > but they only contain some comments. > > > > I didn't find something terribly wrong, so I'd say: > > For merging this branch (r2392 to be exact): > > Acked-by: Christian Boltz <[email protected]> > > > > > > I also noticed my patches > > - new profile tools: preserve full initial comment > > - new profile tools - handling of "(F)inish" > > are not included yet. Can you please review and (hopefully) merge them? > > I'm hoping to, yes, but it requires a bit more deep understanding of the > issues and code then I've got at the moment. Sorry. > > > +++ utils/apparmor/translations.py > [snip] > > + > > +__apparmor_gettext__ = None > > + > > +def init_translation(): > > + global __apparmor_gettext__ > > + if __apparmor_gettext__ is None: > > + t = gettext.translation(TRANSLATION_DOMAIN, fallback=True) > > + __apparmor_gettext__ = t.gettext > > + return __apparmor_gettext__ > > > > # what's the reason to make __apparmor_gettext__ global? > > In python, globals are a bit different than other languages. Globals > are local to the module (i.e. have the scope of the file/module, > translations.py in this case). Also, you can read their value freely, > if there's no variable with the same name declared in a scope more > local than the module (i.e. declared in the current function). > But in order to modify the value and have that change be reflected > outside of the scope of the function, it needs the global declaration; > otherwise the change stays local to the function and is lost when > the function ends. > > > cb@geeko:~/apparmor/sbeattie-gsoc-pre-merge/apparmor-new-pyutils-branch/utils> > diff -r test/ > ~/apparmor/sbeattie-gsoc-pre-merge/pre-merger-cleanups/Testing/ > > diff -u -p -r test/easyprof.conf > /home/cb/apparmor/sbeattie-gsoc-pre-merge/pre-merger-cleanups/Testing/easyprof.conf > > --- test/easyprof.conf 2014-02-14 21:01:40.081542000 +0100 > > +++ > /home/cb/apparmor/sbeattie-gsoc-pre-merge/pre-merger-cleanups/Testing/easyprof.conf > 2014-02-11 18:48:23.035073000 +0100 > > @@ -1,5 +1,5 @@ > > # Location of system policygroups > > -POLICYGROUPS_DIR="./policygroups" > > +POLICYGROUPS_DIR="/usr/share/apparmor/easyprof/policygroups" > > > > # Location of system templates > > -TEMPLATES_DIR="./templates" > > +TEMPLATES_DIR="/usr/share/apparmor/easyprof/templates" > > > > > > # I'd understand this change for the system-wide config, but in /test/ ? > > I think you're a bit confused because of the wonky way > merging has happened, but that's how it was on kshitij's branch: > > http://bazaar.launchpad.net/~kgupta8592/apparmor-profile-tools/trunk/view/head:/Testing/easyprof.conf > Please note that I reverted that particular change when I > merged in Kshitij's branch into my to-be-merged-into-trunk > branch, which from the VCS point of view ended up being a NOP: > > http://bazaar.launchpad.net/~sbeattie/apparmor/apparmor-new-pyutils-branch/view/head:/utils/test/easyprof.conf > > And in general, I agree, we should be isolating tests from external > influences. > > > === modified file 'utils/apparmor/aa.py' > > --- utils/apparmor/aa.py 2014-02-13 18:01:03 +0000 > > +++ utils/apparmor/aa.py 2014-02-13 18:52:00 +0000 > > @@ -1088,7 +1088,7 @@ > > context_new = context_new + '^%s' % hat > > context_new = context_new + ' -> %s' % exec_target > > > > - ans_new = transitions.get(context_new, '') > > + # ans_new = transitions.get(context_new, '') # XXX > ans meant here? > > > > # good question ;-) > > I'm hoping Kshitij can answer it. :) > > > @@ -2188,9 +2188,9 @@ > > > > def do_logprof_pass(logmark='', passno=0, pid=pid): > > ... > > - seen = hasher() > > +# seen = hasher() # XXX global? > > > > # maybe - I'd guess the intention here is to reset it to empty > > # (might be needed for genprof, which can do several passes) > > If that's the case, then seen needs to be declared global in > do_logprof_pass(), because otherwise it's the same situation as > __apparmor_gettext__ above; the re-initialization assignment to > 'seen' and 'skip' below will only last for the lifetime of the > do_logprof_pass() function and not actually change the module scoped > variable. > > The pyflakes tool is complaining about both 'seen' and 'skip' because > they are assigned to but not referenced within this function. (Or > elsewhere, as far as I can see. I think the module level declarations > can be eliminated, too.) > > > > > @@ -2201,7 +2201,7 @@ > > log = [] > > # log_dict = hasher() > > # changed = dict() > > - skip = hasher() > > +# skip = hasher() # XXX global? > > > > # maybe - I'd guess the intention here is to reset it to empty > > # (might be needed for genprof, which can do several passes) > > See above comment. > > > @@ -3402,7 +3401,7 @@ > > 'path': write_paths, > > 'change_profile': write_change_profile, > > } > > - prof_correct = True > > + # prof_correct = True # XXX correct? > > > > # maybe, but not needed because the for loop starts with "correct = true" > > I wasn't sure with any of these that I marked XXX, but pyflakes > is part of make check, and an assignment to a variable that is not > referenced later is something pyflakes warns on, which results in > 'make check' failing. And it caught a legitimate bug; see the second > change in: > > > http://bazaar.launchpad.net/~sbeattie/apparmor/apparmor-new-pyutils-branch/revision/2380#utils/apparmor/aa.py > > But if Kshitij agrees that it's not needed at all, then we can drop it. > > Thanks for the review. > > -- > Steve Beattie > <[email protected]> > http://NxNW.org/~steve/ > > -- > AppArmor mailing list > [email protected] > Modify settings or unsubscribe at: > https://lists.ubuntu.com/mailman/listinfo/apparmor > >
-- AppArmor mailing list [email protected] Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor
