From 37ee46f7c859cb514592eb0ff8e11fc44316ef52 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Sun, 3 Dec 2023 13:47:19 +0100 Subject: [PATCH 01/18] sysusers,tmpfiles: clarify error message for --replace I was trying to run sysusers --replace, but the input file didn't have the right suffix, and the message was very confusing. Let's split the message in two to make it clearer that we care about the extension. --- src/sysusers/sysusers.c | 8 +++++--- src/tmpfiles/tmpfiles.c | 8 +++++--- 2 files changed, 10 insertions(+), 6 deletions(-) diff --git a/src/sysusers/sysusers.c b/src/sysusers/sysusers.c index 96fb6f8ad5..19284217f7 100644 --- a/src/sysusers/sysusers.c +++ b/src/sysusers/sysusers.c @@ -2140,10 +2140,12 @@ static int parse_argv(int argc, char *argv[]) { break; case ARG_REPLACE: - if (!path_is_absolute(optarg) || - !endswith(optarg, ".conf")) + if (!path_is_absolute(optarg)) return log_error_errno(SYNTHETIC_ERRNO(EINVAL), - "The argument to --replace= must an absolute path to a config file"); + "The argument to --replace= must be an absolute path."); + if (!endswith(optarg, ".conf")) + return log_error_errno(SYNTHETIC_ERRNO(EINVAL), + "The argument to --replace= must have the extension '.conf'."); arg_replace = optarg; break; diff --git a/src/tmpfiles/tmpfiles.c b/src/tmpfiles/tmpfiles.c index dbe9156f06..9fc8dad325 100644 --- a/src/tmpfiles/tmpfiles.c +++ b/src/tmpfiles/tmpfiles.c @@ -4167,10 +4167,12 @@ static int parse_argv(int argc, char *argv[]) { break; case ARG_REPLACE: - if (!path_is_absolute(optarg) || - !endswith(optarg, ".conf")) + if (!path_is_absolute(optarg)) return log_error_errno(SYNTHETIC_ERRNO(EINVAL), - "The argument to --replace= must an absolute path to a config file"); + "The argument to --replace= must be an absolute path."); + if (!endswith(optarg, ".conf")) + return log_error_errno(SYNTHETIC_ERRNO(EINVAL), + "The argument to --replace= must have the extension '.conf'."); arg_replace = optarg; break; From 5ac41a1672fd1562ed9d1605c215f3f4d88cc2d6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Sun, 3 Dec 2023 14:59:10 +0100 Subject: [PATCH 02/18] tmpfiles: adjust vertical whitespace --- src/tmpfiles/tmpfiles.c | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/src/tmpfiles/tmpfiles.c b/src/tmpfiles/tmpfiles.c index 9fc8dad325..e4e113b402 100644 --- a/src/tmpfiles/tmpfiles.c +++ b/src/tmpfiles/tmpfiles.c @@ -250,7 +250,13 @@ static bool ERRNO_IS_NOINFO(int r) { ENXIO); /* env var is unset */ } -static int specifier_directory(char specifier, const void *data, const char *root, const void *userdata, char **ret) { +static int specifier_directory( + char specifier, + const void *data, + const char *root, + const void *userdata, + char **ret) { + struct table_entry { uint64_t type; const char *suffix; @@ -1452,7 +1458,6 @@ static int path_set_acls( } static int parse_attribute_from_arg(Item *item) { - static const struct { char character; unsigned value; @@ -4031,7 +4036,6 @@ static int help(void) { } static int parse_argv(int argc, char *argv[]) { - enum { ARG_VERSION = 0x100, ARG_CAT_CONFIG, @@ -4371,7 +4375,6 @@ static int read_config_files( } static int read_credential_lines(Context *c, bool *invalid_config) { - _cleanup_free_ char *j = NULL; const char *d; int r; From 376d0495f5b9170435824cf9fb067c1077b6cc7e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Sun, 3 Dec 2023 15:28:52 +0100 Subject: [PATCH 03/18] tmpfiles: only populate uid and gid caches once a3451c2c4ce7d3c02451f6ace4ee9f873880f78f added offline uid/gid support in a way where the /etc/passwd and /etc/group would be read anew for each configuration file that was parsed. The result would always be the same, so I assume that this was an oversight. Let's use a global cache and and read the file just once. --- src/tmpfiles/tmpfiles.c | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-) diff --git a/src/tmpfiles/tmpfiles.c b/src/tmpfiles/tmpfiles.c index e4e113b402..4e245e3668 100644 --- a/src/tmpfiles/tmpfiles.c +++ b/src/tmpfiles/tmpfiles.c @@ -214,8 +214,11 @@ static ImagePolicy *arg_image_policy = NULL; #define MAX_DEPTH 256 typedef struct Context { - OrderedHashmap *items, *globs; + OrderedHashmap *items; + OrderedHashmap *globs; Set *unix_sockets; + Hashmap *uid_cache; + Hashmap *gid_cache; } Context; STATIC_DESTRUCTOR_REGISTER(arg_include_prefixes, strv_freep); @@ -239,6 +242,9 @@ static void context_done(Context *c) { ordered_hashmap_free(c->globs); set_free(c->unix_sockets); + + hashmap_free(c->uid_cache); + hashmap_free(c->gid_cache); } /* Different kinds of errors that mean that information is not available in the environment. */ @@ -3461,9 +3467,7 @@ static int parse_line( const char *fname, unsigned line, const char *buffer, - bool *invalid_config, - Hashmap **uid_cache, - Hashmap **gid_cache) { + bool *invalid_config) { _cleanup_free_ char *action = NULL, *mode = NULL, *user = NULL, *group = NULL, *age = NULL, *path = NULL; _cleanup_(item_free_contents) Item i = { @@ -3823,7 +3827,7 @@ static int parse_line( else u = user; - r = find_uid(u, &i.uid, uid_cache); + r = find_uid(u, &i.uid, &c->uid_cache); if (r == -ESRCH && arg_graceful) { log_syntax(NULL, LOG_DEBUG, fname, line, r, "%s: user '%s' not found, not adjusting ownership.", i.path, u); @@ -3844,7 +3848,7 @@ static int parse_line( else g = group; - r = find_gid(g, &i.gid, gid_cache); + r = find_gid(g, &i.gid, &c->gid_cache); if (r == -ESRCH && arg_graceful) { log_syntax(NULL, LOG_DEBUG, fname, line, r, "%s: group '%s' not found, not adjusting ownership.", i.path, g); @@ -4221,7 +4225,6 @@ static int read_config_file( bool ignore_enoent, bool *invalid_config) { - _cleanup_hashmap_free_ Hashmap *uid_cache = NULL, *gid_cache = NULL; _cleanup_fclose_ FILE *_f = NULL; _cleanup_free_ char *pp = NULL; unsigned v = 0; @@ -4268,7 +4271,7 @@ static int read_config_file( if (IN_SET(line[0], 0, '#')) continue; - k = parse_line(c, fn, v, line, &invalid_line, &uid_cache, &gid_cache); + k = parse_line(c, fn, v, line, &invalid_line); if (k < 0) { if (invalid_line) /* Allow reporting with a special code if the caller requested this */ From f6a1346e4e738f77afec5c10454edb833a502ba6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Sun, 3 Dec 2023 14:53:49 +0100 Subject: [PATCH 04/18] tmpfiles: split out helper to open and read a "config file" No functional change. Note that this function will be modified in subsequent commits, and the API will change. --- src/basic/conf-files.c | 82 +++++++++++++++++++++++++++++++++++++++++ src/basic/conf-files.h | 18 +++++++++ src/tmpfiles/tmpfiles.c | 66 ++++----------------------------- 3 files changed, 107 insertions(+), 59 deletions(-) diff --git a/src/basic/conf-files.c b/src/basic/conf-files.c index a56f82f8a3..d85d651525 100644 --- a/src/basic/conf-files.c +++ b/src/basic/conf-files.c @@ -9,7 +9,10 @@ #include "conf-files.h" #include "constants.h" #include "dirent-util.h" +#include "errno-util.h" #include "fd-util.h" +#include "fileio.h" +#include "glyph-util.h" #include "hashmap.h" #include "log.h" #include "macro.h" @@ -372,3 +375,82 @@ int conf_files_list_dropins( return conf_files_list_strv(ret, ".conf", root, 0, (const char* const*) dropin_dirs); } + +/** + * Open and read a config file. + * + * The argument may be: + * - '-', meaning stdin. + * - a non-absolute path. In this case are searched. + * - an absolute path. In this case is opened directly. + */ +int conf_file_read( + const char *root, + const char **config_dirs, + const char *fn, + parse_line_t parse_line, + void *userdata, + bool ignore_enoent, + bool *invalid_config) { + + _cleanup_fclose_ FILE *_f = NULL; + _cleanup_free_ char *_fn = NULL; + unsigned v = 0; + FILE *f; + int r; + + assert(fn); + + if (streq(fn, "-")) { + f = stdin; + fn = ""; + + log_debug("Reading config from stdin%s", special_glyph(SPECIAL_GLYPH_ELLIPSIS)); + + } else { + r = search_and_fopen(fn, "re", root, config_dirs, &_f, &_fn); + if (r == -ENOENT && ignore_enoent) { + log_debug_errno(r, "Failed to open \"%s\", ignoring: %m", fn); + return 0; /* No error, but nothing happened. */ + } + if (r < 0) + return log_error_errno(r, "Failed to read '%s': %m", fn); + + f = _f; + fn = _fn; + log_debug("Reading config file \"%s\"%s", fn, special_glyph(SPECIAL_GLYPH_ELLIPSIS)); + } + + r = 1; /* We entered the part where we may modify state. */ + + for (;;) { + _cleanup_free_ char *line = NULL; + bool invalid_line = false; + int k; + + k = read_stripped_line(f, LONG_LINE_MAX, &line); + if (k < 0) + return log_error_errno(k, "Failed to read '%s': %m", fn); + if (k == 0) + break; + + v++; + + if (IN_SET(line[0], 0, '#')) + continue; + + k = parse_line(fn, v, line, &invalid_line, userdata); + if (k < 0 && invalid_line) { + /* Allow reporting with a special code if the caller requested this. */ + if (invalid_config) + *invalid_config = true; + } else + /* The first error, if any, becomes our return value. */ + RET_GATHER(r, k); + } + + if (ferror(f)) + RET_GATHER(r, log_error_errno(SYNTHETIC_ERRNO(EIO), "Failed to read from file %s.", fn)); + + return r; +} diff --git a/src/basic/conf-files.h b/src/basic/conf-files.h index 566cc8fda4..cf89ee62c6 100644 --- a/src/basic/conf-files.h +++ b/src/basic/conf-files.h @@ -1,6 +1,8 @@ /* SPDX-License-Identifier: LGPL-2.1-or-later */ #pragma once +#include + #include "macro.h" enum { @@ -29,3 +31,19 @@ int conf_files_list_dropins( const char *dropin_dirname, const char *root, const char * const *dirs); + +typedef int parse_line_t( + const char *fname, + unsigned line, + const char *buffer, + bool *invalid_config, + void *userdata); + +int conf_file_read( + const char *root, + const char **config_dirs, + const char *fn, + parse_line_t parse_line, + void *userdata, + bool ignore_enoent, + bool *invalid_config); diff --git a/src/tmpfiles/tmpfiles.c b/src/tmpfiles/tmpfiles.c index 4e245e3668..a2eb05c6a6 100644 --- a/src/tmpfiles/tmpfiles.c +++ b/src/tmpfiles/tmpfiles.c @@ -3463,12 +3463,13 @@ static bool is_duplicated_item(ItemArray *existing, const Item *i) { } static int parse_line( - Context *c, const char *fname, unsigned line, const char *buffer, - bool *invalid_config) { + bool *invalid_config, + void *context) { + Context *c = ASSERT_PTR(context); _cleanup_free_ char *action = NULL, *mode = NULL, *user = NULL, *group = NULL, *age = NULL, *path = NULL; _cleanup_(item_free_contents) Item i = { /* The "age-by" argument considers all file timestamp types by default. */ @@ -3481,7 +3482,6 @@ static int parse_line( bool append_or_force = false, boot = false, allow_failure = false, try_replace = false, unbase64 = false, from_cred = false, missing_user_or_group = false; - assert(c); assert(fname); assert(line >= 1); assert(buffer); @@ -4225,62 +4225,16 @@ static int read_config_file( bool ignore_enoent, bool *invalid_config) { - _cleanup_fclose_ FILE *_f = NULL; - _cleanup_free_ char *pp = NULL; - unsigned v = 0; - FILE *f; ItemArray *ia; int r = 0; assert(c); assert(fn); - if (streq(fn, "-")) { - log_debug("Reading config from stdin%s", special_glyph(SPECIAL_GLYPH_ELLIPSIS)); - fn = ""; - f = stdin; - } else { - r = search_and_fopen(fn, "re", arg_root, (const char**) config_dirs, &_f, &pp); - if (r < 0) { - if (ignore_enoent && r == -ENOENT) { - log_debug_errno(r, "Failed to open \"%s\", ignoring: %m", fn); - return 0; - } - - return log_error_errno(r, "Failed to open '%s': %m", fn); - } - - log_debug("Reading config file \"%s\"%s", pp, special_glyph(SPECIAL_GLYPH_ELLIPSIS)); - fn = pp; - f = _f; - } - - for (;;) { - _cleanup_free_ char *line = NULL; - bool invalid_line = false; - int k; - - k = read_stripped_line(f, LONG_LINE_MAX, &line); - if (k < 0) - return log_error_errno(k, "Failed to read '%s': %m", fn); - if (k == 0) - break; - - v++; - - if (IN_SET(line[0], 0, '#')) - continue; - - k = parse_line(c, fn, v, line, &invalid_line); - if (k < 0) { - if (invalid_line) - /* Allow reporting with a special code if the caller requested this */ - *invalid_config = true; - else if (r == 0) - /* The first error becomes our return value */ - r = k; - } - } + r = conf_file_read(arg_root, (const char**) config_dirs, fn, + parse_line, c, ignore_enoent, invalid_config); + if (r <= 0) + return r; /* we have to determine age parameter for each entry of type X */ ORDERED_HASHMAP_FOREACH(ia, c->globs) @@ -4319,12 +4273,6 @@ static int read_config_file( } } - if (ferror(f)) { - log_error_errno(errno, "Failed to read from file %s: %m", fn); - if (r == 0) - r = -EIO; - } - return r; } From 15d660fb418a6017fdf416bd0f92b6e268543383 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Sun, 3 Dec 2023 16:34:15 +0100 Subject: [PATCH 05/18] sysusers: convert to conf_file_read() I was annoyed that systemd-sysusers doesn't print any info when it opens a config file. Its read_config_file() started out the same as the one in tmpfiles, and then they diverged. The one in tmpfiles has that logging, hence the rework to use it here too and get better logging. The two programs should provide similar functionality, so using a common helper will make it easier to extend them in tandem later. No functional change apart from the log info. The userdata argument (Context) is moved to the last position as requested in the review. --- src/basic/conf-files.c | 9 +++--- src/sysusers/sysusers.c | 69 +++++++++-------------------------------- 2 files changed, 18 insertions(+), 60 deletions(-) diff --git a/src/basic/conf-files.c b/src/basic/conf-files.c index d85d651525..c98e7d0d93 100644 --- a/src/basic/conf-files.c +++ b/src/basic/conf-files.c @@ -439,12 +439,11 @@ int conf_file_read( if (IN_SET(line[0], 0, '#')) continue; - k = parse_line(fn, v, line, &invalid_line, userdata); - if (k < 0 && invalid_line) { + k = parse_line(fn, v, line, invalid_config ? &invalid_line : NULL, userdata); + if (k < 0 && invalid_line) /* Allow reporting with a special code if the caller requested this. */ - if (invalid_config) - *invalid_config = true; - } else + *invalid_config = true; + else /* The first error, if any, becomes our return value. */ RET_GATHER(r, k); } diff --git a/src/sysusers/sysusers.c b/src/sysusers/sysusers.c index 19284217f7..8e90438ce0 100644 --- a/src/sysusers/sysusers.c +++ b/src/sysusers/sysusers.c @@ -1680,11 +1680,13 @@ static int item_equivalent(Item *a, Item *b) { } static int parse_line( - Context *c, const char *fname, unsigned line, - const char *buffer) { + const char *buffer, + bool *invalid_config, + void *context) { + Context *c = ASSERT_PTR(context); _cleanup_free_ char *action = NULL, *name = NULL, *resolved_name = NULL, *id = NULL, *resolved_id = NULL, @@ -1697,10 +1699,10 @@ static int parse_line( int r; const char *p; - assert(c); assert(fname); assert(line >= 1); assert(buffer); + assert(!invalid_config); /* We don't support invalid_config yet. */ /* Parse columns */ p = buffer; @@ -1969,57 +1971,14 @@ static int parse_line( } static int read_config_file(Context *c, const char *fn, bool ignore_enoent) { - _cleanup_fclose_ FILE *rf = NULL; - _cleanup_free_ char *pp = NULL; - FILE *f = NULL; - unsigned v = 0; - int r = 0; - - assert(c); - assert(fn); - - if (streq(fn, "-")) - f = stdin; - else { - r = search_and_fopen(fn, "re", arg_root, (const char**) CONF_PATHS_STRV("sysusers.d"), &rf, &pp); - if (r < 0) { - if (ignore_enoent && r == -ENOENT) - return 0; - - return log_error_errno(r, "Failed to open '%s', ignoring: %m", fn); - } - - f = rf; - fn = pp; - } - - for (;;) { - _cleanup_free_ char *line = NULL; - int k; - - k = read_stripped_line(f, LONG_LINE_MAX, &line); - if (k < 0) - return log_error_errno(k, "Failed to read '%s': %m", fn); - if (k == 0) - break; - - v++; - - if (IN_SET(line[0], 0, '#')) - continue; - - k = parse_line(c, fn, v, line); - if (k < 0 && r == 0) - r = k; - } - - if (ferror(f)) { - log_error_errno(errno, "Failed to read from file %s: %m", fn); - if (r == 0) - r = -EIO; - } - - return r; + return conf_file_read( + arg_root, + (const char**) CONF_PATHS_STRV("sysusers.d"), + ASSERT_PTR(fn), + parse_line, + ASSERT_PTR(c), + ignore_enoent, + /* invalid_config= */ NULL); } static int cat_config(void) { @@ -2193,7 +2152,7 @@ static int parse_arguments(Context *c, char **args) { STRV_FOREACH(arg, args) { if (arg_inline) /* Use (argument):n, where n==1 for the first positional arg */ - r = parse_line(c, "(argument)", pos, *arg); + r = parse_line("(argument)", pos, *arg, /* invalid_config= */ NULL, c); else r = read_config_file(c, *arg, /* ignore_enoent= */ false); if (r < 0) From ec3917d2825a32ac14eec209e53c6c6d6cd2b0da Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Sun, 3 Dec 2023 17:06:06 +0100 Subject: [PATCH 06/18] tmpfiles,sysusers: rework path argument handling MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Previously, if given an absolute path, we would open the file, but when given a relative path, we'd attempt to search the directories. If the user wants to open a file from the search path, allowing paths is very confusing. E.g. with a path like 'sysusers/foo.conf', we'd try to open '/etc/sysusers.d/sysusers/foo.conf', '/run/sysusers.d/sysusers/foo.conf', …, and with '../foo.conf', we'd try to open '/etc/sysusers.d/../foo.conf', '/run/sysusers.d/../foo.conf', …. This just isn't useful, and in fact for a scheme like sysusers.d and tmpfiles.d where there we have a flat directory with config files, only searching for plain names can result in success. When a user specifies a relative path, it's more likely that they wanted to open some local file. OTOH, to correctly open a local file, e.g. one that they're just writing, this interface is also awkward, because something like '$PWD/file.conf' has to be used to open a file with a relative path. This patch changes the interface so that any path (i.e. an argument with "/") is used to open a file directly, and only plain basenames are used for searching. (Note that tpmfiles and sysusers are somewhat special here: their "config files" make sense without the other config and users are likely to want to test them without the other config. I was trying to do just that when writing a spec file for a package and attempting to convert the existing scripts to sysusers and tmpfiles. The same logic wouldn't apply for example to units or udev rules, because they generally can only be interpreted with the whole rest of config also available.) --- man/systemd-sysusers.xml | 12 ++++++------ man/systemd-tmpfiles.xml | 19 +++++++++++-------- src/basic/conf-files.c | 40 ++++++++++++++++++++++++++++------------ 3 files changed, 45 insertions(+), 26 deletions(-) diff --git a/man/systemd-sysusers.xml b/man/systemd-sysusers.xml index 548ce24e4e..895422779d 100644 --- a/man/systemd-sysusers.xml +++ b/man/systemd-sysusers.xml @@ -40,17 +40,17 @@ sysusers.d5. - If invoked with no arguments, it applies all directives from all files found in the directories + If invoked with no arguments, directives from the configuration files found in the directories specified by sysusers.d5. When invoked with positional arguments, if option is specified, arguments specified on the command line are used instead of the configuration file PATH. Otherwise, just the configuration specified by the command line - arguments is executed. The string - may be specified instead of a filename to instruct - systemd-sysusers to read the configuration from standard input. If the argument is a - relative path, all configuration directories are searched for a matching file and the file found that has - the highest priority is executed. If the argument is an absolute path, that file is used directly without - searching of the configuration directories. + arguments is executed. If the string - is specified instead of a filename, the + configuration is read from standard input. If the argument is a file name (without any slashes), all + configuration directories are searched for a matching file and the file found that has the highest + priority is executed. If the argument is a path, that file is used directly without searching the + configuration directories for any other matching file. diff --git a/man/systemd-tmpfiles.xml b/man/systemd-tmpfiles.xml index c30a046f72..3a3924a084 100644 --- a/man/systemd-tmpfiles.xml +++ b/man/systemd-tmpfiles.xml @@ -62,14 +62,17 @@ be invoked with one or more options , , and , to select the respective subset of operations. - By default, directives from all configuration files are applied. When invoked with - , arguments specified on the command line are - used instead of the configuration file PATH. Otherwise, if one or more - absolute filenames are passed on the command line, only the directives in these files are applied. If - - is specified instead of a filename, directives are read from standard input. If only - the basename of a configuration file is specified, all configuration directories as specified in - tmpfiles.d5 are - searched for a matching file and the file found that has the highest priority is executed. + If invoked with no arguments, directives from the configuration files found in the directories + specified by + tmpfiles.d5. When + invoked with positional arguments, if option + is specified, arguments specified on the command line are used instead of the configuration file + PATH. Otherwise, just the configuration specified by the command line + arguments is executed. If the string - is specified instead of a filename, the + configuration is read from standard input. If the argument is a file name (without any slashes), all + configuration directories are searched for a matching file and the file found that has the highest + priority is executed. If the argument is a path, that file is used directly without searching the + configuration directories for any other matching file. System services (systemd-tmpfiles-setup.service, systemd-tmpfiles-setup-dev-early.service, diff --git a/src/basic/conf-files.c b/src/basic/conf-files.c index c98e7d0d93..9cb66c099b 100644 --- a/src/basic/conf-files.c +++ b/src/basic/conf-files.c @@ -381,8 +381,10 @@ int conf_files_list_dropins( * * The argument may be: * - '-', meaning stdin. - * - a non-absolute path. In this case are searched. - * - an absolute path. In this case is opened directly. + * - a file name without a path. In this case are searched. + * - a path, either relative or absolute. In this case is opened directly. + * + * This method is only suitable for configuration files which have a flat layout without dropins. */ int conf_file_read( const char *root, @@ -397,7 +399,7 @@ int conf_file_read( _cleanup_free_ char *_fn = NULL; unsigned v = 0; FILE *f; - int r; + int r = 0; assert(fn); @@ -407,20 +409,34 @@ int conf_file_read( log_debug("Reading config from stdin%s", special_glyph(SPECIAL_GLYPH_ELLIPSIS)); + } else if (is_path(fn)) { + r = path_make_absolute_cwd(fn, &_fn); + if (r < 0) + return log_error_errno(r, "Failed to make path absolute: %m"); + fn = _fn; + + f = _f = fopen(fn, "re"); + if (!_f) + r = -errno; + else + log_debug("Reading config file \"%s\"%s", fn, special_glyph(SPECIAL_GLYPH_ELLIPSIS)); + } else { r = search_and_fopen(fn, "re", root, config_dirs, &_f, &_fn); - if (r == -ENOENT && ignore_enoent) { - log_debug_errno(r, "Failed to open \"%s\", ignoring: %m", fn); - return 0; /* No error, but nothing happened. */ + if (r >= 0) { + f = _f; + fn = _fn; + log_debug("Reading config file \"%s\"%s", fn, special_glyph(SPECIAL_GLYPH_ELLIPSIS)); } - if (r < 0) - return log_error_errno(r, "Failed to read '%s': %m", fn); - - f = _f; - fn = _fn; - log_debug("Reading config file \"%s\"%s", fn, special_glyph(SPECIAL_GLYPH_ELLIPSIS)); } + if (r == -ENOENT && ignore_enoent) { + log_debug_errno(r, "Failed to open \"%s\", ignoring: %m", fn); + return 0; /* No error, but nothing happened. */ + } + if (r < 0) + return log_error_errno(r, "Failed to read '%s': %m", fn); + r = 1; /* We entered the part where we may modify state. */ for (;;) { From e5876c29ad81e54f7cf98328d08fb25b9092f814 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Sun, 3 Dec 2023 19:53:51 +0100 Subject: [PATCH 07/18] tmpfiles: inline variable declarations, use FOREACH_ARRAY and RET_GATHER --- src/tmpfiles/tmpfiles.c | 57 +++++++++++------------------------------ 1 file changed, 15 insertions(+), 42 deletions(-) diff --git a/src/tmpfiles/tmpfiles.c b/src/tmpfiles/tmpfiles.c index a2eb05c6a6..ab9ea2768a 100644 --- a/src/tmpfiles/tmpfiles.c +++ b/src/tmpfiles/tmpfiles.c @@ -449,17 +449,10 @@ static bool takes_ownership(ItemType t) { static struct Item* find_glob(OrderedHashmap *h, const char *match) { ItemArray *j; - ORDERED_HASHMAP_FOREACH(j, h) { - size_t n; - - for (n = 0; n < j->n_items; n++) { - Item *item = j->items + n; - + ORDERED_HASHMAP_FOREACH(j, h) + FOREACH_ARRAY(item, j->items, j->n_items) if (fnmatch(item->path, match, FNM_PATHNAME|FNM_PERIOD) == 0) return item; - } - } - return NULL; } @@ -3121,7 +3114,6 @@ static int process_item_array( OperationMask operation) { int r = 0; - size_t n; assert(c); assert(array); @@ -3134,22 +3126,12 @@ static int process_item_array( if ((operation & (OPERATION_REMOVE|OPERATION_CLEAN|OPERATION_PURGE)) && !set_isempty(array->children)) { ItemArray *cc; - SET_FOREACH(cc, array->children) { - int k; - - k = process_item_array(c, cc, operation & (OPERATION_REMOVE|OPERATION_CLEAN|OPERATION_PURGE)); - if (k < 0 && r == 0) - r = k; - } + SET_FOREACH(cc, array->children) + RET_GATHER(r, process_item_array(c, cc, operation & (OPERATION_REMOVE|OPERATION_CLEAN|OPERATION_PURGE))); } - for (n = 0; n < array->n_items; n++) { - int k; - - k = process_item(c, array->items + n, operation); - if (k < 0 && r == 0) - r = k; - } + FOREACH_ARRAY(item, array->items, array->n_items) + RET_GATHER(r, process_item(c, item, operation)); return r; } @@ -3174,13 +3156,11 @@ static void item_free_contents(Item *i) { } static ItemArray* item_array_free(ItemArray *a) { - size_t n; - if (!a) return NULL; - for (n = 0; n < a->n_items; n++) - item_free_contents(a->items + n); + FOREACH_ARRAY(item, a->items, a->n_items) + item_free_contents(item); set_free(a->children); free(a->items); @@ -3443,13 +3423,10 @@ static int parse_age_by_from_arg(const char *age_by_str, Item *item) { } static bool is_duplicated_item(ItemArray *existing, const Item *i) { - assert(existing); assert(i); - for (size_t n = 0; n < existing->n_items; n++) { - const Item *e = existing->items + n; - + FOREACH_ARRAY(e, existing->items, existing->n_items) { if (item_compatible(e, i)) continue; @@ -3478,9 +3455,9 @@ static int parse_line( }; ItemArray *existing; OrderedHashmap *h; - int r, pos; bool append_or_force = false, boot = false, allow_failure = false, try_replace = false, unbase64 = false, from_cred = false, missing_user_or_group = false; + int r; assert(fname); assert(line >= 1); @@ -3542,7 +3519,7 @@ static int parse_line( return log_syntax(NULL, LOG_ERR, fname, line, SYNTHETIC_ERRNO(EBADMSG), "Command too short '%s'.", action); } - for (pos = 1; action[pos]; pos++) { + for (int pos = 1; action[pos]; pos++) if (action[pos] == '!' && !boot) boot = true; else if (action[pos] == '+' && !append_or_force) @@ -3559,7 +3536,6 @@ static int parse_line( *invalid_config = true; return log_syntax(NULL, LOG_ERR, fname, line, SYNTHETIC_ERRNO(EBADMSG), "Unknown modifiers in command '%s'", action); } - } if (boot && !arg_boot) { log_syntax(NULL, LOG_DEBUG, fname, line, 0, "Ignoring entry %s \"%s\" because --boot is not specified.", action, path); @@ -3864,14 +3840,13 @@ static int parse_line( const char *mm; unsigned m; - for (mm = mode;; mm++) { + for (mm = mode;; mm++) if (*mm == '~') i.mask_perms = true; else if (*mm == ':') i.mode_only_create = true; else break; - } r = parse_mode(mm, &m); if (r < 0) { @@ -4238,17 +4213,15 @@ static int read_config_file( /* we have to determine age parameter for each entry of type X */ ORDERED_HASHMAP_FOREACH(ia, c->globs) - for (size_t ni = 0; ni < ia->n_items; ni++) { + FOREACH_ARRAY(i, ia->items, ia->n_items) { ItemArray *ja; - Item *i = ia->items + ni, *candidate_item = NULL; + Item *candidate_item = NULL; if (i->type != IGNORE_DIRECTORY_PATH) continue; ORDERED_HASHMAP_FOREACH(ja, c->items) - for (size_t nj = 0; nj < ja->n_items; nj++) { - Item *j = ja->items + nj; - + FOREACH_ARRAY(j, ja->items, ja->n_items) { if (!IN_SET(j->type, CREATE_DIRECTORY, TRUNCATE_DIRECTORY, CREATE_SUBVOLUME, From ad36bb9fbb44481b41b93607bc38d3f52cd5f9c8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Sun, 3 Dec 2023 20:01:27 +0100 Subject: [PATCH 08/18] tmpfiles: break long log invocations and comments --- src/tmpfiles/tmpfiles.c | 135 +++++++++++++++++++++++----------------- 1 file changed, 78 insertions(+), 57 deletions(-) diff --git a/src/tmpfiles/tmpfiles.c b/src/tmpfiles/tmpfiles.c index ab9ea2768a..ffd654e372 100644 --- a/src/tmpfiles/tmpfiles.c +++ b/src/tmpfiles/tmpfiles.c @@ -683,9 +683,9 @@ static int dir_cleanup( continue; } - /* Try to detect bind mounts of the same filesystem instance; they do not differ in device - * major/minors. This type of query is not supported on all kernels or filesystem types - * though. */ + /* Try to detect bind mounts of the same filesystem instance; they do not differ in + * device major/minors. This type of query is not supported on all kernels or + * filesystem types though. */ if (S_ISDIR(sx.stx_mode)) { int q; @@ -871,8 +871,8 @@ static bool dangerous_hardlinks(void) { static int cached = -1; int r; - /* Check whether the fs.protected_hardlinks sysctl is on. If we can't determine it we assume its off, as that's - * what the upstream default is. */ + /* Check whether the fs.protected_hardlinks sysctl is on. If we can't determine it we assume its off, + * as that's what the upstream default is. */ if (cached >= 0) return cached; @@ -1517,8 +1517,7 @@ static int parse_attribute_from_arg(Item *item) { if (i >= ELEMENTSOF(attributes)) return log_error_errno(SYNTHETIC_ERRNO(EINVAL), - "Unknown file attribute '%c' on '%s'.", - *p, item->path); + "Unknown file attribute '%c' on '%s'.", *p, item->path); v = attributes[i].value; @@ -2104,7 +2103,8 @@ static int create_device( if (r < 0) return log_error_errno(r, "Failed to extract filename from path '%s': %m", i->path); if (r == O_DIRECTORY) - return log_error_errno(SYNTHETIC_ERRNO(EISDIR), "Cannot open path '%s' for creating device node, is a directory.", i->path); + return log_error_errno(SYNTHETIC_ERRNO(EISDIR), + "Cannot open path '%s' for creating device node, is a directory.", i->path); /* Validate the path and use the returned directory fd for copying the target so we're sure that the * path can't be changed behind our back. */ @@ -2171,7 +2171,8 @@ static int create_device( return log_error_errno(errno, "Failed to fstat(%s): %m", i->path); if (((st.st_mode ^ file_type) & S_IFMT) != 0) - return log_error_errno(SYNTHETIC_ERRNO(EBADF), "Device node we just created is not a device node, refusing."); + return log_error_errno(SYNTHETIC_ERRNO(EBADF), + "Device node we just created is not a device node, refusing."); creation = CREATION_FORCE; } else { @@ -2209,7 +2210,8 @@ static int create_fifo(Context *c, Item *i) { if (r < 0) return log_error_errno(r, "Failed to extract filename from path '%s': %m", i->path); if (r == O_DIRECTORY) - return log_error_errno(SYNTHETIC_ERRNO(EISDIR), "Cannot open path '%s' for creating FIFO, is a directory.", i->path); + return log_error_errno(SYNTHETIC_ERRNO(EISDIR), + "Cannot open path '%s' for creating FIFO, is a directory.", i->path); pfd = path_open_parent_safe(i->path, i->allow_failure); if (pfd < 0) @@ -2266,7 +2268,8 @@ static int create_fifo(Context *c, Item *i) { return log_error_errno(errno, "Failed to fstat(%s): %m", i->path); if (!S_ISFIFO(st.st_mode)) - return log_error_errno(SYNTHETIC_ERRNO(EBADF), "FIFO inode we just created is not a FIFO, refusing."); + return log_error_errno(SYNTHETIC_ERRNO(EBADF), + "FIFO inode we just created is not a FIFO, refusing."); creation = CREATION_FORCE; } else { @@ -2295,7 +2298,8 @@ static int create_symlink(Context *c, Item *i) { if (r < 0) return log_error_errno(r, "Failed to extract filename from path '%s': %m", i->path); if (r == O_DIRECTORY) - return log_error_errno(SYNTHETIC_ERRNO(EISDIR), "Cannot open path '%s' for creating FIFO, is a directory.", i->path); + return log_error_errno(SYNTHETIC_ERRNO(EISDIR), + "Cannot open path '%s' for creating FIFO, is a directory.", i->path); pfd = path_open_parent_safe(i->path, i->allow_failure); if (pfd < 0) @@ -3290,27 +3294,30 @@ static int patch_var_run(const char *fname, unsigned line, char **path) { assert(path); assert(*path); - /* Optionally rewrites lines referencing /var/run/, to use /run/ instead. Why bother? tmpfiles merges lines in - * some cases and detects conflicts in others. If files/directories are specified through two equivalent lines - * this is problematic as neither case will be detected. Ideally we'd detect these cases by resolving symlinks - * early, but that's precisely not what we can do here as this code very likely is running very early on, at a - * time where the paths in question are not available yet, or even more importantly, our own tmpfiles rules - * might create the paths that are intermediary to the listed paths. We can't really cover the generic case, - * but the least we can do is cover the specific case of /var/run vs. /run, as /var/run is a legacy name for - * /run only, and we explicitly document that and require that on systemd systems the former is a symlink to - * the latter. Moreover files below this path are by far the primary use case for tmpfiles.d/. */ + /* Optionally rewrites lines referencing /var/run/, to use /run/ instead. Why bother? tmpfiles merges + * lines in some cases and detects conflicts in others. If files/directories are specified through + * two equivalent lines this is problematic as neither case will be detected. Ideally we'd detect + * these cases by resolving symlinks early, but that's precisely not what we can do here as this code + * very likely is running very early on, at a time where the paths in question are not available yet, + * or even more importantly, our own tmpfiles rules might create the paths that are intermediary to + * the listed paths. We can't really cover the generic case, but the least we can do is cover the + * specific case of /var/run vs. /run, as /var/run is a legacy name for /run only, and we explicitly + * document that and require that on systemd systems the former is a symlink to the latter. Moreover + * files below this path are by far the primary use case for tmpfiles.d/. */ k = path_startswith(*path, "/var/run/"); - if (isempty(k)) /* Don't complain about other paths than /var/run, and not about /var/run itself either. */ + if (isempty(k)) /* Don't complain about paths other than under /var/run, + * and not about /var/run itself either. */ return 0; n = path_join("/run", k); if (!n) return log_oom(); - /* Also log about this briefly. We do so at LOG_NOTICE level, as we fixed up the situation automatically, hence - * there's no immediate need for action by the user. However, in the interest of making things less confusing - * to the user, let's still inform the user that these snippets should really be updated. */ + /* Also log about this briefly. We do so at LOG_NOTICE level, as we fixed up the situation + * automatically, hence there's no immediate need for action by the user. However, in the interest of + * making things less confusing to the user, let's still inform the user that these snippets should + * really be updated. */ log_syntax(NULL, LOG_NOTICE, fname, line, 0, "Line references path below legacy directory /var/run/, updating %s → %s; please update the tmpfiles.d/ drop-in file accordingly.", *path, n); @@ -3516,7 +3523,8 @@ static int parse_line( if (isempty(action)) { *invalid_config = true; - return log_syntax(NULL, LOG_ERR, fname, line, SYNTHETIC_ERRNO(EBADMSG), "Command too short '%s'.", action); + return log_syntax(NULL, LOG_ERR, fname, line, SYNTHETIC_ERRNO(EBADMSG), + "Command too short '%s'.", action); } for (int pos = 1; action[pos]; pos++) @@ -3534,11 +3542,13 @@ static int parse_line( from_cred = true; else { *invalid_config = true; - return log_syntax(NULL, LOG_ERR, fname, line, SYNTHETIC_ERRNO(EBADMSG), "Unknown modifiers in command '%s'", action); + return log_syntax(NULL, LOG_ERR, fname, line, SYNTHETIC_ERRNO(EBADMSG), + "Unknown modifiers in command '%s'.", action); } if (boot && !arg_boot) { - log_syntax(NULL, LOG_DEBUG, fname, line, 0, "Ignoring entry %s \"%s\" because --boot is not specified.", action, path); + log_syntax(NULL, LOG_DEBUG, fname, line, 0, + "Ignoring entry %s \"%s\" because --boot is not specified.", action, path); return 0; } @@ -3553,7 +3563,8 @@ static int parse_line( if (r < 0) { if (IN_SET(r, -EINVAL, -EBADSLT)) *invalid_config = true; - return log_syntax(NULL, LOG_ERR, fname, line, r, "Failed to replace specifiers in '%s': %m", path); + return log_syntax(NULL, LOG_ERR, fname, line, r, + "Failed to replace specifiers in '%s': %m", path); } r = patch_var_run(fname, line, &i.path); @@ -3585,14 +3596,8 @@ static int parse_line( case RELABEL_PATH: case RECURSIVE_RELABEL_PATH: if (i.argument) - log_syntax(NULL, - LOG_WARNING, - fname, - line, - 0, - "%c lines don't take argument fields, ignoring.", - (char) i.type); - + log_syntax(NULL, LOG_WARNING, fname, line, 0, + "%c lines don't take argument fields, ignoring.", (char) i.type); break; case CREATE_FILE: @@ -3602,21 +3607,24 @@ static int parse_line( case CREATE_SYMLINK: if (unbase64) { *invalid_config = true; - return log_syntax(NULL, LOG_ERR, fname, line, SYNTHETIC_ERRNO(EBADMSG), "base64 decoding not supported for symlink targets."); + return log_syntax(NULL, LOG_ERR, fname, line, SYNTHETIC_ERRNO(EBADMSG), + "base64 decoding not supported for symlink targets."); } break; case WRITE_FILE: if (!i.argument) { *invalid_config = true; - return log_syntax(NULL, LOG_ERR, fname, line, SYNTHETIC_ERRNO(EBADMSG), "Write file requires argument."); + return log_syntax(NULL, LOG_ERR, fname, line, SYNTHETIC_ERRNO(EBADMSG), + "Write file requires argument."); } break; case COPY_FILES: if (unbase64) { *invalid_config = true; - return log_syntax(NULL, LOG_ERR, fname, line, SYNTHETIC_ERRNO(EBADMSG), "base64 decoding not supported for copy sources."); + return log_syntax(NULL, LOG_ERR, fname, line, SYNTHETIC_ERRNO(EBADMSG), + "base64 decoding not supported for copy sources."); } break; @@ -3624,18 +3632,21 @@ static int parse_line( case CREATE_BLOCK_DEVICE: if (unbase64) { *invalid_config = true; - return log_syntax(NULL, LOG_ERR, fname, line, SYNTHETIC_ERRNO(EBADMSG), "base64 decoding not supported for device node creation."); + return log_syntax(NULL, LOG_ERR, fname, line, SYNTHETIC_ERRNO(EBADMSG), + "base64 decoding not supported for device node creation."); } if (!i.argument) { *invalid_config = true; - return log_syntax(NULL, LOG_ERR, fname, line, SYNTHETIC_ERRNO(EBADMSG), "Device file requires argument."); + return log_syntax(NULL, LOG_ERR, fname, line, SYNTHETIC_ERRNO(EBADMSG), + "Device file requires argument."); } r = parse_devnum(i.argument, &i.major_minor); if (r < 0) { *invalid_config = true; - return log_syntax(NULL, LOG_ERR, fname, line, r, "Can't parse device file major/minor '%s'.", i.argument); + return log_syntax(NULL, LOG_ERR, fname, line, r, + "Can't parse device file major/minor '%s'.", i.argument); } break; @@ -3644,7 +3655,8 @@ static int parse_line( case RECURSIVE_SET_XATTR: if (unbase64) { *invalid_config = true; - return log_syntax(NULL, LOG_ERR, fname, line, SYNTHETIC_ERRNO(EBADMSG), "base64 decoding not supported for extended attributes."); + return log_syntax(NULL, LOG_ERR, fname, line, SYNTHETIC_ERRNO(EBADMSG), + "base64 decoding not supported for extended attributes."); } if (!i.argument) { *invalid_config = true; @@ -3660,7 +3672,8 @@ static int parse_line( case RECURSIVE_SET_ACL: if (unbase64) { *invalid_config = true; - return log_syntax(NULL, LOG_ERR, fname, line, SYNTHETIC_ERRNO(EBADMSG), "base64 decoding not supported for ACLs."); + return log_syntax(NULL, LOG_ERR, fname, line, SYNTHETIC_ERRNO(EBADMSG), + "base64 decoding not supported for ACLs."); } if (!i.argument) { *invalid_config = true; @@ -3676,7 +3689,8 @@ static int parse_line( case RECURSIVE_SET_ATTRIBUTE: if (unbase64) { *invalid_config = true; - return log_syntax(NULL, LOG_ERR, fname, line, SYNTHETIC_ERRNO(EBADMSG), "base64 decoding not supported for file attributes."); + return log_syntax(NULL, LOG_ERR, fname, line, SYNTHETIC_ERRNO(EBADMSG), + "base64 decoding not supported for file attributes."); } if (!i.argument) { *invalid_config = true; @@ -3707,7 +3721,8 @@ static int parse_line( if (r < 0) { if (IN_SET(r, -EINVAL, -EBADSLT)) *invalid_config = true; - return log_syntax(NULL, LOG_ERR, fname, line, r, "Failed to substitute specifiers in argument: %m"); + return log_syntax(NULL, LOG_ERR, fname, line, r, + "Failed to substitute specifiers in argument: %m"); } } @@ -3727,7 +3742,8 @@ static int parse_line( return log_oom(); } else if (!path_is_absolute(i.argument)) { *invalid_config = true; - return log_syntax(NULL, LOG_ERR, fname, line, SYNTHETIC_ERRNO(EBADMSG), "Source path '%s' is not absolute.", i.argument); + return log_syntax(NULL, LOG_ERR, fname, line, SYNTHETIC_ERRNO(EBADMSG), + "Source path '%s' is not absolute.", i.argument); } @@ -3744,7 +3760,8 @@ static int parse_line( if (laccess(i.argument, F_OK) == -ENOENT) { /* Silently skip over lines where the source file is missing. */ - log_syntax(NULL, LOG_DEBUG, fname, line, 0, "Copy source path '%s' does not exist, skipping line.", i.argument); + log_syntax(NULL, LOG_DEBUG, fname, line, 0, + "Copy source path '%s' does not exist, skipping line.", i.argument); return 0; } @@ -3756,9 +3773,11 @@ static int parse_line( if (from_cred) { if (!i.argument) - return log_syntax(NULL, LOG_ERR, fname, line, SYNTHETIC_ERRNO(EINVAL), "Reading from credential requested, but no credential name specified."); + return log_syntax(NULL, LOG_ERR, fname, line, SYNTHETIC_ERRNO(EINVAL), + "Reading from credential requested, but no credential name specified."); if (!credential_name_valid(i.argument)) - return log_syntax(NULL, LOG_ERR, fname, line, SYNTHETIC_ERRNO(EINVAL), "Credential name not valid: %s", i.argument); + return log_syntax(NULL, LOG_ERR, fname, line, SYNTHETIC_ERRNO(EINVAL), + "Credential name not valid: %s", i.argument); r = read_credential(i.argument, &i.binary_argument, &i.binary_argument_size); if (IN_SET(r, -ENXIO, -ENOENT)) { @@ -4188,7 +4207,8 @@ static int parse_argv(int argc, char *argv[]) { "Combination of --user and --root= is not supported."); if (arg_image && arg_root) - return log_error_errno(SYNTHETIC_ERRNO(EINVAL), "Please specify either --root= or --image=, the combination of both is not supported."); + return log_error_errno(SYNTHETIC_ERRNO(EINVAL), + "Please specify either --root= or --image=, the combination of both is not supported."); return 1; } @@ -4327,9 +4347,10 @@ static int link_parent(Context *c, ItemArray *a) { assert(c); assert(a); - /* Finds the closest "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. */ + /* Finds the closest "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; @@ -4499,8 +4520,8 @@ static int run(int argc, char *argv[]) { return r; } - /* 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. */ + /* 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 < _PHASE_MAX; phase++) { OperationMask op; From f7862b2a009fd2ec6ccc35d3899f55bebb988c25 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Mon, 4 Dec 2023 11:57:55 +0100 Subject: [PATCH 09/18] tree-wide: use normal spelling of "reopen" It's a commonly used verb meaning "to open again". --- man/systemd-run.xml | 4 ++-- man/systemd.exec.xml | 2 +- src/boot/efi/util.c | 2 +- src/libsystemd/sd-journal/test-journal-interleaving.c | 2 +- src/login/logind-inhibit.c | 2 +- src/shared/ptyfwd.c | 2 +- src/tmpfiles/tmpfiles.c | 6 +++--- 7 files changed, 10 insertions(+), 10 deletions(-) diff --git a/man/systemd-run.xml b/man/systemd-run.xml index 3e93d3a678..e891920c63 100644 --- a/man/systemd-run.xml +++ b/man/systemd-run.xml @@ -352,10 +352,10 @@ When this option is used the original file descriptors systemd-run receives are passed to the service processes as-is. If the service runs with different privileges than - systemd-run, this means the service might not be able to re-open the passed file + systemd-run, this means the service might not be able to reopen the passed file descriptors, due to normal file descriptor access restrictions. If the invoked process is a shell script that uses the echo "hello" >/dev/stderr construct for writing messages to stderr, this might - cause problems, as this only works if stderr can be re-opened. To mitigate this use the construct echo + cause problems, as this only works if stderr can be reopened. To mitigate this use the construct echo "hello" >&2 instead, which is mostly equivalent and avoids this pitfall. diff --git a/man/systemd.exec.xml b/man/systemd.exec.xml index ca20e6e308..300255e038 100644 --- a/man/systemd.exec.xml +++ b/man/systemd.exec.xml @@ -3017,7 +3017,7 @@ SystemCallErrorNumber=EPERM the kernel log buffer, the unit will implicitly gain a dependency of type After= on systemd-journald.socket (also see the "Implicit Dependencies" section above). Also note that in this case stdout (or stderr, see below) will be an - AF_UNIX stream socket, and not a pipe or FIFO that can be re-opened. This means + AF_UNIX stream socket, and not a pipe or FIFO that can be reopened. This means when executing shell scripts the construct echo "hello" > /dev/stderr for writing text to stderr will not work. To mitigate this use the construct echo "hello" >&2 instead, which is mostly equivalent and avoids this pitfall. diff --git a/src/boot/efi/util.c b/src/boot/efi/util.c index 25f5e0f032..9cedc15d6a 100644 --- a/src/boot/efi/util.c +++ b/src/boot/efi/util.c @@ -303,7 +303,7 @@ EFI_STATUS chunked_read(EFI_FILE *file, size_t *size, void *buf) { * Some broken firmwares cannot handle large file reads and will instead return * an error. As a workaround, read such files in small chunks. * Note that we cannot just try reading the whole file first on such firmware as - * that will permanently break the handle even if it is re-opened. + * that will permanently break the handle even if it is reopened. * * https://github.com/systemd/systemd/issues/25911 */ diff --git a/src/libsystemd/sd-journal/test-journal-interleaving.c b/src/libsystemd/sd-journal/test-journal-interleaving.c index 8aeef8f607..eac1af2f7b 100644 --- a/src/libsystemd/sd-journal/test-journal-interleaving.c +++ b/src/libsystemd/sd-journal/test-journal-interleaving.c @@ -502,7 +502,7 @@ static void test_sequence_numbers_one(void) { test_close(one); /* If the machine-id is not initialized, the header file verification - * (which happens when re-opening a journal file) will fail. */ + * (which happens when reopening a journal file) will fail. */ if (sd_id128_get_machine(NULL) >= 0) { /* restart server */ seqnum = 0; diff --git a/src/login/logind-inhibit.c b/src/login/logind-inhibit.c index 1566dab0b6..7591bce08c 100644 --- a/src/login/logind-inhibit.c +++ b/src/login/logind-inhibit.c @@ -270,7 +270,7 @@ int inhibitor_load(Inhibitor *i) { if (i->fifo_path) { _cleanup_close_ int fd = -EBADF; - /* Let's re-open the FIFO on both sides, and close the writing side right away */ + /* Let's reopen the FIFO on both sides, and close the writing side right away */ fd = inhibitor_create_fifo(i); if (fd < 0) return log_error_errno(fd, "Failed to reopen FIFO: %m"); diff --git a/src/shared/ptyfwd.c b/src/shared/ptyfwd.c index c05ce79489..9eb8834be6 100644 --- a/src/shared/ptyfwd.c +++ b/src/shared/ptyfwd.c @@ -695,7 +695,7 @@ int pty_forward_new( else { /* If we shall be invoked in interactive mode, let's switch on non-blocking mode, so that we * never end up staving one direction while we block on the other. However, let's be careful - * here and not turn on O_NONBLOCK for stdin/stdout directly, but of re-opened copies of + * here and not turn on O_NONBLOCK for stdin/stdout directly, but of reopened copies of * them. This has two advantages: when we are killed abruptly the stdin/stdout fds won't be * left in O_NONBLOCK state for the next process using them. In addition, if some process * running in the background wants to continue writing to our stdout it can do so without diff --git a/src/tmpfiles/tmpfiles.c b/src/tmpfiles/tmpfiles.c index ffd654e372..ff746056af 100644 --- a/src/tmpfiles/tmpfiles.c +++ b/src/tmpfiles/tmpfiles.c @@ -1580,7 +1580,7 @@ static int fd_set_attribute( procfs_fd = fd_reopen(fd, O_RDONLY|O_CLOEXEC|O_NOATIME); if (procfs_fd < 0) - return log_error_errno(procfs_fd, "Failed to re-open '%s': %m", path); + return log_error_errno(procfs_fd, "Failed to reopen '%s': %m", path); unsigned previous, current; r = chattr_full(procfs_fd, NULL, f, item->attribute_mask, &previous, ¤t, CHATTR_FALLBACK_BITWISE); @@ -1732,7 +1732,7 @@ static int create_file( * fd_set_perms() report the error if the perms need to be modified. */ fd = openat(dir_fd, bn, O_NOFOLLOW|O_CLOEXEC|O_PATH, i->mode); if (fd < 0) - return log_error_errno(errno, "Failed to re-open file %s: %m", path); + return log_error_errno(errno, "Failed to reopen file %s: %m", path); if (fstat(fd, &stbuf) < 0) return log_error_errno(errno, "stat(%s) failed: %m", path); @@ -1815,7 +1815,7 @@ static int truncate_file( "Cannot create file %s on a read-only file system.", path); - return log_error_errno(errno, "Failed to re-open file %s: %m", path); + return log_error_errno(errno, "Failed to reopen file %s: %m", path); } erofs = true; From 9c8a0ca69e9270c5446400135e29c99d85e68048 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Mon, 4 Dec 2023 12:15:27 +0100 Subject: [PATCH 10/18] tmpfiles: split out verbs in help One of the three must always be specified, but they buried in a long list of options in the output of --help. Make them more visible to draw the eye. Also, drop "marked" from the description. It's supposed to mean "configured", but it's a strange way to say that, and also it's generally obvious that the program does what its configuration tells it to, and it's not going to remove all files found on the system. --- man/systemd-tmpfiles.xml | 15 +++++++-------- src/tmpfiles/tmpfiles.c | 17 ++++++++++------- tools/command_ignorelist | 6 +++--- 3 files changed, 20 insertions(+), 18 deletions(-) diff --git a/man/systemd-tmpfiles.xml b/man/systemd-tmpfiles.xml index 3a3924a084..414d154c42 100644 --- a/man/systemd-tmpfiles.xml +++ b/man/systemd-tmpfiles.xml @@ -23,8 +23,7 @@ systemd-tmpfiles-setup-dev.service systemd-tmpfiles-clean.service systemd-tmpfiles-clean.timer - Creates, deletes and cleans up volatile - and temporary files and directories + Create, delete, and clean up files and directories @@ -59,7 +58,7 @@ systemd-tmpfiles creates, deletes, and cleans up volatile and temporary files and directories, using the configuration file format and location specified in tmpfiles.d5. It must - be invoked with one or more options , , and + be invoked with one or more commands , , and , to select the respective subset of operations. If invoked with no arguments, directives from the configuration files found in the directories @@ -99,14 +98,14 @@ - Options + Commands and options - The following options are understood: + The following commands are understood: - If this option is passed, all files and + If this command is passed, all files and directories marked with f, F, @@ -132,14 +131,14 @@ - If this option is passed, all files and + If this command is passed, all files and directories with an age parameter configured will be cleaned up. - If this option is passed, the contents of + If this command is passed, the contents of directories marked with D or R, and files or directories themselves marked with r or R are diff --git a/src/tmpfiles/tmpfiles.c b/src/tmpfiles/tmpfiles.c index ff746056af..5dcdb7e406 100644 --- a/src/tmpfiles/tmpfiles.c +++ b/src/tmpfiles/tmpfiles.c @@ -4003,16 +4003,18 @@ static int help(void) { if (r < 0) return log_oom(); - printf("%s [OPTIONS...] [CONFIGURATION FILE...]\n" - "\n%sCreates, deletes and cleans up volatile and temporary files and directories.%s\n\n" + printf("%1$s COMMAND [OPTIONS...] [CONFIGURATION FILE...]\n" + "\n%2$sCreate, delete, and clean up files and directories.%4$s\n" + "\n%3$sCommands:%4$s\n" + " --create Create files and directories\n" + " --clean Clean up files and directories\n" + " --remove Remove files and directories\n" " -h --help Show this help\n" - " --user Execute user configuration\n" " --version Show package version\n" + "\n%3$sOptions:%4$s\n" + " --user Execute user configuration\n" " --cat-config Show configuration files\n" " --tldr Show non-comment parts of configuration\n" - " --create Create marked files/directories\n" - " --clean Clean up marked directories\n" - " --remove Remove marked files/directories\n" " --boot Execute actions only safe at boot\n" " --graceful Quietly ignore unknown users or groups\n" " --purge Delete all files owned by the configuration files\n" @@ -4024,9 +4026,10 @@ static int help(void) { " --image-policy=POLICY Specify disk image dissection policy\n" " --replace=PATH Treat arguments as replacement for PATH\n" " --no-pager Do not pipe output into a pager\n" - "\nSee the %s for details.\n", + "\nSee the %5$s for details.\n", program_invocation_short_name, ansi_highlight(), + ansi_underline(), ansi_normal(), link); diff --git a/tools/command_ignorelist b/tools/command_ignorelist index 186136dd63..af694c16db 100644 --- a/tools/command_ignorelist +++ b/tools/command_ignorelist @@ -169,9 +169,9 @@ systemd-nspawn.xml /refsect1[title="Options"]/refsect2[title="Execution Options" systemd-nspawn.xml /refsect1[title="Options"]/refsect2[title="System Identity Options"]/variablelist/varlistentry[term="--uuid="] systemd-nspawn.xml /refsect1[title="Options"]/refsect2[title="Networking Options"]/variablelist/varlistentry[term="--private-network"] systemd-nspawn.xml /refsect1[title="Options"]/refsect2[title="Image Options"]/variablelist/varlistentry[term="--read-only"] -systemd-tmpfiles.xml /refsect1[title="Options"]/variablelist/varlistentry[term="--create"] -systemd-tmpfiles.xml /refsect1[title="Options"]/variablelist/varlistentry[term="--clean"] -systemd-tmpfiles.xml /refsect1[title="Options"]/variablelist/varlistentry[term="--remove"] +systemd-tmpfiles.xml /refsect1[title="Commands and options"]/variablelist/varlistentry[term="--create"] +systemd-tmpfiles.xml /refsect1[title="Commands and options"]/variablelist/varlistentry[term="--clean"] +systemd-tmpfiles.xml /refsect1[title="Commands and options"]/variablelist/varlistentry[term="--remove"] systemd-tmpfiles.xml /refsect1[title="Options"]/variablelist/varlistentry[term="--prefix=path"] systemd.automount.xml /refsect1[title="Options"]/variablelist/varlistentry[term="Where="] systemd.automount.xml /refsect1[title="Options"]/variablelist/varlistentry[term="DirectoryMode="] From db6873bfebf4b95e607b90d6947cfbdf92370353 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Thu, 7 Dec 2023 13:36:24 +0100 Subject: [PATCH 11/18] tmpfiles: drop unnecessary assignment --- 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 5dcdb7e406..c174d2b6d5 100644 --- a/src/tmpfiles/tmpfiles.c +++ b/src/tmpfiles/tmpfiles.c @@ -663,8 +663,8 @@ static int dir_cleanup( continue; if (r < 0) { /* FUSE, NFS mounts, SELinux might return EACCES */ - r = log_full_errno(r == -EACCES ? LOG_DEBUG : LOG_ERR, r, - "statx(%s/%s) failed: %m", p, de->d_name); + log_full_errno(r == -EACCES ? LOG_DEBUG : LOG_ERR, r, + "statx(%s/%s) failed: %m", p, de->d_name); continue; } From 7eeda1da90c79ba420a6d82c1d9589b23048d79f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Thu, 7 Dec 2023 12:56:44 +0100 Subject: [PATCH 12/18] tmpfiles: split out helper to open a directory In preparation for future commits. --- src/tmpfiles/tmpfiles.c | 102 ++++++++++++++++++++++++++-------------- 1 file changed, 68 insertions(+), 34 deletions(-) diff --git a/src/tmpfiles/tmpfiles.c b/src/tmpfiles/tmpfiles.c index c174d2b6d5..e877a5117a 100644 --- a/src/tmpfiles/tmpfiles.c +++ b/src/tmpfiles/tmpfiles.c @@ -557,6 +557,63 @@ static DIR* opendir_nomod(const char *path) { return xopendirat_nomod(AT_FDCWD, path); } +static int opendir_and_stat( + const char *path, + DIR **ret, + struct statx *ret_sx, + bool *ret_mountpoint) { + + _cleanup_closedir_ DIR *d = NULL; + STRUCT_STATX_DEFINE(sx); + int r; + + assert(path); + assert(ret); + assert(ret_sx); + assert(ret_mountpoint); + + /* Do opendir() and statx() on the directory. + * Return 1 if successful, 0 if file doesn't exist or is not a directory, + * negative errno otherwise. + */ + + d = opendir_nomod(path); + if (!d) { + bool ignore = IN_SET(errno, ENOENT, ENOTDIR); + r = log_full_errno(ignore ? LOG_DEBUG : LOG_ERR, + errno, "Failed to open directory %s: %m", path); + if (!ignore) + return r; + + *ret = NULL; + *ret_sx = (struct statx) {}; + *ret_mountpoint = NULL; + return 0; + } + + r = statx_fallback(dirfd(d), "", AT_EMPTY_PATH, STATX_MODE|STATX_INO|STATX_ATIME|STATX_MTIME, &sx); + if (r < 0) + return log_error_errno(r, "statx(%s) failed: %m", path); + + if (FLAGS_SET(sx.stx_attributes_mask, STATX_ATTR_MOUNT_ROOT)) + *ret_mountpoint = FLAGS_SET(sx.stx_attributes, STATX_ATTR_MOUNT_ROOT); + else { + struct stat ps; + + if (fstatat(dirfd(d), "..", &ps, AT_SYMLINK_NOFOLLOW) != 0) + return log_error_errno(errno, "stat(%s/..) failed: %m", path); + + *ret_mountpoint = + sx.stx_dev_major != major(ps.st_dev) || + sx.stx_dev_minor != minor(ps.st_dev) || + sx.stx_ino != ps.st_ino; + } + + *ret = TAKE_PTR(d); + *ret_sx = sx; + return 1; +} + static bool needs_cleanup( nsec_t atime, nsec_t btime, @@ -2963,49 +3020,25 @@ static int clean_item_instance( const char* instance, CreationMode creation) { - _cleanup_closedir_ DIR *d = NULL; - STRUCT_STATX_DEFINE(sx); - int mountpoint, r; - usec_t cutoff, n; - assert(i); if (!i->age_set) return 0; - n = now(CLOCK_REALTIME); + usec_t n = now(CLOCK_REALTIME); if (n < i->age) return 0; - cutoff = n - i->age; + usec_t cutoff = n - i->age; - d = opendir_nomod(instance); - if (!d) { - if (IN_SET(errno, ENOENT, ENOTDIR)) { - log_debug_errno(errno, "Directory \"%s\": %m", instance); - return 0; - } + _cleanup_closedir_ DIR *d = NULL; + STRUCT_STATX_DEFINE(sx); + bool mountpoint; + int r; - return log_error_errno(errno, "Failed to open directory %s: %m", instance); - } - - r = statx_fallback(dirfd(d), "", AT_EMPTY_PATH, STATX_MODE|STATX_INO|STATX_ATIME|STATX_MTIME, &sx); - if (r < 0) - return log_error_errno(r, "statx(%s) failed: %m", instance); - - if (FLAGS_SET(sx.stx_attributes_mask, STATX_ATTR_MOUNT_ROOT)) - mountpoint = FLAGS_SET(sx.stx_attributes, STATX_ATTR_MOUNT_ROOT); - else { - struct stat ps; - - if (fstatat(dirfd(d), "..", &ps, AT_SYMLINK_NOFOLLOW) != 0) - return log_error_errno(errno, "stat(%s/..) failed: %m", i->path); - - mountpoint = - sx.stx_dev_major != major(ps.st_dev) || - sx.stx_dev_minor != minor(ps.st_dev) || - sx.stx_ino != ps.st_ino; - } + r = opendir_and_stat(instance, &d, &sx, &mountpoint); + if (r <= 0) + return r; if (DEBUG_LOGGING) { _cleanup_free_ char *ab_f = NULL, *ab_d = NULL; @@ -3029,7 +3062,8 @@ static int clean_item_instance( statx_timestamp_load_nsec(&sx.stx_atime), statx_timestamp_load_nsec(&sx.stx_mtime), cutoff * NSEC_PER_USEC, - sx.stx_dev_major, sx.stx_dev_minor, mountpoint, + sx.stx_dev_major, sx.stx_dev_minor, + mountpoint, MAX_DEPTH, i->keep_first_level, i->age_by_file, i->age_by_dir); } 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 13/18] 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 1be6f2e4b3..07be5d932c 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 f8bdffcae3..76807b92e5 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 e877a5117a..f31235921e 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 f71a95f4f3..deee6fe5d1 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 - < Date: Wed, 24 Jan 2024 15:54:36 +0100 Subject: [PATCH 14/18] tmpfiles: simplify how a helper function is called No functional change. --- src/tmpfiles/tmpfiles.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/tmpfiles/tmpfiles.c b/src/tmpfiles/tmpfiles.c index f31235921e..0c6e28d9db 100644 --- a/src/tmpfiles/tmpfiles.c +++ b/src/tmpfiles/tmpfiles.c @@ -2580,6 +2580,9 @@ static int rm_if_wrong_type_safe( assert(!follow_links || parent_st); assert((flags & ~AT_SYMLINK_NOFOLLOW) == 0); + if (mode == 0) + return 0; + if (!filename_is_valid(name)) return log_error_errno(SYNTHETIC_ERRNO(EINVAL), "\"%s\" is not a valid filename.", name); @@ -2656,7 +2659,7 @@ static int mkdir_parents_rm_if_wrong_type(mode_t child_mode, const char *path) { if (!is_path(path)) /* rm_if_wrong_type_safe already logs errors. */ - return child_mode != 0 ? rm_if_wrong_type_safe(child_mode, AT_FDCWD, NULL, path, AT_SYMLINK_NOFOLLOW) : 0; + return rm_if_wrong_type_safe(child_mode, AT_FDCWD, NULL, path, AT_SYMLINK_NOFOLLOW); if (child_mode != 0 && endswith(path, "/")) return log_error_errno(SYNTHETIC_ERRNO(EINVAL), @@ -2686,7 +2689,7 @@ static int mkdir_parents_rm_if_wrong_type(mode_t child_mode, const char *path) { /* Is this the last component? If so, then check the type */ if (*e == 0) - return child_mode != 0 ? rm_if_wrong_type_safe(child_mode, parent_fd, &parent_st, t, AT_SYMLINK_NOFOLLOW) : 0; + return rm_if_wrong_type_safe(child_mode, parent_fd, &parent_st, t, AT_SYMLINK_NOFOLLOW); r = rm_if_wrong_type_safe(S_IFDIR, parent_fd, &parent_st, t, 0); /* Remove dangling symlinks. */ From 9e90d4f42ef7911c286a9f217b32a018159e8ed6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Fri, 8 Dec 2023 09:50:38 +0100 Subject: [PATCH 15/18] tmpfiles: implement --dry-run The idea is simple: skip the final operation that creates or removes things or changes the attributes, but otherwise go through the rest of the code. This results in quite a lot of fairly repetitive conditions in the low-level code. Another approach would be to print earlier, at a higher level, but then we'd have less precise information about what is about to happen. --- TODO | 1 - man/systemd-tmpfiles.xml | 8 + src/tmpfiles/tmpfiles.c | 312 ++++++++++++++++++++++++++------------- 3 files changed, 216 insertions(+), 105 deletions(-) diff --git a/TODO b/TODO index 07be5d932c..98d3632d75 100644 --- a/TODO +++ b/TODO @@ -2451,7 +2451,6 @@ Features: - creating new directories/subvolumes/fifos/device nodes should not follow symlinks. None of the other adjustment or creation calls follow symlinks. - - add --test mode - teach tmpfiles.d q/Q logic something sensible in the context of XFS/ext4 project quota - teach tmpfiles.d m/M to move / atomic move + symlink old -> new diff --git a/man/systemd-tmpfiles.xml b/man/systemd-tmpfiles.xml index 414d154c42..008bff62da 100644 --- a/man/systemd-tmpfiles.xml +++ b/man/systemd-tmpfiles.xml @@ -181,6 +181,14 @@ + + + Process the configuration and print what operations would be performed, but don't + actually change anything in the file system. + + + + Only apply rules with paths that start with diff --git a/src/tmpfiles/tmpfiles.c b/src/tmpfiles/tmpfiles.c index 0c6e28d9db..bb896dffea 100644 --- a/src/tmpfiles/tmpfiles.c +++ b/src/tmpfiles/tmpfiles.c @@ -198,6 +198,7 @@ typedef enum { } CreationMode; static CatFlags arg_cat_flags = CAT_CONFIG_OFF; +static bool arg_dry_run = false; static RuntimeScope arg_runtime_scope = RUNTIME_SCOPE_SYSTEM; static OperationMask arg_operation = 0; static bool arg_boot = false; @@ -341,6 +342,12 @@ static int log_unresolvable_specifier(const char *filename, unsigned line) { return 0; } +#define log_action(would, doing, fmt, ...) \ + log_full(arg_dry_run ? LOG_INFO : LOG_DEBUG, \ + fmt, \ + arg_dry_run ? (would) : (doing), \ + __VA_ARGS__) + static int user_config_paths(char*** ret) { _cleanup_strv_free_ char **config_dirs = NULL, **data_dirs = NULL; _cleanup_free_ char *persistent_config = NULL, *runtime_config = NULL, *data_home = NULL; @@ -801,7 +808,8 @@ static int dir_cleanup( continue; } - if (flock(dirfd(sub_dir), LOCK_EX|LOCK_NB) < 0) { + if (!arg_dry_run && + flock(dirfd(sub_dir), LOCK_EX|LOCK_NB) < 0) { log_debug_errno(errno, "Couldn't acquire shared BSD lock on directory \"%s\", skipping: %m", sub_path); continue; } @@ -834,13 +842,16 @@ static int dir_cleanup( cutoff_nsec, sub_path, age_by_dir, true)) continue; - log_debug("Removing directory \"%s\".", sub_path); - if (unlinkat(dirfd(d), de->d_name, AT_REMOVEDIR) < 0) - if (!IN_SET(errno, ENOENT, ENOTEMPTY)) - r = log_warning_errno(errno, "Failed to remove directory \"%s\", ignoring: %m", sub_path); + log_action("Would remove", "Removing", "%s directory \"%s\"", sub_path); + if (!arg_dry_run && + unlinkat(dirfd(d), de->d_name, AT_REMOVEDIR) < 0 && + !IN_SET(errno, ENOENT, ENOTEMPTY)) + r = log_warning_errno(errno, "Failed to remove directory \"%s\", ignoring: %m", sub_path); } else { - _cleanup_close_ int fd = -EBADF; + _cleanup_close_ int fd = -EBADF; /* This file descriptor is defined here so that the + * lock that is taken below is only dropped _after_ + * the unlink operation has finished. */ /* Skip files for which the sticky bit is set. These are semantics we define, and are * unknown elsewhere. See XDG_RUNTIME_DIR specification for details. */ @@ -872,7 +883,7 @@ static int dir_cleanup( continue; } - /* Keep files on this level around if this is requested */ + /* Keep files on this level if this was requested */ if (keep_this_level) { log_debug("Keeping \"%s\".", sub_path); continue; @@ -882,22 +893,25 @@ static int dir_cleanup( cutoff_nsec, sub_path, age_by_file, false)) continue; - fd = xopenat(dirfd(d), - de->d_name, - O_RDONLY|O_CLOEXEC|O_NOFOLLOW|O_NOATIME|O_NONBLOCK, - /* xopen_flags = */ 0, - /* mode = */ 0); - if (fd < 0 && !IN_SET(fd, -ENOENT, -ELOOP)) - log_warning_errno(fd, "Opening file \"%s\" failed, ignoring: %m", sub_path); - if (fd >= 0 && flock(fd, LOCK_EX|LOCK_NB) < 0 && errno == EAGAIN) { - log_debug_errno(errno, "Couldn't acquire shared BSD lock on file \"%s\", skipping: %m", sub_path); - continue; + if (!arg_dry_run) { + fd = xopenat(dirfd(d), + de->d_name, + O_RDONLY|O_CLOEXEC|O_NOFOLLOW|O_NOATIME|O_NONBLOCK, + /* xopen_flags = */ 0, + /* mode = */ 0); + if (fd < 0 && !IN_SET(fd, -ENOENT, -ELOOP)) + log_warning_errno(fd, "Opening file \"%s\" failed, ignoring: %m", sub_path); + if (fd >= 0 && flock(fd, LOCK_EX|LOCK_NB) < 0 && errno == EAGAIN) { + log_debug_errno(errno, "Couldn't acquire shared BSD lock on file \"%s\", skipping: %m", sub_path); + continue; + } } - log_debug("Removing \"%s\".", sub_path); - if (unlinkat(dirfd(d), de->d_name, 0) < 0) - if (errno != ENOENT) - r = log_warning_errno(errno, "Failed to remove \"%s\", ignoring: %m", sub_path); + log_action("Would remove", "Removing", "%s \"%s\"", sub_path); + if (!arg_dry_run && + unlinkat(dirfd(d), de->d_name, 0) < 0 && + errno != ENOENT) + r = log_warning_errno(errno, "Failed to remove \"%s\", ignoring: %m", sub_path); deleted = true; } @@ -907,16 +921,18 @@ finish: 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", - p, - FORMAT_TIMESTAMP_STYLE(self_atime_nsec / NSEC_PER_USEC, TIMESTAMP_US), - FORMAT_TIMESTAMP_STYLE(self_mtime_nsec / NSEC_PER_USEC, TIMESTAMP_US)); + log_action("Would restore", "Restoring", + "%s access and modification time on \"%s\": %s, %s", + p, + FORMAT_TIMESTAMP_STYLE(self_atime_nsec / NSEC_PER_USEC, TIMESTAMP_US), + FORMAT_TIMESTAMP_STYLE(self_mtime_nsec / NSEC_PER_USEC, TIMESTAMP_US)); timespec_store_nsec(ts + 0, self_atime_nsec); timespec_store_nsec(ts + 1, self_mtime_nsec); /* Restore original directory timestamps */ - if (futimens(dirfd(d), ts) < 0) + if (!arg_dry_run && + futimens(dirfd(d), ts) < 0) log_warning_errno(errno, "Failed to revert timestamps of '%s', ignoring: %m", p); } @@ -1030,18 +1046,23 @@ static int fd_set_perms( if (((m ^ st->st_mode) & 07777) == 0) log_debug("\"%s\" matches temporary mode %o already.", path, m); else { - log_debug("Temporarily changing \"%s\" to mode %o.", path, m); - r = fchmod_opath(fd, m); - if (r < 0) - return log_error_errno(r, "fchmod() of %s failed: %m", path); + log_action("Would temporarily change", "Temporarily changing", + "%s \"%s\" to mode %o", path, m); + if (!arg_dry_run) { + r = fchmod_opath(fd, m); + if (r < 0) + return log_error_errno(r, "fchmod() of %s failed: %m", path); + } } } } if (do_chown) { - log_debug("Changing \"%s\" to owner "UID_FMT":"GID_FMT, path, new_uid, new_gid); + log_action("Would change", "Changing", + "%s \"%s\" to owner "UID_FMT":"GID_FMT, path, new_uid, new_gid); - if (fchownat(fd, "", + if (!arg_dry_run && + fchownat(fd, "", new_uid != st->st_uid ? new_uid : UID_INVALID, new_gid != st->st_gid ? new_gid : GID_INVALID, AT_EMPTY_PATH) < 0) @@ -1054,10 +1075,12 @@ static int fd_set_perms( if (S_ISLNK(st->st_mode)) log_debug("Skipping mode fix for symlink %s.", path); else { - log_debug("Changing \"%s\" to mode %o.", path, new_mode); - r = fchmod_opath(fd, new_mode); - if (r < 0) - return log_error_errno(r, "fchmod() of %s failed: %m", path); + log_action("Would change", "Changing", "%s \"%s\" to mode %o", path, new_mode); + if (!arg_dry_run) { + r = fchmod_opath(fd, new_mode); + if (r < 0) + return log_error_errno(r, "fchmod() of %s failed: %m", path); + } } } @@ -1187,8 +1210,11 @@ static int fd_set_xattrs( assert(path); STRV_FOREACH_PAIR(name, value, i->xattrs) { - log_debug("Setting extended attribute '%s=%s' on %s.", *name, *value, path); - if (setxattr(FORMAT_PROC_FD_PATH(fd), *name, *value, strlen(*value), 0) < 0) + log_action("Would set", "Setting", + "%s extended attribute '%s=%s' on %s", *name, *value, path); + + if (!arg_dry_run && + setxattr(FORMAT_PROC_FD_PATH(fd), *name, *value, strlen(*value), 0) < 0) return log_error_errno(errno, "Setting extended attribute %s=%s on %s failed: %m", *name, *value, path); } @@ -1400,12 +1426,13 @@ static int path_set_acl( return r; t = acl_to_any_text(dup, NULL, ',', TEXT_ABBREVIATE); - log_debug("Setting %s ACL %s on %s.", - type == ACL_TYPE_ACCESS ? "access" : "default", - strna(t), pretty); + log_action("Would set", "Setting", + "%s %s ACL %s on %s", + type == ACL_TYPE_ACCESS ? "access" : "default", + strna(t), pretty); - r = acl_set_file(path, type, dup); - if (r < 0) { + if (!arg_dry_run && + acl_set_file(path, type, dup) < 0) { if (ERRNO_IS_NOT_SUPPORTED(errno)) /* No error if filesystem doesn't support ACLs. Return negative. */ return -errno; @@ -1603,7 +1630,6 @@ static int fd_set_attribute( const struct stat *st, CreationMode creation) { - _cleanup_close_ int procfs_fd = -EBADF; struct stat stbuf; unsigned f; int r; @@ -1635,20 +1661,29 @@ static int fd_set_attribute( if (!S_ISDIR(st->st_mode)) f &= ~FS_DIRSYNC_FL; - procfs_fd = fd_reopen(fd, O_RDONLY|O_CLOEXEC|O_NOATIME); - if (procfs_fd < 0) - return log_error_errno(procfs_fd, "Failed to reopen '%s': %m", path); + log_action("Would try to set", "Trying to set", + "%s file attributes 0x%08x on %s", + f & item->attribute_mask, + path); - unsigned previous, current; - r = chattr_full(procfs_fd, NULL, f, item->attribute_mask, &previous, ¤t, CHATTR_FALLBACK_BITWISE); - if (r == -ENOANO) - log_warning("Cannot set file attributes for '%s', maybe due to incompatibility in specified attributes, " - "previous=0x%08x, current=0x%08x, expected=0x%08x, ignoring.", - path, previous, current, (previous & ~item->attribute_mask) | (f & item->attribute_mask)); - else if (r < 0) - log_full_errno(ERRNO_IS_NOT_SUPPORTED(r) ? LOG_DEBUG : LOG_WARNING, r, - "Cannot set file attributes for '%s', value=0x%08x, mask=0x%08x, ignoring: %m", - path, item->attribute_value, item->attribute_mask); + if (!arg_dry_run) { + _cleanup_close_ int procfs_fd = -EBADF; + + procfs_fd = fd_reopen(fd, O_RDONLY|O_CLOEXEC|O_NOATIME); + if (procfs_fd < 0) + return log_error_errno(procfs_fd, "Failed to reopen '%s': %m", path); + + unsigned previous, current; + r = chattr_full(procfs_fd, NULL, f, item->attribute_mask, &previous, ¤t, CHATTR_FALLBACK_BITWISE); + if (r == -ENOANO) + log_warning("Cannot set file attributes for '%s', maybe due to incompatibility in specified attributes, " + "previous=0x%08x, current=0x%08x, expected=0x%08x, ignoring.", + path, previous, current, (previous & ~item->attribute_mask) | (f & item->attribute_mask)); + else if (r < 0) + log_full_errno(ERRNO_IS_NOT_SUPPORTED(r) ? LOG_DEBUG : LOG_WARNING, r, + "Cannot set file attributes for '%s', value=0x%08x, mask=0x%08x, ignoring: %m", + path, item->attribute_value, item->attribute_mask); + } return 0; } @@ -1686,11 +1721,13 @@ static int write_argument_data(Item *i, int fd, const char *path) { assert(item_binary_argument(i)); - log_debug("Writing to \"%s\".", path); + log_action("Would write", "Writing", "%s to \"%s\"", path); - r = loop_write(fd, item_binary_argument(i), item_binary_argument_size(i)); - if (r < 0) - return log_error_errno(r, "Failed to write file \"%s\": %m", path); + if (!arg_dry_run) { + r = loop_write(fd, item_binary_argument(i), item_binary_argument_size(i)); + if (r < 0) + return log_error_errno(r, "Failed to write file \"%s\": %m", path); + } return 0; } @@ -1717,10 +1754,9 @@ static int write_one_file(Context *c, Item *i, const char *path, CreationMode cr if (dir_fd < 0) return dir_fd; - /* Follows symlinks */ - fd = openat(dir_fd, bn, - O_NONBLOCK|O_CLOEXEC|O_WRONLY|O_NOCTTY|(i->append_or_force ? O_APPEND : 0), - i->mode); + /* Follow symlinks. Open with O_PATH in dry-run mode to make sure we don't use the path inadvertently. */ + int flags = O_NONBLOCK | O_CLOEXEC | O_WRONLY | O_NOCTTY | i->append_or_force * O_APPEND | arg_dry_run * O_PATH; + fd = openat(dir_fd, bn, flags, i->mode); if (fd < 0) { if (errno == ENOENT) { log_debug_errno(errno, "Not writing missing file \"%s\": %m", path); @@ -1766,6 +1802,14 @@ static int create_file( if (r == O_DIRECTORY) return log_error_errno(SYNTHETIC_ERRNO(EISDIR), "Cannot open path '%s' for writing, is a directory.", path); + if (arg_dry_run) { + log_info("Would create file %s", path); + return 0; + + /* The opening of the directory below would fail if it doesn't exist, + * so log and exit before even trying to do that. */ + } + /* Validate the path and keep the fd on the directory for opening the file so we're sure that it * can't be changed behind our back. */ dir_fd = path_open_parent_safe(path, i->allow_failure); @@ -1845,6 +1889,11 @@ static int truncate_file( if (dir_fd < 0) return dir_fd; + if (arg_dry_run) { + log_info("Would truncate %s", path); + return 0; + } + creation = CREATION_EXISTING; fd = RET_NERRNO(openat(dir_fd, bn, O_NOFOLLOW|O_NONBLOCK|O_CLOEXEC|O_WRONLY|O_NOCTTY, i->mode)); if (fd == -ENOENT) { @@ -1912,7 +1961,9 @@ static int copy_files(Context *c, Item *i) { struct stat st, a; int r; - log_debug("Copying tree \"%s\" to \"%s\".", i->argument, i->path); + log_action("Would copy", "Copying", "%s tree \"%s\" to \"%s\"", i->argument, i->path); + if (arg_dry_run) + return 0; r = path_extract_filename(i->path, &bn); if (r < 0) @@ -1991,18 +2042,27 @@ static int create_directory_or_subvolume( * heavy-weight). Thus, chroot() environments and suchlike will get a full brtfs * subvolume set up below their tree only if they specifically set up a btrfs * subvolume for the root dir too. */ - subvol = false; else { - WITH_UMASK((~mode) & 0777) - r = btrfs_subvol_make(pfd, bn); + log_action("Would create", "Creating", "%s btrfs subvolume %s", path); + if (!arg_dry_run) + WITH_UMASK((~mode) & 0777) + r = btrfs_subvol_make(pfd, bn); + else + r = 0; } } else r = 0; - if (!subvol || ERRNO_IS_NEG_NOT_SUPPORTED(r)) - WITH_UMASK(0000) - r = mkdirat_label(pfd, bn, mode); + if (!subvol || ERRNO_IS_NEG_NOT_SUPPORTED(r)) { + log_action("Would create", "Creating", "%s directory \"%s\"", path); + if (!arg_dry_run) + WITH_UMASK(0000) + r = mkdirat_label(pfd, bn, mode); + } + + if (arg_dry_run) + return 0; creation = r >= 0 ? CREATION_NORMAL : CREATION_EXISTING; @@ -2051,6 +2111,11 @@ static int create_directory( assert(i); assert(IN_SET(i->type, CREATE_DIRECTORY, TRUNCATE_DIRECTORY)); + if (arg_dry_run) { + log_info("Would create directory %s", path); + return 0; + } + fd = create_directory_or_subvolume(path, i->mode, /* subvol= */ false, i->allow_failure, &st, &creation); if (fd == -EEXIST) return 0; @@ -2074,6 +2139,11 @@ static int create_subvolume( assert(i); assert(IN_SET(i->type, CREATE_SUBVOLUME, CREATE_SUBVOLUME_NEW_QUOTA, CREATE_SUBVOLUME_INHERIT_QUOTA)); + if (arg_dry_run) { + log_info("Would create subvolume %s", path); + return 0; + } + fd = create_directory_or_subvolume(path, i->mode, /* subvol = */ true, i->allow_failure, &st, &creation); if (fd == -EEXIST) return 0; @@ -2163,6 +2233,11 @@ static int create_device( return log_error_errno(SYNTHETIC_ERRNO(EISDIR), "Cannot open path '%s' for creating device node, is a directory.", i->path); + if (arg_dry_run) { + log_info("Would create device node %s", i->path); + return 0; + } + /* Validate the path and use the returned directory fd for copying the target so we're sure that the * path can't be changed behind our back. */ dfd = path_open_parent_safe(i->path, i->allow_failure); @@ -2270,6 +2345,11 @@ static int create_fifo(Context *c, Item *i) { return log_error_errno(SYNTHETIC_ERRNO(EISDIR), "Cannot open path '%s' for creating FIFO, is a directory.", i->path); + if (arg_dry_run) { + log_info("Would create fifo %s", i->path); + return 0; + } + pfd = path_open_parent_safe(i->path, i->allow_failure); if (pfd < 0) return pfd; @@ -2358,6 +2438,11 @@ static int create_symlink(Context *c, Item *i) { return log_error_errno(SYNTHETIC_ERRNO(EISDIR), "Cannot open path '%s' for creating FIFO, is a directory.", i->path); + if (arg_dry_run) { + log_info("Would create symlink %s -> %s", i->path, i->argument); + return 0; + } + pfd = path_open_parent_safe(i->path, i->allow_failure); if (pfd < 0) return pfd; @@ -2590,58 +2675,61 @@ static int rm_if_wrong_type_safe( if (r < 0) { (void) fd_get_path(parent_fd, &parent_name); return log_full_errno(r == -ENOENT? LOG_DEBUG : LOG_ERR, r, - "Failed to stat \"%s\" at \"%s\": %m", name, strna(parent_name)); + "Failed to stat \"%s/%s\": %m", parent_name ?: "...", name); } /* Fail before removing anything if this is an unsafe transition. */ if (follow_links && unsafe_transition(parent_st, &st)) { (void) fd_get_path(parent_fd, &parent_name); return log_error_errno(SYNTHETIC_ERRNO(ENOLINK), - "Unsafe transition from \"%s\" to \"%s\".", parent_name, name); + "Unsafe transition from \"%s\" to \"%s\".", parent_name ?: "...", name); } if ((st.st_mode & S_IFMT) == mode) return 0; (void) fd_get_path(parent_fd, &parent_name); - log_notice("Wrong file type 0o%o; rm -rf \"%s/%s\"", st.st_mode & S_IFMT, strna(parent_name), name); + log_notice("Wrong file type 0o%o; rm -rf \"%s/%s\"", st.st_mode & S_IFMT, parent_name ?: "...", name); /* If the target of the symlink was the wrong type, the link needs to be removed instead of the * target, so make sure it is identified as a link and not a directory. */ if (follow_links) { r = fstatat_harder(parent_fd, name, &st, AT_SYMLINK_NOFOLLOW, REMOVE_CHMOD | REMOVE_CHMOD_RESTORE); if (r < 0) - return log_error_errno(r, "Failed to stat \"%s\" at \"%s\": %m", name, strna(parent_name)); + return log_error_errno(r, "Failed to stat \"%s/%s\": %m", parent_name ?: "...", name); } /* Do not remove mount points. */ r = fd_is_mount_point(parent_fd, name, follow_links ? AT_SYMLINK_FOLLOW : 0); if (r < 0) - (void) log_warning_errno(r, "Failed to check if \"%s/%s\" is a mount point: %m; Continuing", - strna(parent_name), name); + (void) log_warning_errno(r, "Failed to check if \"%s/%s\" is a mount point: %m; continuing.", + parent_name ?: "...", name); else if (r > 0) return log_error_errno(SYNTHETIC_ERRNO(EBUSY), - "Not removing \"%s/%s\" because it is a mount point.", strna(parent_name), name); + "Not removing \"%s/%s\" because it is a mount point.", parent_name ?: "...", name); - if ((st.st_mode & S_IFMT) == S_IFDIR) { - _cleanup_close_ int child_fd = -EBADF; + log_action("Would remove", "Removing", "%s %s/%s", parent_name ?: "...", name); + if (!arg_dry_run) { + if ((st.st_mode & S_IFMT) == S_IFDIR) { + _cleanup_close_ int child_fd = -EBADF; - child_fd = openat(parent_fd, name, O_NOCTTY | O_CLOEXEC | O_DIRECTORY); - if (child_fd < 0) - return log_error_errno(errno, "Failed to open \"%s\" at \"%s\": %m", name, strna(parent_name)); + child_fd = openat(parent_fd, name, O_NOCTTY | O_CLOEXEC | O_DIRECTORY); + if (child_fd < 0) + return log_error_errno(errno, "Failed to open \"%s/%s\": %m", parent_name ?: "...", name); - r = rm_rf_children(TAKE_FD(child_fd), REMOVE_ROOT|REMOVE_SUBVOLUME|REMOVE_PHYSICAL, &st); + r = rm_rf_children(TAKE_FD(child_fd), REMOVE_ROOT|REMOVE_SUBVOLUME|REMOVE_PHYSICAL, &st); + if (r < 0) + return log_error_errno(r, "Failed to remove contents of \"%s/%s\": %m", parent_name ?: "...", name); + + r = unlinkat_harder(parent_fd, name, AT_REMOVEDIR, REMOVE_CHMOD | REMOVE_CHMOD_RESTORE); + } else + r = unlinkat_harder(parent_fd, name, 0, REMOVE_CHMOD | REMOVE_CHMOD_RESTORE); if (r < 0) - return log_error_errno(r, "Failed to remove contents of \"%s\" at \"%s\": %m", name, strna(parent_name)); + return log_error_errno(r, "Failed to remove \"%s/%s\": %m", parent_name ?: "...", name); + } - r = unlinkat_harder(parent_fd, name, AT_REMOVEDIR, REMOVE_CHMOD | REMOVE_CHMOD_RESTORE); - } else - r = unlinkat_harder(parent_fd, name, 0, REMOVE_CHMOD | REMOVE_CHMOD_RESTORE); - if (r < 0) - return log_error_errno(r, "Failed to remove \"%s\" at \"%s\": %m", name, strna(parent_name)); - - /* This is covered by the log_notice "Wrong file type..." It is logged earlier because it gives - * context to other error messages that might follow. */ + /* This is covered by the log_notice "Wrong file type...". + * It is logged earlier because it gives context to other error messages that might follow. */ return -ENOENT; } @@ -2696,13 +2784,15 @@ static int mkdir_parents_rm_if_wrong_type(mode_t child_mode, const char *path) { if (r == -ENOENT) r = rm_if_wrong_type_safe(S_IFDIR, parent_fd, &parent_st, t, AT_SYMLINK_NOFOLLOW); if (r == -ENOENT) { - WITH_UMASK(0000) - r = mkdirat_label(parent_fd, t, 0755); - if (r < 0) { - _cleanup_free_ char *parent_name = NULL; + if (!arg_dry_run) { + WITH_UMASK(0000) + r = mkdirat_label(parent_fd, t, 0755); + if (r < 0) { + _cleanup_free_ char *parent_name = NULL; - (void) fd_get_path(parent_fd, &parent_name); - return log_error_errno(r, "Failed to mkdir \"%s\" at \"%s\": %m", t, strnull(parent_name)); + (void) fd_get_path(parent_fd, &parent_name); + return log_error_errno(r, "Failed to mkdir \"%s\" at \"%s\": %m", t, strnull(parent_name)); + } } } else if (r < 0) /* rm_if_wrong_type_safe already logs errors. */ @@ -2729,13 +2819,15 @@ static int mkdir_parents_rm_if_wrong_type(mode_t child_mode, const char *path) { static int mkdir_parents_item(Item *i, mode_t child_mode) { int r; + if (i->try_replace) { r = mkdir_parents_rm_if_wrong_type(child_mode, i->path); if (r < 0 && r != -ENOENT) return r; } else WITH_UMASK(0000) - (void) mkdir_parents_label(i->path, 0755); + if (!arg_dry_run) + (void) mkdir_parents_label(i->path, 0755); return 0; } @@ -2926,8 +3018,10 @@ static int remove_recursive( return r; if (r == 0) { if (remove_instance) { - log_debug("Removing file \"%s\".", instance); - if (remove(instance) < 0 && errno != ENOENT) + log_action("Would remove", "Removing", "%s file \"%s\".", instance); + if (!arg_dry_run && + remove(instance) < 0 && + errno != ENOENT) return log_error_errno(errno, "rm %s: %m", instance); } return 0; @@ -2985,7 +3079,10 @@ static int remove_item_instance( switch (i->type) { case REMOVE_PATH: - if (remove(instance) < 0 && errno != ENOENT) + log_action("Would remove", "Removing", "%s \"%s\".", instance); + if (!arg_dry_run && + remove(instance) < 0 && + errno != ENOENT) return log_error_errno(errno, "rm %s: %m", instance); return 0; @@ -4080,6 +4177,7 @@ static int help(void) { " --image=PATH Operate on disk image as filesystem root\n" " --image-policy=POLICY Specify disk image dissection policy\n" " --replace=PATH Treat arguments as replacement for PATH\n" + " --dry-run Just print what would be done\n" " --no-pager Do not pipe output into a pager\n" "\nSee the %5$s for details.\n", program_invocation_short_name, @@ -4109,6 +4207,7 @@ static int parse_argv(int argc, char *argv[]) { ARG_IMAGE, ARG_IMAGE_POLICY, ARG_REPLACE, + ARG_DRY_RUN, ARG_NO_PAGER, }; @@ -4130,6 +4229,7 @@ static int parse_argv(int argc, char *argv[]) { { "image", required_argument, NULL, ARG_IMAGE }, { "image-policy", required_argument, NULL, ARG_IMAGE_POLICY }, { "replace", required_argument, NULL, ARG_REPLACE }, + { "dry-run", no_argument, NULL, ARG_DRY_RUN }, { "no-pager", no_argument, NULL, ARG_NO_PAGER }, {} }; @@ -4237,6 +4337,10 @@ static int parse_argv(int argc, char *argv[]) { arg_replace = optarg; break; + case ARG_DRY_RUN: + arg_dry_run = true; + break; + case ARG_NO_PAGER: arg_pager_flags |= PAGER_DISABLE; break; From 983dbcefe8574798856657cebf65c8f0c8927f7b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Fri, 15 Dec 2023 13:27:44 +0100 Subject: [PATCH 16/18] systemd-tmpfiles: use statx_mount_same It was pointed out in review that the preexisting code should be updated (https://github.com/systemd/systemd/pull/30380#discussion_r1426899180). --- src/tmpfiles/tmpfiles.c | 22 ++++++++++------------ 1 file changed, 10 insertions(+), 12 deletions(-) diff --git a/src/tmpfiles/tmpfiles.c b/src/tmpfiles/tmpfiles.c index bb896dffea..702e4ce7cd 100644 --- a/src/tmpfiles/tmpfiles.c +++ b/src/tmpfiles/tmpfiles.c @@ -571,7 +571,7 @@ static int opendir_and_stat( bool *ret_mountpoint) { _cleanup_closedir_ DIR *d = NULL; - STRUCT_STATX_DEFINE(sx); + STRUCT_NEW_STATX_DEFINE(st1); int r; assert(path); @@ -598,26 +598,24 @@ static int opendir_and_stat( return 0; } - r = statx_fallback(dirfd(d), "", AT_EMPTY_PATH, STATX_MODE|STATX_INO|STATX_ATIME|STATX_MTIME, &sx); + r = statx_fallback(dirfd(d), "", AT_EMPTY_PATH, STATX_MODE|STATX_INO|STATX_ATIME|STATX_MTIME, &st1.sx); if (r < 0) return log_error_errno(r, "statx(%s) failed: %m", path); - if (FLAGS_SET(sx.stx_attributes_mask, STATX_ATTR_MOUNT_ROOT)) - *ret_mountpoint = FLAGS_SET(sx.stx_attributes, STATX_ATTR_MOUNT_ROOT); + if (FLAGS_SET(st1.sx.stx_attributes_mask, STATX_ATTR_MOUNT_ROOT)) + *ret_mountpoint = FLAGS_SET(st1.sx.stx_attributes, STATX_ATTR_MOUNT_ROOT); else { - struct stat ps; + STRUCT_NEW_STATX_DEFINE(st2); - if (fstatat(dirfd(d), "..", &ps, AT_SYMLINK_NOFOLLOW) != 0) - return log_error_errno(errno, "stat(%s/..) failed: %m", path); + r = statx_fallback(dirfd(d), "..", 0, STATX_INO, &st2.sx); + if (r < 0) + return log_error_errno(r, "statx(%s/..) failed: %m", path); - *ret_mountpoint = - sx.stx_dev_major != major(ps.st_dev) || - sx.stx_dev_minor != minor(ps.st_dev) || - sx.stx_ino != ps.st_ino; + *ret_mountpoint = !statx_mount_same(&st1.nsx, &st2.nsx); } *ret = TAKE_PTR(d); - *ret_sx = sx; + *ret_sx = st1.sx; return 1; } From b214427752b214c1c41493a7c8c71328068db185 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Fri, 8 Dec 2023 10:17:04 +0100 Subject: [PATCH 17/18] TEST-22: add --dry-run calls aCdDefLprRwxXz are tested with --dry-run. I added a primitive test of bc. There were no tests for AhHt, and I didn't add those either. --- test/units/testsuite-22.02.sh | 16 +++++++ test/units/testsuite-22.03.sh | 29 +++++++++++ test/units/testsuite-22.04.sh | 6 +++ test/units/testsuite-22.05.sh | 9 ++++ test/units/testsuite-22.06.sh | 9 +++- test/units/testsuite-22.11.sh | 90 +++++++++++++++++++++++++++++++++++ test/units/testsuite-22.12.sh | 10 ++++ test/units/testsuite-22.15.sh | 12 +++++ test/units/testsuite-22.16.sh | 5 ++ test/units/testsuite-22.18.sh | 21 ++++---- test/units/testsuite-22.19.sh | 29 +++++++++++ 11 files changed, 225 insertions(+), 11 deletions(-) create mode 100755 test/units/testsuite-22.19.sh diff --git a/test/units/testsuite-22.02.sh b/test/units/testsuite-22.02.sh index b883a9672e..f191ae3ee7 100755 --- a/test/units/testsuite-22.02.sh +++ b/test/units/testsuite-22.02.sh @@ -14,6 +14,14 @@ mkdir /tmp/{C,d,D,e} mkdir /tmp/d/2 chmod 777 /tmp/d/2 +systemd-tmpfiles --dry-run --create - < Date: Thu, 14 Dec 2023 15:32:45 +0100 Subject: [PATCH 18/18] TODO: add --dry-run/-n --- TODO | 1 + 1 file changed, 1 insertion(+) diff --git a/TODO b/TODO index 98d3632d75..a690f36169 100644 --- a/TODO +++ b/TODO @@ -2456,6 +2456,7 @@ Features: - teach tmpfiles.d m/M to move / atomic move + symlink old -> new - add new line type for setting btrfs subvolume attributes (i.e. rw/ro) - tmpfiles: add new line type for setting fcaps + - add -n as shortcut for --dry-run in tmpfiles & sysusers & possibly other places * udev-link-config: - Make sure ID_PATH is always exported and complete for