[apparmor] [utils] [patch] Fix stack trace in aa-mergeprof

[Kshitij Gupta]

Hello,

On Fri, Sep 5, 2014 at 3:32 AM, Christian Boltz <apparmor at cboltz.de> wrote:
> Hello,
>
> Am Freitag, 5. September 2014 schrieb Kshitij Gupta:
>> On Fri, Sep 5, 2014 at 1:27 AM, Christian Boltz wrote:
>
>> > I'm afraid you are a bit late with this patch ;-)
>
>> Sorry I missed that patch. I've been having great troubles with net
>> connection.
>>
>> I did pull the latest version a few hours before sending this patch,
>> maybe I missed it.
>
> No problem, it's better to have two fixes than a bug ;-)
>
>> Yes it seems like a good idea to be consistent.
>>
>> But is list(netrules_other['rule'][fam].keys()) very
>> intuitive/readable? Its not instantly obvious that a new list object
>> is being created which is not affected by the pop operation being
>> performed later on.
>
On second thought it looks intuitive and works as one would expect it to. :-)

> As long as it doesn't throw a stack trace (like it does without list()),
> the average programmer won't even care if it is the original or a copy ;-)
> (and py3 is the only programming language I know that bails out if you
> change the array you are using in the for loop. I'm still undecided if
> this is a good or a bad idea ;-)
>
> Besides that, list() has the advantage that we get a copy based on the
> latest netrules_other instead of the (possible outdated) copy you create
> at the start of the function.
>
> Also, I'd expect that using list() is faster than copy.deepcopy(), but
> that's just a guess.
>
>> Either way a consistent style would be better.
>
> Indeed. Let me start in the function we were talking about:
>
>
> === modified file 'utils/apparmor/cleanprofile.py'
> --- utils/apparmor/cleanprofile.py      2014-09-04 11:19:39 +0000
> +++ utils/apparmor/cleanprofile.py      2014-09-04 21:45:06 +0000
> @@ -123,14 +123,13 @@
>  def delete_net_duplicates(netrules, netrules_other, same_profile=True):
>      deleted = 0
>      hasher_obj = apparmor.aa.hasher()
> -    copy_netrules_other = copy.deepcopy(netrules_other)
>      if netrules_other and netrules:
>          netglob = False
>          # Delete matching rules
>          if netrules.get('all', False):
>              netglob = True
>          # Iterate over a copy of the rules in the other profile
> -        for fam in copy_netrules_other['rule'].keys():
> +        for fam in list(netrules_other['rule'].keys()):
>              if netglob or (type(netrules['rule'][fam]) != type(hasher_obj) and netrules['rule'][fam]):
>                  if not same_profile:
>                      if type(netrules_other['rule'][fam]) == type(hasher_obj):
>
>
>
Looks good. However removing that line results in an import being unused.

Please push with the minor cleanup.

Thanks.

Acked-by: Kshitij Gupta <kgupta8592 at gmail.com>.

Regards,
Kshitij Gupta
>
> Regards,
>
> Christian Boltz
> --
> Before it can be fixed, technical description of faulty behavior is
> needed. Note adjective "technical". "Extremely messed up" usually
> does not qualify as technical description.
> [Andrey Borzenkov in opensuse-factory]
>
>
> --
> AppArmor mailing list
> AppArmor at lists.ubuntu.com
> Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor