One minor nitpick/question... On 2013-07-23 08:36:12, Steve Beattie wrote: > Subject: [parser patch] fix apparmor cache tempfile location to use passed arg > > This patch fixes problems in the handling of both the final cache > name location and the temporary cache file when an alternate location > is specified. > > The first issue is that if the alternate cache directory location was > specified, the alternate directory name would be used as the final location > for > the cache file, rather than the alternate directory + the basename of > the profile. > > The second issue is that it would generate the temporary file that it > stores the cache file in [basedir]/cache even if an alternate cache > location was specified on the command line. This causes a problem > if [basedir]/cache is on a separate device than the alternate cache > location, because the rename() of the tempfile into the final location > would fail (which the parser would not check the return code of). It > will also break if the filesystem the basedir is located on is mounted > read-only, which would be a motivating reason to use an alternate > cache location. > > This patch fixes the above by incorporating the basename into the cache > file name if the alternate cache location has been specified, bases the > temporary cache file name on the destination cache name (such that they > end up in the same directory), and finally detects if the rename fails > and unlinks the temporary file if that happens (rather than leave it > around). > > Signed-off-by: Steve Beattie <[email protected]> > > --- > parser/parser_main.c | 12 +++++++----- > 1 file changed, 7 insertions(+), 5 deletions(-) > > Index: b/parser/parser_main.c > =================================================================== > --- a/parser/parser_main.c > +++ b/parser/parser_main.c > @@ -1069,8 +1069,7 @@ int process_profile(int option, char *pr > if ((profilename && option != OPTION_REMOVE) && !force_complain && > !skip_cache) { > if (cacheloc) { > - cachename = strdup(cacheloc); > - if (!cachename) { > + if (asprintf(&cachename, "%s/%s", cacheloc, > basename)<0) { > PERROR(_("Memory allocation error.")); > exit(1); > } > @@ -1089,7 +1088,7 @@ int process_profile(int option, char *pr > } > if (write_cache) { > /* Otherwise, set up to save a cached copy */ > - if (asprintf(&cachetemp, "%s/%s/%s-XXXXXX", basedir, > "cache", basename)<0) { > + if (asprintf(&cachetemp, "%s-XXXXXX", cachename)<0) { > perror("asprintf"); > exit(1); > } > @@ -1147,8 +1146,11 @@ out: > } > > if (useable_cache) { > - rename(cachetemp, cachename); > - if (show_cache) > + if (rename(cachetemp, cachename) < 0) { > + PERROR("Warning failed to write cache: %s\n", > cachename);
I've been curious about the policy surrounding -q. Here's the option's
description:
-q, --quiet
Do not report on the profiles as they are
loaded, and not show warnings.
I see there's a pwarn() that respects -q and that there's one PERROR()
call conditional on -q. This message starts with "Warning", so it seems
like it should also respect -q.
(I'm mainly wondering because there's a dbus parser test that I wrote
which spams the build log with the same warning over and over, so this
PERROR() call caught my eye.)
Tyler
> + unlink(cachetemp);
> + }
> + else if (show_cache)
> PERROR("Wrote cache: %s\n", cachename);
> }
> else {
> --
> Steve Beattie
> <[email protected]>
> http://NxNW.org/~steve/
> --
> AppArmor mailing list
> [email protected]
> Modify settings or unsubscribe at:
> https://lists.ubuntu.com/mailman/listinfo/apparmor
signature.asc
Description: Digital signature
-- AppArmor mailing list [email protected] Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor
