[apparmor] new profile tools - review of merging branch
Kshitij Gupta
kgupta8592 at gmail.com
Sat Feb 15 19:39:20 UTC 2014
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" <steve at nxnw.org> 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 <apparmor at 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 at 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
> <sbeattie at ubuntu.com>
> http://NxNW.org/~steve/
>
> --
> AppArmor mailing list
> AppArmor at lists.ubuntu.com
> Modify settings or unsubscribe at:
> https://lists.ubuntu.com/mailman/listinfo/apparmor
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.ubuntu.com/archives/apparmor/attachments/20140216/2d0506c5/attachment.html>
More information about the AppArmor
mailing list