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)
- We need to be able to turn creation on and off
- 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.
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
--
AppArmor mailing list
[email protected]
Modify settings or unsubscribe at:
https://lists.ubuntu.com/mailman/listinfo/apparmor
