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 <appar...@cboltz.de> > > > 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 <sbeat...@ubuntu.com> http://NxNW.org/~steve/
signature.asc
Description: Digital signature
-- AppArmor mailing list AppArmor@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor
