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

Sun Jan 29 01:25:58 UTC 2017

On 01/28/2017 04:23 PM, Christian Boltz wrote:
> 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.
> 
> 
Acked-by: John Johansen <john.johansen at canonical.com>

As for the question? I need to spend some more time with the code before
I can answer

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