[apparmor] [patch] Rename global variable "pid" to "log_pid"

Sun Jan 29 00:23:41 UTC 2017

Hello,

aa.py has a global variable "pid", but it also has several functions
that use "pid" as a local variable name. do_logprof_pass() even uses
both - first, it passes the global variable to ReadLog, and then it
creates a local variable in the "for pid in ..." loop.

This patch renames the global variable to log_pid to get rid of the
confusion.

Note that the global variable is only handed over to ReadLog, and the
only case where its previous content _might_ be used is aa-genprof which
does multipe do_logprof_pass() runs.

Maybe we could even get rid of this variable in aa.py and make it local
to the ReadLog class, but I'm not sure if that would affect aa-genprof
in interesting[tm] ways. (Feedback welcome.)

To help you to decide if keeping the variable state is needed between
do_logprof_pass() runs, I'll paste the IRC log with all details after
the patch.



[ 01-rename-global-variable-pid.diff ]

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

--- utils/apparmor/aa.py        2017-01-29 00:33:26.144842133 +0100
+++ utils/apparmor/aa.py        2017-01-29 00:33:08.076936440 +0100
@@ -105,7 +105,7 @@
 extras = hasher()  # Inactive profiles from extras
 ### end our
 log = []
-pid = dict()
+log_pid = dict()  # handed over to ReadLog, gets filled in logparser.py. The only case the previous content of this variable _might_(?) be used is aa-genprof (multiple do_logprof_pass() runs)
 
 profile_changes = hasher()
 prelog = hasher()
@@ -1857,7 +1857,7 @@
     elif os.path.isdir(logfile):
         raise AppArmorException(_('%s is a directory. Please specify a file as logfile') % logfile)
 
-def do_logprof_pass(logmark='', passno=0, pid=pid):
+def do_logprof_pass(logmark='', passno=0, log_pid=log_pid):
     # set up variables for this pass
 #    transitions = hasher()
     global log
@@ -1885,7 +1885,7 @@
     ##    if not repo_cfg['repository'].get('enabled', False) or repo_cfg['repository]['enabled'] not in ['yes', 'no']:
     ##    UI_ask_to_enable_repo()
 
-    log_reader = apparmor.logparser.ReadLog(pid, logfile, existing_profiles, profile_dir, log)
+    log_reader = apparmor.logparser.ReadLog(log_pid, logfile, existing_profiles, profile_dir, log)
     log = log_reader.read_log(logmark)
     #read_log(logmark)



IRC log about the variable:

[23:19:31] <cboltz> for additional fun - one of the log lines manages to crash my local copy of aa-logprof
[23:19:45] <cboltz> (no need to worry, the code in bzr doesn't crash)
[23:22:25] <cboltz> ah, found it - now I know that the "pid" variable in aa.py isn't unused ;-)
[23:27:01] <cboltz> to make things funnier, several functions in aa.py have a _local_ "pid" variable that doesn't have to do anything with the global variable
[23:27:37] <cboltz> one even first uses the global one, and then locally overwrites it :-/
[23:38:27] <jjohansen> :(
[23:38:57] <cboltz> oh, I can even top that ;-)
[23:39:12] <cboltz> it seems the global one has exactly one usage:
[23:39:20] <cboltz> it's handed over to logparser.py
[23:39:30] <cboltz> but it seems nothing in aa.py cares about its content
[23:39:36] <jjohansen> I have no doubt you can, cboltz harbinger of bugs
[23:39:53] <cboltz> so maybe we can drop it in aa.py and simply add it in logparser.py
[23:40:16] <jjohansen> wfm
[00:13:49] <cboltz> maybe it won't, but I'm not totally sure ;-)
[00:14:02] <cboltz> there is one case where the variable gets used again - aa-genprof
[00:14:28] <cboltz> (every "(s)can logfile" will call logparser.py, which can then see the previous variable content)
[00:14:57] <cboltz> now the interesting question is if this is needed or not
[00:16:09] <cboltz> I'd _guess_ it could be helpful to track long-running execs to "translate" null-* to the right profile name, but I'm not really sure about it
[00:22:22] <cboltz> OTOH, we have profile_changes[] which is meant to track exactly that
[00:22:24] <cboltz> hmmm....
[00:44:12] * cboltz wonders if he finally scared jjohansen
[00:54:13] <jjohansen> cboltz: heh, not as bad as my son driving
[00:54:14] <jjohansen> :)
[00:55:48] <cboltz> ;-)
[00:56:27] <cboltz> for the variable - for now, I'll send a patch to rename it to get rid of the confusion
[00:56:47] <cboltz> the patch description will also include the question if it's needed in aa.py at all
[00:57:30] <cboltz> I'll simply paste the IRC log into the mail so that everybody can see all details ;-)
[00:58:24] <jjohansen> hahaha
[00:59:16] <cboltz> I wonder if I should add "sponsored by Aspirin" ;-)
[01:03:13] <jjohansen> you could have a nice little side business if Asprin was sponsoring all the bugs you find
[01:03:35] <cboltz> little? ;-)
[01:04:21] <jjohansen> hehehe

 

Regards,

Christian Boltz
-- 
Oh großer Meister! Darf man euch untertänigst darauf aufmerksam
machen, daß das diff'en von Postscriptfonts komplette Unterordner
synchronisiert und diff't, unter Berücksichtigung von Links? :-)
[Ratti in fontlinge-devel]
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: This is a digitally signed message part.
URL: <https://lists.ubuntu.com/archives/apparmor/attachments/20170129/10f10422/attachment.pgp>
