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.johan...@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
>
>
>
--
AppArmor mailing list
AppArmor@lists.ubuntu.com
Modify settings or unsubscribe at:
https://lists.ubuntu.com/mailman/listinfo/apparmor
