On 2015-06-13 11:59:27, John Johansen wrote: > On 06/13/2015 10:43 AM, Tyler Hicks wrote: > > When the cache directory does not exist, but --write-cache and > > --skip-bad-cache are specified, the cache directory and the features > > file should be created but writes to the cache directory should not > > happen. This patch restores that behavior. > > > > The errno values libapparmor's aa_policy_cache_new() uses to indicate > > when the cache directory does not exist and when an existing, invalid > > cache already exists needed to be separated out. They were both ENOENT > > but now the latter situation uses EEXIST. > > > > libapparmor also needed to be updated to not print an error message to > > the syslog from aa_policy_cache_new() when the max_caches parameter is > > 0, indicating that a new cache should not be created, and the cache > > directory does not exist. This is an error situation but a debug message > > is more appropriate. > > > > Signed-off-by: Tyler Hicks <[email protected]> > > --- > > > > This patch is intended to be applied as a fixup patch at the end of the > > greater > > patch series. > > > > The approach taken is to modify the parser to recall into libapparmor, under > > certain conditions, to continue supporting the parser's --skip-bad-cache and > > --write-cache behavior when the cache directory does not exist. > > > > libraries/libapparmor/src/policy_cache.c | 6 ++++-- > > parser/parser_main.c | 19 ++++++++++++++++++- > > 2 files changed, 22 insertions(+), 3 deletions(-) > > > > diff --git a/libraries/libapparmor/src/policy_cache.c > > b/libraries/libapparmor/src/policy_cache.c > > index f685f0a..72c0176 100644 > > --- a/libraries/libapparmor/src/policy_cache.c > > +++ b/libraries/libapparmor/src/policy_cache.c > > @@ -78,7 +78,7 @@ static int init_cache_features(aa_policy_cache > > *policy_cache, > > } else if (!aa_features_is_equal(policy_cache->features, > > kernel_features)) { > > if (!create) { > > - errno = ENOENT; > > + errno = EEXIST; > > return -1; > > } > > > > @@ -166,8 +166,10 @@ open: > > if (mkdirat(dirfd, path, 0700) == 0) > > goto open; > > PERROR("Can't create cache directory '%s': %m\n", path); > > - } else { > > + } else if (create) { > > PERROR("Can't update cache directory '%s': %m\n", path); > > + } else { > > + PDEBUG("Cache directory '%s' does not exist\n", path); > > } > > > Acked by John Johansen <[email protected]> on the error code > portion of the patch > > > save = errno; > > diff --git a/parser/parser_main.c b/parser/parser_main.c > > index f274d00..3bb115d 100644 > > --- a/parser/parser_main.c > > +++ b/parser/parser_main.c > > @@ -918,13 +918,30 @@ int main(int argc, char *argv[]) > > if (create_cache_dir) > > pwarn(_("The --create-cache-dir option is deprecated. > > Please use --write-cache.\n")); > > > > +retry: > > retval = aa_policy_cache_new(&policy_cache, features, > > AT_FDCWD, cacheloc, max_caches); > > if (retval) { > > - if (errno != ENOENT) { > > + if (errno != ENOENT && errno != EEXIST) { > > PERROR(_("Failed setting up policy cache (%s): > > %s\n"), > > cacheloc, strerror(errno)); > > return 1; > > + } else if (errno == ENOENT && max_caches == 0 && > > + write_cache && !cond_clear_cache) { > > + /** > > + * --write-cache and --skip-bad-cache are > > + * present and errno is ENOENT, meaning that > > + * the cache dir does not exist. Legacy > > + * behavior is to create the cache dir in this > > + * situation so retry with a non-zero > > + * max_caches. 2.9 went ahead and wrote to the > > + * newly created cache dir. It has been decided, > > + * as of r2927, that 2.10 will not write to the > > + * new cache dir. > > + */ > > + write_cache = 0; > > + max_caches = 1; > > + goto retry; > > } > > > > if (show_cache) { > > > So my take away from the IRC discussion was the r2927 behavior was not > correct either > > I think we need to relook at this and decide exactly what we want going > forward. Lets get the API right not worry so much about legacy > > - Going forward we are going to have multiple caches (just not yet) > - Going forward it is possible a cache could be shared between multiple > kernels (again not yet)
max_caches supports the two above scenarios in a future-proof way > - We need to be able to turn creation on and off I think max_caches supports this. max_caches > 0 will allow caches to be created (and old ones auto-reaped, if needed). max_cache == 0 turns on read-only caching so that new caches will not be created but an old, valid cache to be used if available. > - The skip behavior was added to keep caches intact if they didn't match. > Which isn't so important once we have multip caches. After looking > more at what skip-bad-caches is forcing the code to do, I am now > inclined to just drop it and have the parser issue a warning that > skip-bad-caches is no longer supported. I'm fine with this. > Sorry I know I raised the issue around the cache changes, but you > have convinced me its not worth keeping. If cache skipping is > really needed we can just use skip-cache No worries. I agree with you that we need to get the API right. If you agree that max_caches does the right thing, then I think the original patch in this thread is sufficient. I can send out a follow up patch that deprecates --skip-bad-cache. Does that sound like the correct plan of action to you? Tyler
signature.asc
Description: Digital signature
-- AppArmor mailing list [email protected] Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor
