From b1935cc9430dfb6a28ed9b9881c641194d1fd587 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Thu, 7 Dec 2023 13:01:27 +0100 Subject: [PATCH] tmpfiles: use dir_cleanup() for R and D ... i.e. apply nested config (exclusions and such) when executing R and D. This fixes a long-standing RFE. The existing logic seems to have been an accident of implementation. After all, if somebody specifies a config with 'R /foo; x /tmp/bar', then probably the goal is to remove stuff from under /foo, but keep /tmp/bar. If they just wanted to nuke everything, then would not specify the second item. This also makes R and D use O_NOATIME, i.e. the access times of the directories that are accessed will not be changed by the cleanup. Obviously, we'll have to add this to NEWS and such. Looking at the whole tmpfiles.d config in Fedora, this change has no effect. The test cases are adjusted as appropriate. I also added another test case for 'R'/'D' with a file, just to test this code path more. Replaces #20641. Fixes #1633. --- TODO | 1 - man/tmpfiles.d.xml | 8 +--- src/tmpfiles/tmpfiles.c | 80 +++++++++++++++++++++-------------- test/units/testsuite-22.11.sh | 28 +++++++++--- 4 files changed, 74 insertions(+), 43 deletions(-) diff --git a/TODO b/TODO index 1be6f2e4b31..07be5d932ca 100644 --- a/TODO +++ b/TODO @@ -2446,7 +2446,6 @@ Features: * support crash reporting operation modes (https://live.gnome.org/GnomeOS/Design/Whiteboards/ProblemReporting) * tmpfiles: - - apply "x" on "D" too (see patch from William Douglas) - allow time-based cleanup in r and R too - instead of ignoring unknown fields, reject them. - creating new directories/subvolumes/fifos/device nodes diff --git a/man/tmpfiles.d.xml b/man/tmpfiles.d.xml index f8bdffcae33..76807b92e5c 100644 --- a/man/tmpfiles.d.xml +++ b/man/tmpfiles.d.xml @@ -353,9 +353,7 @@ L /tmp/foobar - - - - /dev/null x Ignore a path during cleaning. Use this type to exclude paths from clean-up as controlled with the Age - parameter. Note that lines of this type do not influence the - effect of r or R - lines. Lines of this type accept shell-style globs in place + parameter. Lines of this type accept shell-style globs in place of normal path names. @@ -365,9 +363,7 @@ L /tmp/foobar - - - - /dev/null to exclude paths from clean-up as controlled with the Age parameter. Unlike x, this parameter will not exclude the content if path is a directory, but only - directory itself. Note that lines of this type do not - influence the effect of r or - R lines. Lines of this type accept + directory itself. Lines of this type accept shell-style globs in place of normal path names. diff --git a/src/tmpfiles/tmpfiles.c b/src/tmpfiles/tmpfiles.c index e877a5117aa..f31235921e8 100644 --- a/src/tmpfiles/tmpfiles.c +++ b/src/tmpfiles/tmpfiles.c @@ -904,7 +904,7 @@ static int dir_cleanup( } finish: - if (deleted) { + if (deleted && (self_atime_nsec < NSEC_INFINITY || self_mtime_nsec < NSEC_INFINITY)) { struct timespec ts[2]; log_debug("Restoring access and modification time on \"%s\": %s, %s", @@ -2907,26 +2907,62 @@ static int create_item(Context *c, Item *i) { return 0; } -static int purge_item_instance(Context *c, Item *i, const char *instance, CreationMode creation) { +static int remove_recursive( + Context *c, + Item *i, + const char *instance, + bool remove_instance) { + + _cleanup_closedir_ DIR *d = NULL; + STRUCT_STATX_DEFINE(sx); + bool mountpoint; int r; - /* FIXME: we probably should use dir_cleanup() here instead of rm_rf() so that 'x' is honoured. */ - log_debug("rm -rf \"%s\"", instance); - r = rm_rf(instance, REMOVE_ROOT|REMOVE_SUBVOLUME|REMOVE_PHYSICAL); - if (r < 0 && r != -ENOENT) - return log_error_errno(r, "rm_rf(%s): %m", instance); + r = opendir_and_stat(instance, &d, &sx, &mountpoint); + if (r < 0) + return r; + if (r == 0) { + if (remove_instance) { + log_debug("Removing file \"%s\".", instance); + if (remove(instance) < 0 && errno != ENOENT) + return log_error_errno(errno, "rm %s: %m", instance); + } + return 0; + } + r = dir_cleanup(c, i, instance, d, + /* self_atime_nsec= */ NSEC_INFINITY, + /* self_mtime_nsec= */ NSEC_INFINITY, + /* cutoff_nsec= */ NSEC_INFINITY, + sx.stx_dev_major, sx.stx_dev_minor, + mountpoint, + MAX_DEPTH, + /* keep_this_level= */ false, + /* age_by_file= */ 0, + /* age_by_dir= */ 0); + if (r < 0) + return r; + + if (remove_instance) { + log_debug("Removing directory \"%s\".", instance); + r = RET_NERRNO(rmdir(instance)); + if (r < 0 && !IN_SET(r, -ENOENT, -ENOTEMPTY)) + return log_error_errno(r, "Failed to remove %s: %m", instance); + } return 0; } -static int purge_item(Context *c, Item *i) { +static int purge_item_instance(Context *c, Item *i, const char *instance, CreationMode creation) { + return remove_recursive(c, i, instance, /* remove_instance= */ true); +} +static int purge_item(Context *c, Item *i) { assert(i); if (!needs_purge(i->type)) return 0; - log_debug("Running purge owned action for entry %c %s", (char) i->type, i->path); + log_debug("Running purge action for entry %c %s", (char) i->type, i->path); if (needs_glob(i->type)) return glob_item(c, i, purge_item_instance); @@ -2940,8 +2976,6 @@ static int remove_item_instance( const char *instance, CreationMode creation) { - int r; - assert(c); assert(i); @@ -2949,29 +2983,19 @@ static int remove_item_instance( case REMOVE_PATH: if (remove(instance) < 0 && errno != ENOENT) - return log_error_errno(errno, "rm(%s): %m", instance); + return log_error_errno(errno, "rm %s: %m", instance); - break; + return 0; case RECURSIVE_REMOVE_PATH: - /* FIXME: we probably should use dir_cleanup() here instead of rm_rf() so that 'x' is honoured. */ - log_debug("rm -rf \"%s\"", instance); - r = rm_rf(instance, REMOVE_ROOT|REMOVE_SUBVOLUME|REMOVE_PHYSICAL); - if (r < 0 && r != -ENOENT) - return log_error_errno(r, "rm_rf(%s): %m", instance); - - break; + return remove_recursive(c, i, instance, /* remove_instance= */ true); default: assert_not_reached(); } - - return 0; } static int remove_item(Context *c, Item *i) { - int r; - assert(c); assert(i); @@ -2980,13 +3004,7 @@ static int remove_item(Context *c, Item *i) { switch (i->type) { case TRUNCATE_DIRECTORY: - /* FIXME: we probably should use dir_cleanup() here instead of rm_rf() so that 'x' is honoured. */ - log_debug("rm -rf \"%s\"", i->path); - r = rm_rf(i->path, REMOVE_PHYSICAL); - if (r < 0 && r != -ENOENT) - return log_error_errno(r, "rm_rf(%s): %m", i->path); - - return 0; + return remove_recursive(c, i, i->path, /* remove_instance= */ false); case REMOVE_PATH: case RECURSIVE_REMOVE_PATH: diff --git a/test/units/testsuite-22.11.sh b/test/units/testsuite-22.11.sh index f71a95f4f39..deee6fe5d1c 100755 --- a/test/units/testsuite-22.11.sh +++ b/test/units/testsuite-22.11.sh @@ -126,16 +126,34 @@ mkdir -p /tmp/x/{1,2}/a touch /tmp/x/1/a/{x1,x2} /tmp/x/2/a/{y1,y2} systemd-tmpfiles --remove - <