Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
dirs,interfaces/apparmor: remove unused apparmor cache entries #770
Conversation
zyga
added some commits
Mar 31, 2016
jdstrand
reviewed
Mar 31, 2016
| @@ -70,6 +73,22 @@ apparmor_parser output: | ||
| "--replace --write-cache -O no-expr-simplify --cache-loc=/var/cache/apparmor /path/to/snap.samba.smbd"}) |
jdstrand
Mar 31, 2016
Contributor
This context for the diff shows a hard-coded --cache-loc. Please adjust it and any other uses of --cache-loc. With those changes, LGTM.
zyga
Mar 31, 2016
Contributor
This is okay (in this case) because we don't divert the root directory. I can add the diversion to ensure we are indeed using the variable name.
niemeyer
reviewed
Mar 31, 2016
| @@ -169,12 +169,14 @@ func reloadProfiles(profiles []string) error { | ||
| return nil | ||
| } | ||
| -func unloadProfiles(profiles []string) error { | ||
| +func unloadAndRemoveCachedProfiles(profiles []string) error { |
niemeyer
Mar 31, 2016
Contributor
Let's please keep the original function name. The point is still to unload the profile. The fact we clean up the cache is an implementation detail of it.
niemeyer
reviewed
Mar 31, 2016
| + err = os.MkdirAll(dirs.AppArmorCacheDir, 0700) | ||
| + c.Assert(err, IsNil) | ||
| + // Mock away any real apparmor interaction | ||
| + s.cmds = map[string]*testutil.MockCmd{ |
niemeyer
Mar 31, 2016
Contributor
Why do we need this map?
parser := testutil.MockCommand(c, "apparmor_parser" ...)
?
zyga
Apr 1, 2016
Contributor
We don't, it's just looks nicer to me to refer to a particular command as it appears on the shell rather than through a go variable name. I can get rid of it easily.
niemeyer
reviewed
Mar 31, 2016
| @@ -49,6 +52,11 @@ func LoadProfile(fname string) error { | ||
| return nil | ||
| } | ||
| +// RemoveCachedProfile removes binary cache file from /var/cache/apparmor | ||
| +func RemoveCachedProfile(profile string) error { | ||
| + return os.Remove(filepath.Join(dirs.AppArmorCacheDir, profile)) |
niemeyer
Mar 31, 2016
Contributor
Why isn't that being done inside UnloadProfile itself? This would make it the complement of LoadProfile, which writes the cache.
|
LGTM, but it feels like the logic might be simplified a bit in some cases, per inline comments. |
zyga
added some commits
Apr 1, 2016
zyga
merged commit baf26ec
into
snapcore:master
Apr 1, 2016
niemeyer
reviewed
Apr 1, 2016
| + err = os.MkdirAll(dirs.AppArmorCacheDir, 0700) | ||
| + c.Assert(err, IsNil) | ||
| + // Mock away any real apparmor interaction | ||
| + s.mockCmd = testutil.MockCommand(c, "apparmor_parser", fakeAppArmorParser) |
zyga commentedMar 31, 2016
This branch ensures we remove entries from
/var/cache/apparmorcorresponding to removed apparmor profiles. We have to do this becauseapparmor_parserwrites the cache but never removes it.