From ae1940d294e297d5d801f9f14f1809bfaab90e4d Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Mon, 29 Oct 2018 19:23:49 +0100 Subject: [PATCH 01/17] btrfs-util: before deleting a subvol check that it is one This has the benefit that we can return ENOTTY rather than EPERM if we are attempting to delete a subvol and don't have the privs to. --- src/basic/btrfs-util.c | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/src/basic/btrfs-util.c b/src/basic/btrfs-util.c index 89800c5d61..48e819a7cb 100644 --- a/src/basic/btrfs-util.c +++ b/src/basic/btrfs-util.c @@ -1178,6 +1178,18 @@ static int subvol_remove_children(int fd, const char *subvolume, uint64_t subvol if (subvol_fd < 0) return -errno; + /* Let's check if this is actually a subvolume. Note that this is mostly redundant, as BTRFS_IOC_SNAP_DESTROY + * would fail anyway if it is not. However, it's a good thing to check this ahead of time so that we can return + * ENOTTY unconditionally in this case. This is different from the ioctl() which will return EPERM/EACCES if we + * don't have the privileges to remove subvolumes, regardless if the specified directory is actually a + * subvolume or not. In order to make it easy for callers to cover the "this is not a btrfs subvolume" case + * let's prefer ENOTTY over EPERM/EACCES though. */ + r = btrfs_is_subvol_fd(subvol_fd); + if (r < 0) + return r; + if (r == 0) /* Not a btrfs subvolume */ + return -ENOTTY; + if (subvol_id == 0) { r = btrfs_subvol_get_id_fd(subvol_fd, &subvol_id); if (r < 0) From 647687141ad36c67f1fe1405e7ca4f8b05ec6102 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 25 Oct 2018 13:48:25 +0200 Subject: [PATCH 02/17] tmpfiles: rename second parameter to GREEDY_REALLOC() 'allocated' We pretty much always name it like that, and it is very descriptive, hence let's stick to that nomenclature here. --- src/tmpfiles/tmpfiles.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/tmpfiles/tmpfiles.c b/src/tmpfiles/tmpfiles.c index 7819d96c67..bad7fb6063 100644 --- a/src/tmpfiles/tmpfiles.c +++ b/src/tmpfiles/tmpfiles.c @@ -136,7 +136,7 @@ typedef struct Item { typedef struct ItemArray { Item *items; size_t count; - size_t size; + size_t allocated; } ItemArray; typedef enum DirectoryType { @@ -2795,7 +2795,7 @@ static int parse_line(const char *fname, unsigned line, const char *buffer, bool return log_oom(); } - if (!GREEDY_REALLOC(existing->items, existing->size, existing->count + 1)) + if (!GREEDY_REALLOC(existing->items, existing->allocated, existing->count + 1)) return log_oom(); memcpy(existing->items + existing->count++, &i, sizeof(i)); From 96d10d7837303fc4e08f810e617739f6517ba275 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 25 Oct 2018 13:51:01 +0200 Subject: [PATCH 03/17] =?UTF-8?q?tmpfiles:=20rename=20"count"=20=E2=86=92?= =?UTF-8?q?=20"n=5Fitems"?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit "count" is so very generic. Let's follow our usual naming logic here, and rename this to "n_items", to make clear what we count here. --- src/tmpfiles/tmpfiles.c | 29 ++++++++++++++++------------- 1 file changed, 16 insertions(+), 13 deletions(-) diff --git a/src/tmpfiles/tmpfiles.c b/src/tmpfiles/tmpfiles.c index bad7fb6063..945c5df991 100644 --- a/src/tmpfiles/tmpfiles.c +++ b/src/tmpfiles/tmpfiles.c @@ -135,7 +135,7 @@ typedef struct Item { typedef struct ItemArray { Item *items; - size_t count; + size_t n_items; size_t allocated; } ItemArray; @@ -352,9 +352,9 @@ static struct Item* find_glob(OrderedHashmap *h, const char *match) { Iterator i; ORDERED_HASHMAP_FOREACH(j, h, i) { - unsigned n; + size_t n; - for (n = 0; n < j->count; n++) { + for (n = 0; n < j->n_items; n++) { Item *item = j->items + n; if (fnmatch(item->path, match, FNM_PATHNAME|FNM_PERIOD) == 0) @@ -2286,12 +2286,14 @@ static int process_item(Item *i) { } static int process_item_array(ItemArray *array) { - unsigned n; - int r = 0, k; + int r = 0; + size_t n; assert(array); - for (n = 0; n < array->count; n++) { + for (n = 0; n < array->n_items; n++) { + int k; + k = process_item(array->items + n); if (k < 0 && r == 0) r = k; @@ -2313,13 +2315,14 @@ static void item_free_contents(Item *i) { } static void item_array_free(ItemArray *a) { - unsigned n; + size_t n; if (!a) return; - for (n = 0; n < a->count; n++) + for (n = 0; n < a->n_items; n++) item_free_contents(a->items + n); + free(a->items); free(a); } @@ -2776,9 +2779,9 @@ static int parse_line(const char *fname, unsigned line, const char *buffer, bool existing = ordered_hashmap_get(h, i.path); if (existing) { - unsigned n; + size_t n; - for (n = 0; n < existing->count; n++) { + for (n = 0; n < existing->n_items; n++) { if (!item_compatible(existing->items + n, &i)) { log_notice("[%s:%u] Duplicate line for path \"%s\", ignoring.", fname, line, i.path); @@ -2795,13 +2798,13 @@ static int parse_line(const char *fname, unsigned line, const char *buffer, bool return log_oom(); } - if (!GREEDY_REALLOC(existing->items, existing->allocated, existing->count + 1)) + if (!GREEDY_REALLOC(existing->items, existing->allocated, existing->n_items + 1)) return log_oom(); - memcpy(existing->items + existing->count++, &i, sizeof(i)); + memcpy(existing->items + existing->n_items++, &i, sizeof(i)); /* Sort item array, to enforce stable ordering of application */ - typesafe_qsort(existing->items, existing->count, item_compare); + typesafe_qsort(existing->items, existing->n_items, item_compare); zero(i); return 0; From 7ab7529d45ee9c780f4fd7a974652043c4b504ce Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 25 Oct 2018 13:53:07 +0200 Subject: [PATCH 04/17] tmpfiles: why memset() and memcpy() if we have '=' and structured initialization? --- src/tmpfiles/tmpfiles.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/tmpfiles/tmpfiles.c b/src/tmpfiles/tmpfiles.c index 945c5df991..b1e85e2232 100644 --- a/src/tmpfiles/tmpfiles.c +++ b/src/tmpfiles/tmpfiles.c @@ -2801,12 +2801,12 @@ static int parse_line(const char *fname, unsigned line, const char *buffer, bool if (!GREEDY_REALLOC(existing->items, existing->allocated, existing->n_items + 1)) return log_oom(); - memcpy(existing->items + existing->n_items++, &i, sizeof(i)); + existing->items[existing->n_items++] = i; + i = (struct Item) {}; /* Sort item array, to enforce stable ordering of application */ typesafe_qsort(existing->items, existing->n_items, item_compare); - zero(i); return 0; } From 81fa4479f8bb469959c93f7aa15c9331d9683240 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 25 Oct 2018 13:57:32 +0200 Subject: [PATCH 05/17] tmpfiles: use free_and_replace() where appropriate --- src/tmpfiles/tmpfiles.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/tmpfiles/tmpfiles.c b/src/tmpfiles/tmpfiles.c index b1e85e2232..67060e57c1 100644 --- a/src/tmpfiles/tmpfiles.c +++ b/src/tmpfiles/tmpfiles.c @@ -2471,8 +2471,7 @@ static int patch_var_run(const char *fname, unsigned line, char **path) { log_notice("[%s:%u] Line references path below legacy directory /var/run/, updating %s → %s; please update the tmpfiles.d/ drop-in file accordingly.", fname, line, *path, n); - free(*path); - *path = n; + free_and_replace(*path, n); return 0; } @@ -2709,8 +2708,7 @@ static int parse_line(const char *fname, unsigned line, const char *buffer, bool if (!p) return log_oom(); - free(i.path); - i.path = p; + free_and_replace(i.path, p); } if (!isempty(user) && !streq(user, "-")) { From ccd114f0f9d6bf9b11e5efc0127f7345f98356be Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 25 Oct 2018 13:57:44 +0200 Subject: [PATCH 06/17] tmpfiles: fix minor memory leak on error path --- src/tmpfiles/tmpfiles.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/tmpfiles/tmpfiles.c b/src/tmpfiles/tmpfiles.c index 67060e57c1..cc5cbc382f 100644 --- a/src/tmpfiles/tmpfiles.c +++ b/src/tmpfiles/tmpfiles.c @@ -2792,8 +2792,10 @@ static int parse_line(const char *fname, unsigned line, const char *buffer, bool return log_oom(); r = ordered_hashmap_put(h, i.path, existing); - if (r < 0) + if (r < 0) { + free(existing); return log_oom(); + } } if (!GREEDY_REALLOC(existing->items, existing->allocated, existing->n_items + 1)) From 1a967b6bb1f2f153f62908474ffce4873eaf59dc Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 25 Oct 2018 23:19:00 +0200 Subject: [PATCH 07/17] tmpfiles: replace the three arg_create/arg_remove/arg_clean booleans with a single bitmask No change in behaviour, just a bit of refactoring. --- src/tmpfiles/tmpfiles.c | 25 +++++++++++++++---------- 1 file changed, 15 insertions(+), 10 deletions(-) diff --git a/src/tmpfiles/tmpfiles.c b/src/tmpfiles/tmpfiles.c index cc5cbc382f..73e1271dad 100644 --- a/src/tmpfiles/tmpfiles.c +++ b/src/tmpfiles/tmpfiles.c @@ -65,6 +65,12 @@ * properly owned directories beneath /tmp, /var/tmp, /run, which are * volatile and hence need to be recreated on bootup. */ +typedef enum OperationMask { + OPERATION_CREATE = 1 << 0, + OPERATION_REMOVE = 1 << 1, + OPERATION_CLEAN = 1 << 2, +} OperationMask; + typedef enum ItemType { /* These ones take file names */ CREATE_FILE = 'f', @@ -149,9 +155,7 @@ typedef enum DirectoryType { static bool arg_cat_config = false; static bool arg_user = false; -static bool arg_create = false; -static bool arg_clean = false; -static bool arg_remove = false; +static OperationMask arg_operation = 0; static bool arg_boot = false; static bool arg_no_pager = false; @@ -2272,13 +2276,14 @@ static int process_item(Item *i) { if (chase_symlinks(i->path, NULL, CHASE_NO_AUTOFS, NULL) == -EREMOTE) return t; - r = arg_create ? create_item(i) : 0; - q = arg_remove ? remove_item(i) : 0; - p = arg_clean ? clean_item(i) : 0; + r = FLAGS_SET(arg_operation, OPERATION_CREATE) ? create_item(i) : 0; /* Failure can only be tolerated for create */ if (i->allow_failure) r = 0; + q = FLAGS_SET(arg_operation, OPERATION_REMOVE) ? remove_item(i) : 0; + p = FLAGS_SET(arg_operation, OPERATION_CLEAN) ? clean_item(i) : 0; + return t < 0 ? t : r < 0 ? r : q < 0 ? q : @@ -2910,15 +2915,15 @@ static int parse_argv(int argc, char *argv[]) { break; case ARG_CREATE: - arg_create = true; + arg_operation |= OPERATION_CREATE; break; case ARG_CLEAN: - arg_clean = true; + arg_operation |= OPERATION_CLEAN; break; case ARG_REMOVE: - arg_remove = true; + arg_operation |= OPERATION_REMOVE; break; case ARG_BOOT: @@ -2962,7 +2967,7 @@ static int parse_argv(int argc, char *argv[]) { assert_not_reached("Unhandled option"); } - if (!arg_clean && !arg_create && !arg_remove && !arg_cat_config) { + if (arg_operation == 0 && !arg_cat_config) { log_error("You need to specify at least one of --clean, --create or --remove."); return -EINVAL; } From 599ebe29a3919d0155d1b701efde3f24724e3348 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 25 Oct 2018 23:22:18 +0200 Subject: [PATCH 08/17] tmpfiles: instead of accessing global 'arg_operation' pass it through the stack Just some refactoring, no change in behaviour. --- src/tmpfiles/tmpfiles.c | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/src/tmpfiles/tmpfiles.c b/src/tmpfiles/tmpfiles.c index 73e1271dad..016693c813 100644 --- a/src/tmpfiles/tmpfiles.c +++ b/src/tmpfiles/tmpfiles.c @@ -2243,9 +2243,9 @@ static int clean_item(Item *i) { } } -static int process_item_array(ItemArray *array); +static int process_item_array(ItemArray *array, OperationMask operation); -static int process_item(Item *i) { +static int process_item(Item *i, OperationMask operation) { int r, q, p, t = 0; _cleanup_free_ char *prefix = NULL; @@ -2267,7 +2267,7 @@ static int process_item(Item *i) { if (j) { int s; - s = process_item_array(j); + s = process_item_array(j, operation); if (s < 0 && t == 0) t = s; } @@ -2276,13 +2276,13 @@ static int process_item(Item *i) { if (chase_symlinks(i->path, NULL, CHASE_NO_AUTOFS, NULL) == -EREMOTE) return t; - r = FLAGS_SET(arg_operation, OPERATION_CREATE) ? create_item(i) : 0; + r = FLAGS_SET(operation, OPERATION_CREATE) ? create_item(i) : 0; /* Failure can only be tolerated for create */ if (i->allow_failure) r = 0; - q = FLAGS_SET(arg_operation, OPERATION_REMOVE) ? remove_item(i) : 0; - p = FLAGS_SET(arg_operation, OPERATION_CLEAN) ? clean_item(i) : 0; + q = FLAGS_SET(operation, OPERATION_REMOVE) ? remove_item(i) : 0; + p = FLAGS_SET(operation, OPERATION_CLEAN) ? clean_item(i) : 0; return t < 0 ? t : r < 0 ? r : @@ -2290,7 +2290,7 @@ static int process_item(Item *i) { p; } -static int process_item_array(ItemArray *array) { +static int process_item_array(ItemArray *array, OperationMask operation) { int r = 0; size_t n; @@ -2299,7 +2299,7 @@ static int process_item_array(ItemArray *array) { for (n = 0; n < array->n_items; n++) { int k; - k = process_item(array->items + n); + k = process_item(array->items + n, operation); if (k < 0 && r == 0) r = k; } @@ -3189,7 +3189,7 @@ int main(int argc, char *argv[]) { /* The non-globbing ones usually create things, hence we apply * them first */ ORDERED_HASHMAP_FOREACH(a, items, iterator) { - k = process_item_array(a); + k = process_item_array(a, arg_operation); if (k < 0 && r_process == 0) r_process = k; } @@ -3197,7 +3197,7 @@ int main(int argc, char *argv[]) { /* The globbing ones usually alter things, hence we apply them * second. */ ORDERED_HASHMAP_FOREACH(a, globs, iterator) { - k = process_item_array(a); + k = process_item_array(a, arg_operation); if (k < 0 && r_process == 0) r_process = k; } From 133bbca42e0ea4056e2151a1f5696b9ca8b6630b Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 26 Oct 2018 10:32:48 +0200 Subject: [PATCH 09/17] tmpfiles: no need to set zero initialization of first enum value --- src/tmpfiles/tmpfiles.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/tmpfiles/tmpfiles.c b/src/tmpfiles/tmpfiles.c index 016693c813..f7f72e9d0d 100644 --- a/src/tmpfiles/tmpfiles.c +++ b/src/tmpfiles/tmpfiles.c @@ -146,7 +146,7 @@ typedef struct ItemArray { } ItemArray; typedef enum DirectoryType { - DIRECTORY_RUNTIME = 0, + DIRECTORY_RUNTIME, DIRECTORY_STATE, DIRECTORY_CACHE, DIRECTORY_LOGS, From 811a15877825da9e53f9a2a8603da34589af6bbb Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 26 Oct 2018 20:56:37 +0200 Subject: [PATCH 10/17] tmpfiles: create parents before children, but remove children before parents Previously, we'd always process parents before children. With this change we are a bit more careful and create parents before children but clean up children before parents. The "done" boolean by item is replaced by a mask so that we can descent and ascend for the right operations only. See: #9508 --- src/tmpfiles/tmpfiles.c | 106 ++++++++++++++++++++++++++++++---------- 1 file changed, 79 insertions(+), 27 deletions(-) diff --git a/src/tmpfiles/tmpfiles.c b/src/tmpfiles/tmpfiles.c index f7f72e9d0d..2b38068d58 100644 --- a/src/tmpfiles/tmpfiles.c +++ b/src/tmpfiles/tmpfiles.c @@ -136,13 +136,16 @@ typedef struct Item { bool allow_failure:1; - bool done:1; + OperationMask done; } Item; typedef struct ItemArray { Item *items; size_t n_items; size_t allocated; + + struct ItemArray *parent; + Set *children; } ItemArray; typedef enum DirectoryType { @@ -2243,38 +2246,20 @@ static int clean_item(Item *i) { } } -static int process_item_array(ItemArray *array, OperationMask operation); - static int process_item(Item *i, OperationMask operation) { - int r, q, p, t = 0; - _cleanup_free_ char *prefix = NULL; + OperationMask todo; + int r, q, p; assert(i); - if (i->done) + todo = operation & ~i->done; + if (todo == 0) /* Everything already done? */ return 0; - i->done = true; - - prefix = malloc(strlen(i->path) + 1); - if (!prefix) - return log_oom(); - - PATH_FOREACH_PREFIX(prefix, i->path) { - ItemArray *j; - - j = ordered_hashmap_get(items, prefix); - if (j) { - int s; - - s = process_item_array(j, operation); - if (s < 0 && t == 0) - t = s; - } - } + i->done |= operation; if (chase_symlinks(i->path, NULL, CHASE_NO_AUTOFS, NULL) == -EREMOTE) - return t; + return 0; r = FLAGS_SET(operation, OPERATION_CREATE) ? create_item(i) : 0; /* Failure can only be tolerated for create */ @@ -2284,8 +2269,7 @@ static int process_item(Item *i, OperationMask operation) { q = FLAGS_SET(operation, OPERATION_REMOVE) ? remove_item(i) : 0; p = FLAGS_SET(operation, OPERATION_CLEAN) ? clean_item(i) : 0; - return t < 0 ? t : - r < 0 ? r : + return r < 0 ? r : q < 0 ? q : p; } @@ -2296,6 +2280,24 @@ static int process_item_array(ItemArray *array, OperationMask operation) { assert(array); + /* Create any parent first. */ + if (FLAGS_SET(operation, OPERATION_CREATE) && array->parent) + r = process_item_array(array->parent, operation & OPERATION_CREATE); + + /* Clean up all children first */ + if ((operation & (OPERATION_REMOVE|OPERATION_CLEAN)) && !set_isempty(array->children)) { + Iterator i; + ItemArray *c; + + SET_FOREACH(c, array->children, i) { + int k; + + k = process_item_array(c, operation & (OPERATION_REMOVE|OPERATION_CLEAN)); + if (k < 0 && r == 0) + r = k; + } + } + for (n = 0; n < array->n_items; n++) { int k; @@ -2328,6 +2330,7 @@ static void item_array_free(ItemArray *a) { for (n = 0; n < a->n_items; n++) item_free_contents(a->items + n); + set_free(a->children); free(a->items); free(a); } @@ -3117,6 +3120,43 @@ static int read_config_files(char **config_dirs, char **args, bool *invalid_conf return 0; } +static int link_parent(ItemArray *a) { + const char *path; + char *prefix; + int r; + + assert(a); + + /* Finds the closestq "parent" item array for the specified item array. Then registers the specified item array + * as child of it, and fills the parent in, linking them both ways. This allows us to later create parents + * before their children, and clean up/remove children before their parents. */ + + if (a->n_items <= 0) + return 0; + + path = a->items[0].path; + prefix = alloca(strlen(path) + 1); + PATH_FOREACH_PREFIX(prefix, path) { + ItemArray *j; + + j = ordered_hashmap_get(items, prefix); + if (j) { + r = set_ensure_allocated(&j->children, NULL); + if (r < 0) + return log_oom(); + + r = set_put(j->children, a); + if (r < 0) + return log_oom(); + + a->parent = j; + return 1; + } + } + + return 0; +} + int main(int argc, char *argv[]) { int r, k, r_process = 0; ItemArray *a; @@ -3186,6 +3226,18 @@ int main(int argc, char *argv[]) { if (r < 0) goto finish; + /* Let's now link up all child/parent relationships */ + ORDERED_HASHMAP_FOREACH(a, items, iterator) { + r = link_parent(a); + if (r < 0) + goto finish; + } + ORDERED_HASHMAP_FOREACH(a, globs, iterator) { + r = link_parent(a); + if (r < 0) + goto finish; + } + /* The non-globbing ones usually create things, hence we apply * them first */ ORDERED_HASHMAP_FOREACH(a, items, iterator) { From 21af33863fcfc9f3699d12ce4c65ee52807fefe8 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 26 Oct 2018 21:07:38 +0200 Subject: [PATCH 11/17] tmpfiles: log when we skip an entry due to autofs --- src/tmpfiles/tmpfiles.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/tmpfiles/tmpfiles.c b/src/tmpfiles/tmpfiles.c index 2b38068d58..af8c12e06e 100644 --- a/src/tmpfiles/tmpfiles.c +++ b/src/tmpfiles/tmpfiles.c @@ -2258,8 +2258,12 @@ static int process_item(Item *i, OperationMask operation) { i->done |= operation; - if (chase_symlinks(i->path, NULL, CHASE_NO_AUTOFS, NULL) == -EREMOTE) + r = chase_symlinks(i->path, NULL, CHASE_NO_AUTOFS, NULL); + if (r == -EREMOTE) { + log_debug_errno(r, "Item '%s' is behind autofs, skipping.", i->path); return 0; + } else if (r < 0) + log_debug_errno(r, "Failed to determine whether '%s' is behind autofs, ignoring: %m", i->path); r = FLAGS_SET(operation, OPERATION_CREATE) ? create_item(i) : 0; /* Failure can only be tolerated for create */ From 64adb37968748d6708e209919a6c1e176a9dd81b Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 26 Oct 2018 21:19:36 +0200 Subject: [PATCH 12/17] tmpfiles: always remove/clean-up before creating Let's always clean the platform before we build something new. Fixes: #9508 --- src/tmpfiles/tmpfiles.c | 46 +++++++++++++++++++++++++++-------------- 1 file changed, 30 insertions(+), 16 deletions(-) diff --git a/src/tmpfiles/tmpfiles.c b/src/tmpfiles/tmpfiles.c index af8c12e06e..ce9146c1d1 100644 --- a/src/tmpfiles/tmpfiles.c +++ b/src/tmpfiles/tmpfiles.c @@ -3162,11 +3162,11 @@ static int link_parent(ItemArray *a) { } int main(int argc, char *argv[]) { - int r, k, r_process = 0; - ItemArray *a; - Iterator iterator; _cleanup_strv_free_ char **config_dirs = NULL; + int r, k, r_process = 0, phase; bool invalid_config = false; + Iterator iterator; + ItemArray *a; r = parse_argv(argc, argv); if (r <= 0) @@ -3242,20 +3242,34 @@ int main(int argc, char *argv[]) { goto finish; } - /* The non-globbing ones usually create things, hence we apply - * them first */ - ORDERED_HASHMAP_FOREACH(a, items, iterator) { - k = process_item_array(a, arg_operation); - if (k < 0 && r_process == 0) - r_process = k; - } + /* If multiple operations are requested, let's first run the remove/clean operations, and only then the create + * operations. i.e. that we first clean out the platform we then build on. */ + for (phase = 0; phase < 2; phase++) { + OperationMask op; - /* The globbing ones usually alter things, hence we apply them - * second. */ - ORDERED_HASHMAP_FOREACH(a, globs, iterator) { - k = process_item_array(a, arg_operation); - if (k < 0 && r_process == 0) - r_process = k; + if (phase == 0) + op = arg_operation & (OPERATION_REMOVE|OPERATION_CLEAN); + else if (phase == 1) + op = arg_operation & OPERATION_CREATE; + else + assert_not_reached("unexpected phase"); + + if (op == 0) /* Nothing requested in this phase */ + continue; + + /* The non-globbing ones usually create things, hence we apply them first */ + ORDERED_HASHMAP_FOREACH(a, items, iterator) { + k = process_item_array(a, op); + if (k < 0 && r_process == 0) + r_process = k; + } + + /* The globbing ones usually alter things, hence we apply them second. */ + ORDERED_HASHMAP_FOREACH(a, globs, iterator) { + k = process_item_array(a, op); + if (k < 0 && r_process == 0) + r_process = k; + } } finish: From 2c575977459f815f7ca833a3ffb37286dc20b125 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Mon, 29 Oct 2018 19:24:56 +0100 Subject: [PATCH 13/17] tmpfiles: 'D' doesn't do globs for creation, shouldn't do for removal either --- src/tmpfiles/tmpfiles.c | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-) diff --git a/src/tmpfiles/tmpfiles.c b/src/tmpfiles/tmpfiles.c index ce9146c1d1..871beb9043 100644 --- a/src/tmpfiles/tmpfiles.c +++ b/src/tmpfiles/tmpfiles.c @@ -2138,12 +2138,10 @@ static int remove_item_instance(Item *i, const char *instance) { break; - case TRUNCATE_DIRECTORY: case RECURSIVE_REMOVE_PATH: - /* FIXME: we probably should use dir_cleanup() here - * instead of rm_rf() so that 'x' is honoured. */ + /* 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, (i->type == RECURSIVE_REMOVE_PATH ? REMOVE_ROOT|REMOVE_SUBVOLUME : 0) | REMOVE_PHYSICAL); + 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); @@ -2157,14 +2155,24 @@ static int remove_item_instance(Item *i, const char *instance) { } static int remove_item(Item *i) { + int r; + assert(i); log_debug("Running remove action for entry %c %s", (char) i->type, i->path); switch (i->type) { - case REMOVE_PATH: 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; + + case REMOVE_PATH: case RECURSIVE_REMOVE_PATH: return glob_item(i, remove_item_instance); From f8ed99c845fc79ca5ca7b1e949e45f7fc0a66559 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Mon, 29 Oct 2018 19:34:18 +0100 Subject: [PATCH 14/17] TEST-22: extend test suite a bit Let's add a test based on #9508 --- test/TEST-22-TMPFILES/test-06.sh | 38 ++++++++++++++++++++++++++++++++ 1 file changed, 38 insertions(+) create mode 100755 test/TEST-22-TMPFILES/test-06.sh diff --git a/test/TEST-22-TMPFILES/test-06.sh b/test/TEST-22-TMPFILES/test-06.sh new file mode 100755 index 0000000000..cd65ba6726 --- /dev/null +++ b/test/TEST-22-TMPFILES/test-06.sh @@ -0,0 +1,38 @@ +#! /bin/bash +# +# Inspired by https://github.com/systemd/systemd/issues/9508 +# + +set -e + +test_snippet() { + systemd-tmpfiles "$@" - < Date: Mon, 29 Oct 2018 19:35:40 +0100 Subject: [PATCH 15/17] man: stop mentioning /var/run in tmpfiles.d(5) It's obsolete, stop mentioning it. Let's not confuse people suggests it would be OK to use that, because it really isn't anymore, and it gives us trouble with merging idenctical lines. --- man/tmpfiles.d.xml | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/man/tmpfiles.d.xml b/man/tmpfiles.d.xml index 49a5bdd166..7d7f977979 100644 --- a/man/tmpfiles.d.xml +++ b/man/tmpfiles.d.xml @@ -46,12 +46,9 @@ directories which usually reside in directories such as /run or /tmp. - Volatile and temporary files and directories are those - located in /run (and its alias - /var/run), /tmp, - /var/tmp, the API file systems such as - /sys or /proc, as well - as some other directories below /var. + Volatile and temporary files and directories are those located in /run, + /tmp, /var/tmp, the API file systems such as /sys or + /proc, as well as some other directories below /var. System daemons frequently require private runtime directories below /run to place communication From bdee3f5580540c2ae7e964bae5c2d21a736f8b34 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Mon, 29 Oct 2018 19:40:42 +0100 Subject: [PATCH 16/17] man: document that removal/clean-up is done before creation in systemd-tmpfiles --- man/systemd-tmpfiles.xml | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/man/systemd-tmpfiles.xml b/man/systemd-tmpfiles.xml index 9ff114feef..9978d95cab 100644 --- a/man/systemd-tmpfiles.xml +++ b/man/systemd-tmpfiles.xml @@ -176,14 +176,12 @@ - It is possible to combine , - , and in one - invocation. For example, during boot the following command line is - executed to ensure that all temporary and volatile directories are + It is possible to combine , , and + in one invocation (in which case removal and clean-up are executed before creation of new files). For example, + during boot the following command line is executed to ensure that all temporary and volatile directories are removed and created according to the configuration file: systemd-tmpfiles --remove --create - From ad19c57898b96ecae39990e849a865db5b94fe4d Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Mon, 29 Oct 2018 19:41:59 +0100 Subject: [PATCH 17/17] man: document that for removal tmpfiles.d prefix is run after suffix --- man/tmpfiles.d.xml | 36 ++++++++++++++---------------------- 1 file changed, 14 insertions(+), 22 deletions(-) diff --git a/man/tmpfiles.d.xml b/man/tmpfiles.d.xml index 7d7f977979..71dcfe870f 100644 --- a/man/tmpfiles.d.xml +++ b/man/tmpfiles.d.xml @@ -67,28 +67,20 @@ The second variant should be used when it is desirable to make it easy to override just this part of configuration. - Files in /etc/tmpfiles.d override files - with the same name in /usr/lib/tmpfiles.d and - /run/tmpfiles.d. Files in - /run/tmpfiles.d override files with the same - name in /usr/lib/tmpfiles.d. Packages should - install their configuration files in - /usr/lib/tmpfiles.d. Files in - /etc/tmpfiles.d are reserved for the local - administrator, who may use this logic to override the - configuration files installed by vendor packages. All - configuration files are sorted by their filename in lexicographic - order, regardless of which of the directories they reside in. If - multiple files specify the same path, the entry in the file with - the lexicographically earliest name will be applied. All other - conflicting entries will be logged as errors. When two lines are - prefix and suffix of each other, then the prefix is always - processed first, the suffix later. Lines that take globs are - applied after those accepting no globs. If multiple operations - shall be applied on the same file, (such as ACL, xattr, file - attribute adjustments), these are always done in the same fixed - order. Otherwise, the files/directories are processed in the order - they are listed. + Files in /etc/tmpfiles.d override files with the same name in + /usr/lib/tmpfiles.d and /run/tmpfiles.d. Files in + /run/tmpfiles.d override files with the same name in + /usr/lib/tmpfiles.d. Packages should install their configuration files in + /usr/lib/tmpfiles.d. Files in /etc/tmpfiles.d are reserved for the local + administrator, who may use this logic to override the configuration files installed by vendor packages. All + configuration files are sorted by their filename in lexicographic order, regardless of which of the directories + they reside in. If multiple files specify the same path, the entry in the file with the lexicographically earliest + name will be applied. All other conflicting entries will be logged as errors. When two lines are prefix path and + suffix path of each other, then the prefix line is always created first, the suffix later (and if removal applies + to the line, the order is reversed: the suffix is removed first, the prefix later). Lines that take globs are + applied after those accepting no globs. If multiple operations shall be applied on the same file, (such as ACL, + xattr, file attribute adjustments), these are always done in the same fixed order. Otherwise, the files/directories + are processed in the order they are listed. If the administrator wants to disable a configuration file supplied by the vendor, the recommended way is to place a symlink