Hello,
On Thu, Dec 25, 2014 at 6:02 AM, Christian Boltz <[email protected]> wrote:
> Hello,
>
> Am Mittwoch, 24. Dezember 2014 schrieb Kshitij Gupta:
> > On Sun, Dec 7, 2014 at 2:49 AM, Christian Boltz wrote:
> > > this patch updates is_skippable_file() to match all extensions that
> > > are listed in libapparmor _aa_is_blacklisted() - some extensions
> > > were missing in the python code.
> > >
> > > Also make the code more readable - even if it merges 4 re.search()
> > > into one, and add some testcases.
> > >
> > > Notes:
> > > - the original code additionally ignored *.swp. I didn't include
> > > that ->
> > > *.swp looks like vim swap files which are also dot files
> >
> > I'm not sure do we want to keep this or not. As stated parser_misc
> > doesn't ignore this.
> > A abc.txt.swp (open by vim) wont be covered under the dot file?
> > Also there are other ways a .swp file maybe created.
>
> vim creates a dot file - "vi abc.txt" creates .abc.txt.swp, and that is
> already covered by the "filename starts with a dot" rule.
>
> I must've missed the first "."
> We'll get a bug report if it is meant to be there.
>
> ;-)
>
> > A user says even mv created a .swp file:
> > http://unix.stackexchange.com/questions/27923/what-causes-swap-files-t
> > o-be-created
>
> Maybe (I never checked), but moving around profiles in GB size range is
> a very rare event ;-) and for smaller files it's unlikely that the tools
> see that file in the second it exists.
>
> Given enough time it will happen for someone at sometime. ;-)
> - the python code ignores README files, but the C code doesn't
> >
> > > (do we need to add README in the C code?)
> >
> > Somebody else can answer this?
>
> John? Tyler? Steve?
>
> > > [ is_skippable_file.diff ]
> > >
> > > === modified file 'utils/apparmor/aa.py'
> > > --- utils/apparmor/aa.py 2014-11-29 12:40:10 +0000
> > > +++ utils/apparmor/aa.py 2014-12-06 21:12:49 +0000
> > > @@ -2546,13 +2546,16 @@
> > >
> > > else:
> > > return False
> > >
> > > -# rpm backup files, dotfiles, emacs backup files should not be
> > > processed -# The skippable files type needs be synced with apparmor
> > > initscript>
> > > def is_skippable_file(path):
> > > - """Returns True if filename matches something to be skipped"""
> > > - if (re.search('(^|/)\.[^/]*$', path) or
> > > re.search('\.rpm(save|new)$', path)
> > > - or re.search('\.dpkg-(old|new)$', path) or
> > > re.search('\.swp$', path)
> > > - or path[-1] == '~' or path == 'README'):
> > > + """Returns True if filename matches something to be skipped
> > > (rpm or dpkg backup files, hidden files etc.)
> > > + The list of skippable files needs to be synced with
> > > apparmor
> > > initscript and libapparmor _aa_is_blacklisted()
> > > + path: filename (without directory)"""
> > > +
> > > + skippable_suffix =
> > > '(\.dpkg-new|\.dpkg-old|\.dpkg-dist|\.dpkg-bak|\.rpmnew|\.rpmsave|\.
> > > orig|\.rej|~)$'
> > > + skippable_files = '^(.*/)?(README|\.[^/]*|)$'
> > > # README, dot files and empty filename
> >
> > I suppose the number of calls to this function will be many as a large
> > number of files are parsed/ignored.
>
> Yes, basically number of files in /etc/apparmor.d/ (and subdirectories?)
>
> That are quite some files, but we have places that are called more often
> (for example parsing each line in the profile ;-)
>
> Hence, I said over-optimise ;-)
> > So, might I suggest that we (over?)optimise this?
> >
> > I can think of two ways:
> > 1. Use the regex and use re.compile for all the three regex's outside
> > the function call (It shows considerable speedup as tested by
> > cProfile in Python2.7)
>
> Indeed, re.compile is a good idea.
>
> Note that the first two are only strings that are then merged into
> skippable_files (to keep the regex parts easier readable).
> Only skippable_files is used as regex.
>
> Indeed only one re.compile call is needed.
> > 2. Use simple string methods and leverage os.path submodule. This
> > gives probably performance better than uncompiled regex and only a
> > bit better than compiled versions.
> >
> > An alternative version string based:
> >
> > def is_skippable_file(path):
> > basename = os.path.basename(path)
> > if not basename or basename[-1] == '~' or basename[0] == '.' or
> > basename == 'README':
> > return True
> > skippable_extension = ['.dpkg-new', '.dpkg-old', '.dpkg-dist',
> > '.dpkg-bak', '.rpmnew', '.rpmsave', '.orig', '.rej']
> > extension = os.path.splitext(basename)[1]
> > if extension in skippable_extension:
> > return True
> > return False
>
> That gives slightly less readable code IMHO - and the os.path.* calls
> also don't come for free. If it doesn't bring a big performance boost,
> I'd prefer the regex-based way.
>
> BTW: using basename.endswith() could also be an option, and the python
> docs say that it accepts tuplis as suffix since py 2.5.
>
> Sounds like a good idea to use ends with.
> > Letme know if you want to have a look at the script I used to compare
> > the performance.
>
> Yes, that would be interesting.
>
> PFA.
> I'd also be interested in some numbers - how long do the following
> variants take?
>
> a) the "old" code we currently have in bzr (hint: it does 4 re.search)
0.185 s for 1000 (x13 in each for all cases) calls to functions
b) with my patch applied (only one re.search, so I'd expect it to be
> faster)
>
0.160 s for 1000 (x13 in each for all cases) calls to functions
c) with my patch applied + using re.compile for skippable_files
> c1) with re.compile inside the function (= one re.compile per file)
>
0.144 s for 1000 (x13 in each for all cases) calls to functions
c2) with re.compile outside the function (= only one re.compile in
> total)
>
0.065 s for 1000 (x13 in each for all cases) calls to functions
d) with the code using os.path.* you proposed
>
Using os.path.* and in operator:
0.082 s for 1000 (x13 in each for all cases) calls to functions
Using os.path.basename followed by your suggested endswith for basename:
0.052 s for 1000 (x13 in each for all cases) calls to functions
I've attach the test scripts, separate files for all 6 cases.
Though the %improvement may seem big but for all purposes it wont be a huge
improvement, maybe a couple of milliseconds for large number of files. ;-)
>
(If you send me the test script, I can do the tests myself - but I won't
> object if you do them and send the result ;-)
>
> > + skippable_all = '%s|%s' % (skippable_suffix, skippable_files)
> >
> > > +
> > >
> > > + if re.search(skippable_all, path):
> > > return True
> > >
> > > Also maybe add a "return False" at the end ;-)
>
> Python returns None by default, which is "good enough" ;-)
>
>
OTOH, it never hurts if the code explicitely states what it does, so
> I'll add it in the next patch version.
>
:-)
>
> > def is_skippable_dir(path):
> > > === modified file 'utils/test/test-aa.py'
> > > --- utils/test/test-aa.py 2014-11-27 22:20:26 +0000
> > > +++ utils/test/test-aa.py 2014-12-06 21:06:15 +0000
> ...
> > The tests look good. I'll hold-back the ACK for the modified
> > version.
> >
> > Please have a look at the suggestions.
> >
> > Merry Christmas. (Apparently you were expecting an ACK under the
> > christmas tree ;-) )
>
> A review with comments is also ok ;-)
>
>
> Regards,
>
> Christian Boltz
> --
> [GUI vs. Command-Line] Einen ähnlichen Streit wird es in 20 Jahren
> auch geben, wenn die "2D-Screenfanatiker" auf die "VR Fans" losgehen
> und wieder ein Streit vom Zaun bricht der an Sinnfreiheit kaum zu
> überbieten ist. [Phillip Richdale in suse-linux]
>
>
> --
> AppArmor mailing list
> [email protected]
> Modify settings or unsubscribe at:
> https://lists.ubuntu.com/mailman/listinfo/apparmor
>
import re
import os
import cProfile
def string_version(p):
basename = os.path.basename(p)
if not basename or basename[-1] == '~' or basename[0] == '.' or basename == 'README':
return True
skippable_extension = ['.dpkg-new', '.dpkg-old', '.dpkg-dist', '.dpkg-bak', '.rpmnew', '.rpmsave', '.orig', '.rej']
extension = os.path.splitext(basename)[1]
if extension in skippable_extension:
return True
return False
def wrapper1():
for i in range(1000):
string_version("usr.lib.dovecot.anvil")
string_version("")
string_version("README")
string_version(".usr.bin")
string_version("bin.ping~")
string_version("usr.lib.dovecot.dpkg-new")
string_version("usr.lib.dovecot.dpkg-old")
string_version("usr.lib.dovecot.dpkg-dist")
string_version("usr.lib.dovecot.dpkg-bak")
string_version("usr.lib.dovecot.rpmnew")
string_version("usr.lib.dovecot.rpmsave")
string_version("usr.lib.dovecot.orig")
string_version("usr.lib.dovecot.rej")
cProfile.run('wrapper1()')
import re
import os
import cProfile
skippable_suffix = '(\.dpkg-new|\.dpkg-old|\.dpkg-dist|\.dpkg-bak|\.rpmnew|\.rpmsave|\.orig|\.rej|~)$'
skippable_files = '^(.*/)?(README|\.[^/]*|)$' # README, dot files and empty filename
skippable_all = re.compile('%s|%s' % (skippable_suffix, skippable_files))
def re_external_compile(p):
if skippable_all.search(p):
return True
return False
def wrapper2():
for i in range(1000):
re_external_compile("usr.lib.dovecot.anvil")
re_external_compile("")
re_external_compile("README")
re_external_compile(".usr.bin")
re_external_compile("bin.ping~")
re_external_compile("usr.lib.dovecot.dpkg-new")
re_external_compile("usr.lib.dovecot.dpkg-old")
re_external_compile("usr.lib.dovecot.dpkg-dist")
re_external_compile("usr.lib.dovecot.dpkg-bak")
re_external_compile("usr.lib.dovecot.rpmnew")
re_external_compile("usr.lib.dovecot.rpmsave")
re_external_compile("usr.lib.dovecot.orig")
re_external_compile("usr.lib.dovecot.rej")
cProfile.run('wrapper2()')
import re
import os
import cProfile
def re_internal_compile(p):
skippable_suffix = '(\.dpkg-new|\.dpkg-old|\.dpkg-dist|\.dpkg-bak|\.rpmnew|\.rpmsave|\.orig|\.rej|~)$'
skippable_files = '^(.*/)?(README|\.[^/]*|)$'
skippable_all = re.compile('%s|%s' % (skippable_suffix, skippable_files))
if skippable_all.search(p):
return True
return False
def wrapper3():
for i in range(1000):
re_internal_compile("usr.lib.dovecot.anvil")
re_internal_compile("")
re_internal_compile("README")
re_internal_compile(".usr.bin")
re_internal_compile("bin.ping~")
re_internal_compile("usr.lib.dovecot.dpkg-new")
re_internal_compile("usr.lib.dovecot.dpkg-old")
re_internal_compile("usr.lib.dovecot.dpkg-dist")
re_internal_compile("usr.lib.dovecot.dpkg-bak")
re_internal_compile("usr.lib.dovecot.rpmnew")
re_internal_compile("usr.lib.dovecot.rpmsave")
re_internal_compile("usr.lib.dovecot.orig")
re_internal_compile("usr.lib.dovecot.rej")
cProfile.run('wrapper3()')
import re
import os
import cProfile
def re_internal_search(p):
skippable_suffix = '(\.dpkg-new|\.dpkg-old|\.dpkg-dist|\.dpkg-bak|\.rpmnew|\.rpmsave|\.orig|\.rej|~)$'
skippable_files = '^(.*/)?(README|\.[^/]*|)$'
skippable_all = '%s|%s' % (skippable_suffix, skippable_files)
if re.search(skippable_all, p):
return True
return False
def wrapper4():
for i in range(1000):
re_internal_search("usr.lib.dovecot.anvil")
re_internal_search("")
re_internal_search("README")
re_internal_search(".usr.bin")
re_internal_search("bin.ping~")
re_internal_search("usr.lib.dovecot.dpkg-new")
re_internal_search("usr.lib.dovecot.dpkg-old")
re_internal_search("usr.lib.dovecot.dpkg-dist")
re_internal_search("usr.lib.dovecot.dpkg-bak")
re_internal_search("usr.lib.dovecot.rpmnew")
re_internal_search("usr.lib.dovecot.rpmsave")
re_internal_search("usr.lib.dovecot.orig")
re_internal_search("usr.lib.dovecot.rej")
cProfile.run('wrapper4()')
import re
import os
import cProfile
def re_old(p):
if (re.search('(^|/)\.[^/]*$', p)
or re.search('\.rpm(save|new)$', p)
or re.search('\.dpkg-(old|new)$', p)
or re.search('\.swp$', p)
or not p
or p[-1] == '~'
or p == 'README'):
return True
return False
def wrapper5():
for i in range(1000):
re_old("usr.lib.dovecot.anvil")
re_old("")
re_old("README")
re_old(".usr.bin")
re_old("bin.ping~")
re_old("usr.lib.dovecot.dpkg-new")
re_old("usr.lib.dovecot.dpkg-old")
re_old("usr.lib.dovecot.dpkg-dist")
re_old("usr.lib.dovecot.dpkg-bak")
re_old("usr.lib.dovecot.rpmnew")
re_old("usr.lib.dovecot.rpmsave")
re_old("usr.lib.dovecot.orig")
re_old("usr.lib.dovecot.rej")
cProfile.run('wrapper5()')
import re
import os
import cProfile
def string_version(p):
basename = os.path.basename(p)
if not basename or basename[-1] == '~' or basename[0] == '.' or basename == 'README':
return True
skippable_extension = ('.dpkg-new', '.dpkg-old', '.dpkg-dist', '.dpkg-bak', '.rpmnew', '.rpmsave', '.orig', '.rej')
if basename.endswith(skippable_extension):
return True
return False
def wrapper1():
for i in range(1000):
string_version("usr.lib.dovecot.anvil")
string_version("")
string_version("README")
string_version(".usr.bin")
string_version("bin.ping~")
string_version("usr.lib.dovecot.dpkg-new")
string_version("usr.lib.dovecot.dpkg-old")
string_version("usr.lib.dovecot.dpkg-dist")
string_version("usr.lib.dovecot.dpkg-bak")
string_version("usr.lib.dovecot.rpmnew")
string_version("usr.lib.dovecot.rpmsave")
string_version("usr.lib.dovecot.orig")
string_version("usr.lib.dovecot.rej")
cProfile.run('wrapper1()')
--
AppArmor mailing list
[email protected]
Modify settings or unsubscribe at:
https://lists.ubuntu.com/mailman/listinfo/apparmor
