[apparmor] new profile tools - review of merging branch

[Christian Boltz]

Hello,

Am Freitag, 14. Februar 2014 schrieb Steve Beattie:
> On Sat, Feb 15, 2014 at 12:36:03AM +0100, Christian Boltz wrote:

> > 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.

Hmm, I thought you read all the code you merge, didn't you? ;-)

You can check both issues by testing (with and without the patches 
applied) - and my patch descriptions should explain what to test ;-)

> > +++ 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.

Thanks for explaining the details. Only needing the global declaration 
for writing sounds like an interesting[tm] design decision (even PHP is 
better here - it requires "global" for read and write access to global 
variables) - but I'm afraid we won't get that changed in python ;-)

This still doesn't explain why you didn't make __apparmor_gettext__ 
local to the function.

The only reason I can imagine is a bit of performance tuning if more 
than one script or module needs translations (the second one will get a 
cached instance of t.gettext).

> > +++
> > /home/cb/apparmor/sbeattie-gsoc-pre-merge/pre-merger-cleanups/Testi
> > ng/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/v
> iew/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:

Right, in the merged result it is "./" so everything is fine :-)


> > === modified file 'utils/apparmor/aa.py'

snipped - I'll let Kshitij answer the technical details ;-)


Regards,

Christian Boltz
-- 
wie jeder weiß ist Debian auf ISDN die langsamste bekannte Methode
Selbstmord zu begehen ("Selbstmord durch Erosion")
[http://blog.koehntopp.de/archives/113-Debian-ist-doch-schlecht..html]