[apparmor] [PATCH 2/2] utils: Clean up str_to_mode()

[Tyler Hicks]

On 2014-04-23 22:18:04, Christian Boltz wrote:
> Hello,
> 
> Am Mittwoch, 23. April 2014 schrieb Tyler Hicks:
> > The first conditional around string being set is not needed. If string
> > is not set, the while loop will be skipped and mode will be returned.
> > 
> > The variable tmp was being overloaded by being the regex search result
> > and then being reassigned to be the first match group in the regex
> > search result. This patch keeps tmp as the regex search result and
> > then uses mode_char to represent the first match group of the search.
> > 
> > Group the search and replace actions together at the beginning of the
> > loop and group the mode character processing at the end of the loop.
> > 
> > Finally, remove the unnecessary check of tmp (now mode_char) before
> > calling MODE_HASH.get(tmp, False). If tmp is None or '', get() will
> > do the right thing and return False.
> > 
> > Signed-off-by: Tyler Hicks <tyhicks at canonical.com>
> > ---
> >  utils/apparmor/aamode.py | 10 +++++-----
> >  1 file changed, 5 insertions(+), 5 deletions(-)
> > 
> > diff --git a/utils/apparmor/aamode.py b/utils/apparmor/aamode.py
> > index a9fd307..6f64e45 100644
> > --- a/utils/apparmor/aamode.py
> > +++ b/utils/apparmor/aamode.py
> > @@ -87,16 +87,16 @@ def str_to_mode(string):
> > 
> >  def sub_str_to_mode(string):
> >      mode = set()
> > -    if not string:
> > -        return mode
> > +
> >      while string:
> >          tmp = MODE_MAP_RE.search(string)
> >          if not tmp:
> 
> Would a warning like "unknown mode %s"%string in the   if not tmp:  
> branch be a good idea? Silently ignoring the remaining part doesn't 
> sound like the best solution. (I know that warning won't be too helpful 
> because the function doesn't know where string comes from, but it's 
> still more helpful than silently ignoring stuff.)

I dunno. aamode.py doesn't emit any warnings. Also, str_to_mode() and
mode_to_str() are both called quite a bit. I'd be afraid of the warnings
they could generate. Note that mode_to_str() silently ignores unknown
mode bits, too.

> 
> >              break
> > -        tmp = tmp.groups()[0]
> >          string = MODE_MAP_RE.sub('', string, 1)
> > -        if tmp and MODE_HASH.get(tmp, False):
> > -            mode |= MODE_HASH[tmp]
> > +
> > +        mode_char = tmp.groups()[0]
> > +        if MODE_HASH.get(mode_char, False):
> > +            mode |= MODE_HASH[mode_char]
> >          else:
> >              pass
> 
> Nice cleanup!
> 
> With or without the warning,
> Acked-by: Christian Boltz <apparmor at cboltz.de>

Thanks!

Tyler

> 
> 
> Regards,
> 
> Christian Boltz
> -- 
> > Entscheiden was du brauchst musst du vorher, ein absolut stabiles 
> > System(das ist stable bei Debian unbestritten) oder etwas aktuelles
> Das Ding ist so stabil, daß ein "clamscan" sogar zu 100% reproduzierbar
> einfriert -- seit einem Dreivierteljahr.
> [Uwe Driessen und Peer Heinlein in postfixbuch-users]
> 
> 
> -- 
> AppArmor mailing list
> AppArmor at lists.ubuntu.com
> Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <https://lists.ubuntu.com/archives/apparmor/attachments/20140423/b6f57189/attachment.pgp>