From 93419a9601a0874a8304d0b6fe11a2f5c40c48b6 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 17 Feb 2021 10:38:12 +0100 Subject: [PATCH 1/2] install: make UnitFileChangeType enum anonymous We almost never use the named enum type, in almost all cases we use "int" instead, since we overload it with negative errnos. To simplify things, let's use "int" really everywhere. Moreover, let's rename the fields for this enum to "type_or_errno", to make the overloading clear. And let's ad some assertions that things are in the right range. --- src/core/dbus-manager.c | 13 ++-- src/shared/bus-unit-util.c | 2 +- src/shared/install.c | 56 ++++++++++------- src/shared/install.h | 22 +++---- src/systemctl/systemctl-is-enabled.c | 2 +- src/test/test-install-root.c | 94 ++++++++++++++-------------- src/test/test-install.c | 4 +- 7 files changed, 103 insertions(+), 90 deletions(-) diff --git a/src/core/dbus-manager.c b/src/core/dbus-manager.c index 86d0b3744d0..f54e73684e8 100644 --- a/src/core/dbus-manager.c +++ b/src/core/dbus-manager.c @@ -2062,9 +2062,9 @@ static int install_error( for (size_t i = 0; i < n_changes; i++) - switch(changes[i].type) { + switch(changes[i].type_or_errno) { - case 0 ... INT_MAX: + case 0 ... _UNIT_FILE_CHANGE_TYPE_MAX: /* not errors */ continue; case -EEXIST: @@ -2106,7 +2106,8 @@ static int install_error( goto found; default: - r = sd_bus_error_set_errnof(error, changes[i].type, "File %s: %m", changes[i].path); + assert(changes[i].type_or_errno < 0); /* other errors */ + r = sd_bus_error_set_errnof(error, changes[i].type_or_errno, "File %s: %m", changes[i].path); goto found; } @@ -2151,14 +2152,14 @@ static int reply_unit_file_changes_and_free( for (size_t i = 0; i < n_changes; i++) { - if (changes[i].type < 0) { + if (changes[i].type_or_errno < 0) { bad = true; continue; } r = sd_bus_message_append( reply, "(sss)", - unit_file_change_type_to_string(changes[i].type), + unit_file_change_type_to_string(changes[i].type_or_errno), changes[i].path, changes[i].source); if (r < 0) @@ -2542,7 +2543,7 @@ static int method_get_unit_file_links(sd_bus_message *message, void *userdata, s return log_error_errno(r, "Failed to get file links for %s: %m", name); for (i = 0; i < n_changes; i++) - if (changes[i].type == UNIT_FILE_UNLINK) { + if (changes[i].type_or_errno == UNIT_FILE_UNLINK) { r = sd_bus_message_append(reply, "s", changes[i].path); if (r < 0) return r; diff --git a/src/shared/bus-unit-util.c b/src/shared/bus-unit-util.c index 61d264b9537..548dc57e3ce 100644 --- a/src/shared/bus-unit-util.c +++ b/src/shared/bus-unit-util.c @@ -2388,7 +2388,7 @@ int bus_deserialize_and_dump_unit_file_changes(sd_bus_message *m, bool quiet, Un while ((r = sd_bus_message_read(m, "(sss)", &type, &path, &source)) > 0) { /* We expect only "success" changes to be sent over the bus. Hence, reject anything negative. */ - UnitFileChangeType ch = unit_file_change_type_from_string(type); + int ch = unit_file_change_type_from_string(type); if (ch < 0) { log_notice_errno(ch, "Manager reported unknown change type \"%s\" for path \"%s\", ignoring.", type, path); diff --git a/src/shared/install.c b/src/shared/install.c index 77208582b7d..183da5865e0 100644 --- a/src/shared/install.c +++ b/src/shared/install.c @@ -14,6 +14,7 @@ #include "conf-parser.h" #include "def.h" #include "dirent-util.h" +#include "errno-list.h" #include "extract-word.h" #include "fd-util.h" #include "fileio.h" @@ -261,7 +262,7 @@ static const char* config_path_from_flags(const LookupPaths *paths, UnitFileFlag int unit_file_changes_add( UnitFileChange **changes, size_t *n_changes, - int type, + int type_or_errno, /* UNIT_FILE_SYMLINK, _UNLINK, _IS_MASKED, _IS_DANGLING if positive or errno if negative */ const char *path, const char *source) { @@ -271,6 +272,11 @@ int unit_file_changes_add( assert(path); assert(!changes == !n_changes); + if (type_or_errno >= 0) + assert(type_or_errno < _UNIT_FILE_CHANGE_TYPE_MAX); + else + assert(type_or_errno >= -ERRNO_MAX); + if (!changes) return 0; @@ -280,19 +286,25 @@ int unit_file_changes_add( *changes = c; p = strdup(path); - if (source) - s = strdup(source); - - if (!p || (source && !s)) + if (!p) return -ENOMEM; path_simplify(p, false); - if (s) - path_simplify(s, false); - c[*n_changes] = (UnitFileChange) { type, p, s }; - p = s = NULL; - (*n_changes) ++; + if (source) { + s = strdup(source); + if (!s) + return -ENOMEM; + + path_simplify(s, false); + } + + c[(*n_changes)++] = (UnitFileChange) { + .type_or_errno = type_or_errno, + .path = TAKE_PTR(p), + .source = TAKE_PTR(s), + }; + return 0; } @@ -315,9 +327,9 @@ void unit_file_dump_changes(int r, const char *verb, const UnitFileChange *chang assert(verb || r >= 0); for (size_t i = 0; i < n_changes; i++) { - assert(verb || changes[i].type >= 0); + assert(verb || changes[i].type_or_errno >= 0); - switch(changes[i].type) { + switch(changes[i].type_or_errno) { case UNIT_FILE_SYMLINK: if (!quiet) log_info("Created symlink %s %s %s.", @@ -340,45 +352,45 @@ void unit_file_dump_changes(int r, const char *verb, const UnitFileChange *chang break; case -EEXIST: if (changes[i].source) - log_error_errno(changes[i].type, + log_error_errno(changes[i].type_or_errno, "Failed to %s unit, file %s already exists and is a symlink to %s.", verb, changes[i].path, changes[i].source); else - log_error_errno(changes[i].type, + log_error_errno(changes[i].type_or_errno, "Failed to %s unit, file %s already exists.", verb, changes[i].path); logged = true; break; case -ERFKILL: - log_error_errno(changes[i].type, "Failed to %s unit, unit %s is masked.", + log_error_errno(changes[i].type_or_errno, "Failed to %s unit, unit %s is masked.", verb, changes[i].path); logged = true; break; case -EADDRNOTAVAIL: - log_error_errno(changes[i].type, "Failed to %s unit, unit %s is transient or generated.", + log_error_errno(changes[i].type_or_errno, "Failed to %s unit, unit %s is transient or generated.", verb, changes[i].path); logged = true; break; case -EUCLEAN: - log_error_errno(changes[i].type, + log_error_errno(changes[i].type_or_errno, "Failed to %s unit, \"%s\" is not a valid unit name.", verb, changes[i].path); logged = true; break; case -ELOOP: - log_error_errno(changes[i].type, "Failed to %s unit, refusing to operate on linked unit file %s", + log_error_errno(changes[i].type_or_errno, "Failed to %s unit, refusing to operate on linked unit file %s", verb, changes[i].path); logged = true; break; case -ENOENT: - log_error_errno(changes[i].type, "Failed to %s unit, unit %s does not exist.", verb, changes[i].path); + log_error_errno(changes[i].type_or_errno, "Failed to %s unit, unit %s does not exist.", verb, changes[i].path); logged = true; break; default: - assert(changes[i].type < 0); - log_error_errno(changes[i].type, "Failed to %s unit, file %s: %m.", + assert(changes[i].type_or_errno < 0); + log_error_errno(changes[i].type_or_errno, "Failed to %s unit, file %s: %m.", verb, changes[i].path); logged = true; } @@ -3464,7 +3476,7 @@ static const char* const unit_file_change_type_table[_UNIT_FILE_CHANGE_TYPE_MAX] [UNIT_FILE_IS_DANGLING] = "dangling", }; -DEFINE_STRING_TABLE_LOOKUP(unit_file_change_type, UnitFileChangeType); +DEFINE_STRING_TABLE_LOOKUP(unit_file_change_type, int); static const char* const unit_file_preset_mode_table[_UNIT_FILE_PRESET_MAX] = { [UNIT_FILE_PRESET_FULL] = "full", diff --git a/src/shared/install.h b/src/shared/install.h index 232184de33b..305e4f74b8f 100644 --- a/src/shared/install.h +++ b/src/shared/install.h @@ -25,7 +25,9 @@ enum UnitFilePresetMode { _UNIT_FILE_PRESET_INVALID = -EINVAL, }; -enum UnitFileChangeType { +/* This enum type is anonymous, since we usually store it in an 'int', as we overload it with negative errno + * values. */ +enum { UNIT_FILE_SYMLINK, UNIT_FILE_UNLINK, UNIT_FILE_IS_MASKED, @@ -42,20 +44,18 @@ enum UnitFileFlags { _UNIT_FILE_FLAGS_MASK_PUBLIC = UNIT_FILE_RUNTIME|UNIT_FILE_PORTABLE|UNIT_FILE_FORCE, }; -/* type can either one of the UnitFileChangeTypes listed above, or a negative error. - * If source is specified, it should be the contents of the path symlink. - * In case of an error, source should be the existing symlink contents or NULL - */ +/* type can either one of the UNIT_FILE_SYMLINK, UNIT_FILE_UNLINK, … listed above, or a negative errno value. + * If source is specified, it should be the contents of the path symlink. In case of an error, source should + * be the existing symlink contents or NULL. */ struct UnitFileChange { - int type; /* UnitFileChangeType or bust */ + int type_or_errno; /* UNIT_FILE_SYMLINK, … if positive, errno if negative */ char *path; char *source; }; static inline bool unit_file_changes_have_modification(const UnitFileChange* changes, size_t n_changes) { - size_t i; - for (i = 0; i < n_changes; i++) - if (IN_SET(changes[i].type, UNIT_FILE_SYMLINK, UNIT_FILE_UNLINK)) + for (size_t i = 0; i < n_changes; i++) + if (IN_SET(changes[i].type_or_errno, UNIT_FILE_SYMLINK, UNIT_FILE_UNLINK)) return true; return false; } @@ -206,8 +206,8 @@ const char *unit_file_state_to_string(UnitFileState s) _const_; UnitFileState unit_file_state_from_string(const char *s) _pure_; /* from_string conversion is unreliable because of the overlap between -EPERM and -1 for error. */ -const char *unit_file_change_type_to_string(UnitFileChangeType s) _const_; -UnitFileChangeType unit_file_change_type_from_string(const char *s) _pure_; +const char *unit_file_change_type_to_string(int s) _const_; +int unit_file_change_type_from_string(const char *s) _pure_; const char *unit_file_preset_mode_to_string(UnitFilePresetMode m) _const_; UnitFilePresetMode unit_file_preset_mode_from_string(const char *s) _pure_; diff --git a/src/systemctl/systemctl-is-enabled.c b/src/systemctl/systemctl-is-enabled.c index de1d016bb3b..e33dffaf299 100644 --- a/src/systemctl/systemctl-is-enabled.c +++ b/src/systemctl/systemctl-is-enabled.c @@ -23,7 +23,7 @@ static int show_installation_targets_client_side(const char *name) { return log_error_errno(r, "Failed to get file links for %s: %m", name); for (size_t i = 0; i < n_changes; i++) - if (changes[i].type == UNIT_FILE_UNLINK) + if (changes[i].type_or_errno == UNIT_FILE_UNLINK) printf(" %s\n", changes[i].path); return 0; diff --git a/src/test/test-install-root.c b/src/test/test-install-root.c index aedec54a7f8..04125f1db1f 100644 --- a/src/test/test-install-root.c +++ b/src/test/test-install-root.c @@ -55,7 +55,7 @@ static void test_basic_mask_and_enable(const char *root) { assert_se(unit_file_mask(UNIT_FILE_SYSTEM, 0, root, STRV_MAKE("a.service"), &changes, &n_changes) >= 0); assert_se(n_changes == 1); - assert_se(changes[0].type == UNIT_FILE_SYMLINK); + assert_se(changes[0].type_or_errno == UNIT_FILE_SYMLINK); assert_se(streq(changes[0].source, "/dev/null")); p = strjoina(root, SYSTEM_CONFIG_UNIT_DIR"/a.service"); assert_se(streq(changes[0].path, p)); @@ -75,7 +75,7 @@ static void test_basic_mask_and_enable(const char *root) { assert_se(unit_file_unmask(UNIT_FILE_SYSTEM, 0, root, STRV_MAKE("a.service"), &changes, &n_changes) >= 0); assert_se(n_changes == 1); - assert_se(changes[0].type == UNIT_FILE_UNLINK); + assert_se(changes[0].type_or_errno == UNIT_FILE_UNLINK); p = strjoina(root, SYSTEM_CONFIG_UNIT_DIR"/a.service"); assert_se(streq(changes[0].path, p)); unit_file_changes_free(changes, n_changes); @@ -83,7 +83,7 @@ static void test_basic_mask_and_enable(const char *root) { assert_se(unit_file_enable(UNIT_FILE_SYSTEM, 0, root, STRV_MAKE("a.service"), &changes, &n_changes) == 1); assert_se(n_changes == 1); - assert_se(changes[0].type == UNIT_FILE_SYMLINK); + assert_se(changes[0].type_or_errno == UNIT_FILE_SYMLINK); assert_se(streq(changes[0].source, "/usr/lib/systemd/system/a.service")); p = strjoina(root, SYSTEM_CONFIG_UNIT_DIR"/multi-user.target.wants/a.service"); assert_se(streq(changes[0].path, p)); @@ -103,7 +103,7 @@ static void test_basic_mask_and_enable(const char *root) { assert_se(unit_file_disable(UNIT_FILE_SYSTEM, 0, root, STRV_MAKE("a.service"), &changes, &n_changes) >= 0); assert_se(n_changes == 1); - assert_se(changes[0].type == UNIT_FILE_UNLINK); + assert_se(changes[0].type_or_errno == UNIT_FILE_UNLINK); p = strjoina(root, SYSTEM_CONFIG_UNIT_DIR"/multi-user.target.wants/a.service"); assert_se(streq(changes[0].path, p)); unit_file_changes_free(changes, n_changes); @@ -123,7 +123,7 @@ static void test_basic_mask_and_enable(const char *root) { /* Let's enable this indirectly via a symlink */ assert_se(unit_file_enable(UNIT_FILE_SYSTEM, 0, root, STRV_MAKE("d.service"), &changes, &n_changes) >= 0); assert_se(n_changes == 1); - assert_se(changes[0].type == UNIT_FILE_SYMLINK); + assert_se(changes[0].type_or_errno == UNIT_FILE_SYMLINK); assert_se(streq(changes[0].source, "/usr/lib/systemd/system/a.service")); p = strjoina(root, SYSTEM_CONFIG_UNIT_DIR"/multi-user.target.wants/a.service"); assert_se(streq(changes[0].path, p)); @@ -139,10 +139,10 @@ static void test_basic_mask_and_enable(const char *root) { assert_se(unit_file_reenable(UNIT_FILE_SYSTEM, 0, root, STRV_MAKE("b.service"), &changes, &n_changes) >= 0); assert_se(n_changes == 2); - assert_se(changes[0].type == UNIT_FILE_UNLINK); + assert_se(changes[0].type_or_errno == UNIT_FILE_UNLINK); p = strjoina(root, SYSTEM_CONFIG_UNIT_DIR"/multi-user.target.wants/a.service"); assert_se(streq(changes[0].path, p)); - assert_se(changes[1].type == UNIT_FILE_SYMLINK); + assert_se(changes[1].type_or_errno == UNIT_FILE_SYMLINK); assert_se(streq(changes[1].source, "/usr/lib/systemd/system/a.service")); assert_se(streq(changes[1].path, p)); unit_file_changes_free(changes, n_changes); @@ -224,7 +224,7 @@ static void test_linked_units(const char *root) { /* First, let's link the unit into the search path */ assert_se(unit_file_link(UNIT_FILE_SYSTEM, 0, root, STRV_MAKE("/opt/linked.service"), &changes, &n_changes) >= 0); assert_se(n_changes == 1); - assert_se(changes[0].type == UNIT_FILE_SYMLINK); + assert_se(changes[0].type_or_errno == UNIT_FILE_SYMLINK); assert_se(streq(changes[0].source, "/opt/linked.service")); p = strjoina(root, SYSTEM_CONFIG_UNIT_DIR"/linked.service"); assert_se(streq(changes[0].path, p)); @@ -236,7 +236,7 @@ static void test_linked_units(const char *root) { /* Let's unlink it from the search path again */ assert_se(unit_file_disable(UNIT_FILE_SYSTEM, 0, root, STRV_MAKE("linked.service"), &changes, &n_changes) >= 0); assert_se(n_changes == 1); - assert_se(changes[0].type == UNIT_FILE_UNLINK); + assert_se(changes[0].type_or_errno == UNIT_FILE_UNLINK); p = strjoina(root, SYSTEM_CONFIG_UNIT_DIR"/linked.service"); assert_se(streq(changes[0].path, p)); unit_file_changes_free(changes, n_changes); @@ -250,7 +250,7 @@ static void test_linked_units(const char *root) { p = strjoina(root, SYSTEM_CONFIG_UNIT_DIR"/multi-user.target.wants/linked.service"); q = strjoina(root, SYSTEM_CONFIG_UNIT_DIR"/linked.service"); for (i = 0 ; i < n_changes; i++) { - assert_se(changes[i].type == UNIT_FILE_SYMLINK); + assert_se(changes[i].type_or_errno == UNIT_FILE_SYMLINK); assert_se(streq(changes[i].source, "/opt/linked.service")); if (p && streq(changes[i].path, p)) @@ -272,7 +272,7 @@ static void test_linked_units(const char *root) { p = strjoina(root, SYSTEM_CONFIG_UNIT_DIR"/multi-user.target.wants/linked.service"); q = strjoina(root, SYSTEM_CONFIG_UNIT_DIR"/linked.service"); for (i = 0; i < n_changes; i++) { - assert_se(changes[i].type == UNIT_FILE_UNLINK); + assert_se(changes[i].type_or_errno == UNIT_FILE_UNLINK); if (p && streq(changes[i].path, p)) p = NULL; @@ -292,7 +292,7 @@ static void test_linked_units(const char *root) { p = strjoina(root, SYSTEM_CONFIG_UNIT_DIR"/multi-user.target.wants/linked2.service"); q = strjoina(root, SYSTEM_CONFIG_UNIT_DIR"/linked2.service"); for (i = 0 ; i < n_changes; i++) { - assert_se(changes[i].type == UNIT_FILE_SYMLINK); + assert_se(changes[i].type_or_errno == UNIT_FILE_SYMLINK); assert_se(streq(changes[i].source, "/opt/linked2.service")); if (p && streq(changes[i].path, p)) @@ -308,7 +308,7 @@ static void test_linked_units(const char *root) { assert_se(unit_file_enable(UNIT_FILE_SYSTEM, 0, root, STRV_MAKE("linked3.service"), &changes, &n_changes) >= 0); assert_se(n_changes == 1); - assert_se(changes[0].type == UNIT_FILE_SYMLINK); + assert_se(changes[0].type_or_errno == UNIT_FILE_SYMLINK); assert_se(startswith(changes[0].path, root)); assert_se(endswith(changes[0].path, "linked3.service")); assert_se(streq(changes[0].source, "/opt/linked3.service")); @@ -332,7 +332,7 @@ static void test_default(const char *root) { assert_se(unit_file_set_default(UNIT_FILE_SYSTEM, 0, root, "idontexist.target", &changes, &n_changes) == -ENOENT); assert_se(n_changes == 1); - assert_se(changes[0].type == -ENOENT); + assert_se(changes[0].type_or_errno == -ENOENT); assert_se(streq_ptr(changes[0].path, "idontexist.target")); unit_file_changes_free(changes, n_changes); changes = NULL; n_changes = 0; @@ -341,7 +341,7 @@ static void test_default(const char *root) { assert_se(unit_file_set_default(UNIT_FILE_SYSTEM, 0, root, "test-default.target", &changes, &n_changes) >= 0); assert_se(n_changes == 1); - assert_se(changes[0].type == UNIT_FILE_SYMLINK); + assert_se(changes[0].type_or_errno == UNIT_FILE_SYMLINK); assert_se(streq(changes[0].source, "/usr/lib/systemd/system/test-default-real.target")); p = strjoina(root, SYSTEM_CONFIG_UNIT_DIR "/" SPECIAL_DEFAULT_TARGET); assert_se(streq(changes[0].path, p)); @@ -371,7 +371,7 @@ static void test_add_dependency(const char *root) { assert_se(unit_file_add_dependency(UNIT_FILE_SYSTEM, 0, root, STRV_MAKE("add-dependency-test-service.service"), "add-dependency-test-target.target", UNIT_WANTS, &changes, &n_changes) >= 0); assert_se(n_changes == 1); - assert_se(changes[0].type == UNIT_FILE_SYMLINK); + assert_se(changes[0].type_or_errno == UNIT_FILE_SYMLINK); assert_se(streq(changes[0].source, "/usr/lib/systemd/system/real-add-dependency-test-service.service")); p = strjoina(root, SYSTEM_CONFIG_UNIT_DIR"/real-add-dependency-test-target.target.wants/real-add-dependency-test-service.service"); assert_se(streq(changes[0].path, p)); @@ -412,7 +412,7 @@ static void test_template_enable(const char *root) { assert_se(unit_file_enable(UNIT_FILE_SYSTEM, 0, root, STRV_MAKE("template@.service"), &changes, &n_changes) >= 0); assert_se(n_changes == 1); - assert_se(changes[0].type == UNIT_FILE_SYMLINK); + assert_se(changes[0].type_or_errno == UNIT_FILE_SYMLINK); assert_se(streq(changes[0].source, "/usr/lib/systemd/system/template@.service")); p = strjoina(root, SYSTEM_CONFIG_UNIT_DIR"/multi-user.target.wants/template@def.service"); assert_se(streq(changes[0].path, p)); @@ -428,7 +428,7 @@ static void test_template_enable(const char *root) { assert_se(unit_file_disable(UNIT_FILE_SYSTEM, 0, root, STRV_MAKE("template@.service"), &changes, &n_changes) >= 0); assert_se(n_changes == 1); - assert_se(changes[0].type == UNIT_FILE_UNLINK); + assert_se(changes[0].type_or_errno == UNIT_FILE_UNLINK); assert_se(streq(changes[0].path, p)); unit_file_changes_free(changes, n_changes); changes = NULL; n_changes = 0; @@ -443,7 +443,7 @@ static void test_template_enable(const char *root) { log_info("== %s with template@foo.service enabled ==", __func__); assert_se(unit_file_enable(UNIT_FILE_SYSTEM, 0, root, STRV_MAKE("template@foo.service"), &changes, &n_changes) >= 0); - assert_se(changes[0].type == UNIT_FILE_SYMLINK); + assert_se(changes[0].type_or_errno == UNIT_FILE_SYMLINK); assert_se(streq(changes[0].source, "/usr/lib/systemd/system/template@.service")); p = strjoina(root, SYSTEM_CONFIG_UNIT_DIR"/multi-user.target.wants/template@foo.service"); assert_se(streq(changes[0].path, p)); @@ -459,7 +459,7 @@ static void test_template_enable(const char *root) { assert_se(unit_file_disable(UNIT_FILE_SYSTEM, 0, root, STRV_MAKE("template@foo.service"), &changes, &n_changes) >= 0); assert_se(n_changes == 1); - assert_se(changes[0].type == UNIT_FILE_UNLINK); + assert_se(changes[0].type_or_errno == UNIT_FILE_UNLINK); assert_se(streq(changes[0].path, p)); unit_file_changes_free(changes, n_changes); changes = NULL; n_changes = 0; @@ -476,7 +476,7 @@ static void test_template_enable(const char *root) { log_info("== %s with template-symlink@quux.service enabled ==", __func__); assert_se(unit_file_enable(UNIT_FILE_SYSTEM, 0, root, STRV_MAKE("template-symlink@quux.service"), &changes, &n_changes) >= 0); - assert_se(changes[0].type == UNIT_FILE_SYMLINK); + assert_se(changes[0].type_or_errno == UNIT_FILE_SYMLINK); assert_se(streq(changes[0].source, "/usr/lib/systemd/system/template@.service")); p = strjoina(root, SYSTEM_CONFIG_UNIT_DIR"/multi-user.target.wants/template@quux.service"); assert_se(streq(changes[0].path, p)); @@ -522,7 +522,7 @@ static void test_indirect(const char *root) { assert_se(unit_file_enable(UNIT_FILE_SYSTEM, 0, root, STRV_MAKE("indirectc.service"), &changes, &n_changes) >= 0); assert_se(n_changes == 1); - assert_se(changes[0].type == UNIT_FILE_SYMLINK); + assert_se(changes[0].type_or_errno == UNIT_FILE_SYMLINK); assert_se(streq(changes[0].source, "/usr/lib/systemd/system/indirectb.service")); p = strjoina(root, SYSTEM_CONFIG_UNIT_DIR"/multi-user.target.wants/indirectb.service"); assert_se(streq(changes[0].path, p)); @@ -535,7 +535,7 @@ static void test_indirect(const char *root) { assert_se(unit_file_disable(UNIT_FILE_SYSTEM, 0, root, STRV_MAKE("indirectc.service"), &changes, &n_changes) >= 0); assert_se(n_changes == 1); - assert_se(changes[0].type == UNIT_FILE_UNLINK); + assert_se(changes[0].type_or_errno == UNIT_FILE_UNLINK); p = strjoina(root, SYSTEM_CONFIG_UNIT_DIR"/multi-user.target.wants/indirectb.service"); assert_se(streq(changes[0].path, p)); unit_file_changes_free(changes, n_changes); @@ -574,7 +574,7 @@ static void test_preset_and_list(const char *root) { assert_se(unit_file_preset(UNIT_FILE_SYSTEM, 0, root, STRV_MAKE("preset-yes.service"), UNIT_FILE_PRESET_FULL, &changes, &n_changes) >= 0); assert_se(n_changes == 1); - assert_se(changes[0].type == UNIT_FILE_SYMLINK); + assert_se(changes[0].type_or_errno == UNIT_FILE_SYMLINK); assert_se(streq(changes[0].source, "/usr/lib/systemd/system/preset-yes.service")); p = strjoina(root, SYSTEM_CONFIG_UNIT_DIR"/multi-user.target.wants/preset-yes.service"); assert_se(streq(changes[0].path, p)); @@ -586,7 +586,7 @@ static void test_preset_and_list(const char *root) { assert_se(unit_file_disable(UNIT_FILE_SYSTEM, 0, root, STRV_MAKE("preset-yes.service"), &changes, &n_changes) >= 0); assert_se(n_changes == 1); - assert_se(changes[0].type == UNIT_FILE_UNLINK); + assert_se(changes[0].type_or_errno == UNIT_FILE_UNLINK); p = strjoina(root, SYSTEM_CONFIG_UNIT_DIR"/multi-user.target.wants/preset-yes.service"); assert_se(streq(changes[0].path, p)); unit_file_changes_free(changes, n_changes); @@ -611,11 +611,11 @@ static void test_preset_and_list(const char *root) { for (i = 0; i < n_changes; i++) { - if (changes[i].type == UNIT_FILE_SYMLINK) { + if (changes[i].type_or_errno == UNIT_FILE_SYMLINK) { assert_se(streq(changes[i].source, "/usr/lib/systemd/system/preset-yes.service")); assert_se(streq(changes[i].path, p)); } else - assert_se(changes[i].type == UNIT_FILE_UNLINK); + assert_se(changes[i].type_or_errno == UNIT_FILE_UNLINK); } unit_file_changes_free(changes, n_changes); @@ -678,7 +678,7 @@ static void test_revert(const char *root) { /* Revert the override file */ assert_se(unit_file_revert(UNIT_FILE_SYSTEM, root, STRV_MAKE("xx.service"), &changes, &n_changes) >= 0); assert_se(n_changes == 1); - assert_se(changes[0].type == UNIT_FILE_UNLINK); + assert_se(changes[0].type_or_errno == UNIT_FILE_UNLINK); assert_se(streq(changes[0].path, p)); unit_file_changes_free(changes, n_changes); changes = NULL; n_changes = 0; @@ -690,11 +690,11 @@ static void test_revert(const char *root) { /* Revert the dropin file */ assert_se(unit_file_revert(UNIT_FILE_SYSTEM, root, STRV_MAKE("xx.service"), &changes, &n_changes) >= 0); assert_se(n_changes == 2); - assert_se(changes[0].type == UNIT_FILE_UNLINK); + assert_se(changes[0].type_or_errno == UNIT_FILE_UNLINK); assert_se(streq(changes[0].path, p)); p = strjoina(root, SYSTEM_CONFIG_UNIT_DIR"/xx.service.d"); - assert_se(changes[1].type == UNIT_FILE_UNLINK); + assert_se(changes[1].type_or_errno == UNIT_FILE_UNLINK); assert_se(streq(changes[1].path, p)); unit_file_changes_free(changes, n_changes); changes = NULL; n_changes = 0; @@ -730,7 +730,7 @@ static void test_preset_order(const char *root) { assert_se(unit_file_preset(UNIT_FILE_SYSTEM, 0, root, STRV_MAKE("prefix-1.service"), UNIT_FILE_PRESET_FULL, &changes, &n_changes) >= 0); assert_se(n_changes == 1); - assert_se(changes[0].type == UNIT_FILE_SYMLINK); + assert_se(changes[0].type_or_errno == UNIT_FILE_SYMLINK); assert_se(streq(changes[0].source, "/usr/lib/systemd/system/prefix-1.service")); p = strjoina(root, SYSTEM_CONFIG_UNIT_DIR"/multi-user.target.wants/prefix-1.service"); assert_se(streq(changes[0].path, p)); @@ -842,8 +842,8 @@ static void test_with_dropin(const char *root) { assert_se(unit_file_enable(UNIT_FILE_SYSTEM, 0, root, STRV_MAKE("with-dropin-1.service"), &changes, &n_changes) == 1); assert_se(n_changes == 2); - assert_se(changes[0].type == UNIT_FILE_SYMLINK); - assert_se(changes[1].type == UNIT_FILE_SYMLINK); + assert_se(changes[0].type_or_errno == UNIT_FILE_SYMLINK); + assert_se(changes[1].type_or_errno == UNIT_FILE_SYMLINK); assert_se(streq(changes[0].source, "/usr/lib/systemd/system/with-dropin-1.service")); assert_se(streq(changes[1].source, "/usr/lib/systemd/system/with-dropin-1.service")); p = strjoina(root, SYSTEM_CONFIG_UNIT_DIR"/multi-user.target.wants/with-dropin-1.service"); @@ -856,8 +856,8 @@ static void test_with_dropin(const char *root) { assert_se(unit_file_enable(UNIT_FILE_SYSTEM, 0, root, STRV_MAKE("with-dropin-2.service"), &changes, &n_changes) == 1); assert_se(unit_file_get_state(UNIT_FILE_SYSTEM, root, "with-dropin-2.service", &state) >= 0 && state == UNIT_FILE_ENABLED); assert_se(n_changes == 2); - assert_se(changes[0].type == UNIT_FILE_SYMLINK); - assert_se(changes[1].type == UNIT_FILE_SYMLINK); + assert_se(changes[0].type_or_errno == UNIT_FILE_SYMLINK); + assert_se(changes[1].type_or_errno == UNIT_FILE_SYMLINK); assert_se(streq(changes[0].source, SYSTEM_CONFIG_UNIT_DIR"/with-dropin-2.service")); assert_se(streq(changes[1].source, SYSTEM_CONFIG_UNIT_DIR"/with-dropin-2.service")); p = strjoina(root, SYSTEM_CONFIG_UNIT_DIR"/multi-user.target.wants/with-dropin-2.service"); @@ -870,8 +870,8 @@ static void test_with_dropin(const char *root) { assert_se(unit_file_enable(UNIT_FILE_SYSTEM, 0, root, STRV_MAKE("with-dropin-3.service"), &changes, &n_changes) == 1); assert_se(unit_file_get_state(UNIT_FILE_SYSTEM, root, "with-dropin-3.service", &state) >= 0 && state == UNIT_FILE_ENABLED); assert_se(n_changes == 2); - assert_se(changes[0].type == UNIT_FILE_SYMLINK); - assert_se(changes[1].type == UNIT_FILE_SYMLINK); + assert_se(changes[0].type_or_errno == UNIT_FILE_SYMLINK); + assert_se(changes[1].type_or_errno == UNIT_FILE_SYMLINK); assert_se(streq(changes[0].source, "/usr/lib/systemd/system/with-dropin-3.service")); assert_se(streq(changes[1].source, "/usr/lib/systemd/system/with-dropin-3.service")); p = strjoina(root, SYSTEM_CONFIG_UNIT_DIR"/multi-user.target.wants/with-dropin-3.service"); @@ -884,8 +884,8 @@ static void test_with_dropin(const char *root) { assert_se(unit_file_enable(UNIT_FILE_SYSTEM, 0, root, STRV_MAKE("with-dropin-4a.service"), &changes, &n_changes) == 2); assert_se(unit_file_get_state(UNIT_FILE_SYSTEM, root, "with-dropin-3.service", &state) >= 0 && state == UNIT_FILE_ENABLED); assert_se(n_changes == 2); - assert_se(changes[0].type == UNIT_FILE_SYMLINK); - assert_se(changes[1].type == UNIT_FILE_SYMLINK); + assert_se(changes[0].type_or_errno == UNIT_FILE_SYMLINK); + assert_se(changes[1].type_or_errno == UNIT_FILE_SYMLINK); assert_se(streq(changes[0].source, "/usr/lib/systemd/system/with-dropin-4a.service")); assert_se(streq(changes[1].source, "/usr/lib/systemd/system/with-dropin-4b.service")); p = strjoina(root, SYSTEM_CONFIG_UNIT_DIR"/multi-user.target.wants/with-dropin-4a.service"); @@ -954,8 +954,8 @@ static void test_with_dropin_template(const char *root) { assert_se(unit_file_enable(UNIT_FILE_SYSTEM, 0, root, STRV_MAKE("with-dropin-1@instance-1.service"), &changes, &n_changes) == 1); assert_se(n_changes == 2); - assert_se(changes[0].type == UNIT_FILE_SYMLINK); - assert_se(changes[1].type == UNIT_FILE_SYMLINK); + assert_se(changes[0].type_or_errno == UNIT_FILE_SYMLINK); + assert_se(changes[1].type_or_errno == UNIT_FILE_SYMLINK); assert_se(streq(changes[0].source, "/usr/lib/systemd/system/with-dropin-1@.service")); assert_se(streq(changes[1].source, "/usr/lib/systemd/system/with-dropin-1@.service")); p = strjoina(root, SYSTEM_CONFIG_UNIT_DIR"/multi-user.target.wants/with-dropin-1@instance-1.service"); @@ -967,8 +967,8 @@ static void test_with_dropin_template(const char *root) { assert_se(unit_file_enable(UNIT_FILE_SYSTEM, 0, root, STRV_MAKE("with-dropin-2@instance-1.service"), &changes, &n_changes) == 1); assert_se(n_changes == 2); - assert_se(changes[0].type == UNIT_FILE_SYMLINK); - assert_se(changes[1].type == UNIT_FILE_SYMLINK); + assert_se(changes[0].type_or_errno == UNIT_FILE_SYMLINK); + assert_se(changes[1].type_or_errno == UNIT_FILE_SYMLINK); assert_se(streq(changes[0].source, "/usr/lib/systemd/system/with-dropin-2@.service")); assert_se(streq(changes[1].source, "/usr/lib/systemd/system/with-dropin-2@.service")); p = strjoina(root, SYSTEM_CONFIG_UNIT_DIR"/multi-user.target.wants/with-dropin-2@instance-1.service"); @@ -980,7 +980,7 @@ static void test_with_dropin_template(const char *root) { assert_se(unit_file_enable(UNIT_FILE_SYSTEM, 0, root, STRV_MAKE("with-dropin-2@instance-2.service"), &changes, &n_changes) == 1); assert_se(n_changes == 1); - assert_se(changes[0].type == UNIT_FILE_SYMLINK); + assert_se(changes[0].type_or_errno == UNIT_FILE_SYMLINK); assert_se(streq(changes[0].source, "/usr/lib/systemd/system/with-dropin-2@.service")); p = strjoina(root, SYSTEM_CONFIG_UNIT_DIR"/multi-user.target.wants/with-dropin-2@instance-2.service"); assert_se(streq(changes[0].path, p)); @@ -989,7 +989,7 @@ static void test_with_dropin_template(const char *root) { assert_se(unit_file_enable(UNIT_FILE_SYSTEM, 0, root, STRV_MAKE("with-dropin-3@.service"), &changes, &n_changes) == 1); assert_se(n_changes == 1); - assert_se(changes[0].type == UNIT_FILE_SYMLINK); + assert_se(changes[0].type_or_errno == UNIT_FILE_SYMLINK); assert_se(streq(changes[0].source, "/usr/lib/systemd/system/with-dropin-3@.service")); p = strjoina(root, SYSTEM_CONFIG_UNIT_DIR"/multi-user.target.wants/with-dropin-3@instance-2.service"); assert_se(streq(changes[0].path, p)); @@ -1030,7 +1030,7 @@ static void test_preset_multiple_instances(const char *root) { assert_se(unit_file_preset(UNIT_FILE_SYSTEM, 0, root, STRV_MAKE("foo@bar0.service"), UNIT_FILE_PRESET_FULL, &changes, &n_changes) >= 0); assert_se(unit_file_get_state(UNIT_FILE_SYSTEM, root, "foo@bar0.service", &state) >= 0 && state == UNIT_FILE_ENABLED); assert_se(n_changes == 1); - assert_se(changes[0].type == UNIT_FILE_SYMLINK); + assert_se(changes[0].type_or_errno == UNIT_FILE_SYMLINK); p = strjoina(root, SYSTEM_CONFIG_UNIT_DIR"/multi-user.target.wants/foo@bar0.service"); assert_se(streq(changes[0].path, p)); unit_file_changes_free(changes, n_changes); @@ -1038,7 +1038,7 @@ static void test_preset_multiple_instances(const char *root) { assert_se(unit_file_disable(UNIT_FILE_SYSTEM, 0, root, STRV_MAKE("foo@bar0.service"), &changes, &n_changes) >= 0); assert_se(n_changes == 1); - assert_se(changes[0].type == UNIT_FILE_UNLINK); + assert_se(changes[0].type_or_errno == UNIT_FILE_UNLINK); p = strjoina(root, SYSTEM_CONFIG_UNIT_DIR"/multi-user.target.wants/foo@bar0.service"); assert_se(streq(changes[0].path, p)); unit_file_changes_free(changes, n_changes); diff --git a/src/test/test-install.c b/src/test/test-install.c index 7cd91efcb7f..7a0beb2d248 100644 --- a/src/test/test-install.c +++ b/src/test/test-install.c @@ -12,9 +12,9 @@ static void dump_changes(UnitFileChange *c, unsigned n) { assert_se(n == 0 || c); for (i = 0; i < n; i++) { - if (c[i].type == UNIT_FILE_UNLINK) + if (c[i].type_or_errno == UNIT_FILE_UNLINK) printf("rm '%s'\n", c[i].path); - else if (c[i].type == UNIT_FILE_SYMLINK) + else if (c[i].type_or_errno == UNIT_FILE_SYMLINK) printf("ln -s '%s' '%s'\n", c[i].source, c[i].path); } } From ba5b6c5925aa05b861b757a66b09d3cc51c8dde7 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 17 Feb 2021 10:47:30 +0100 Subject: [PATCH 2/2] portable: make PortableChangeType enum anonymous Same reasons as previous commit. --- src/portable/portable.c | 16 +++++++++++----- src/portable/portable.h | 12 +++++++----- src/portable/portabled-bus.c | 5 ++++- src/portable/portabled-image-bus.c | 2 +- 4 files changed, 23 insertions(+), 12 deletions(-) diff --git a/src/portable/portable.c b/src/portable/portable.c index 0a3b1e8fde6..6c09e8bbd4f 100644 --- a/src/portable/portable.c +++ b/src/portable/portable.c @@ -10,6 +10,7 @@ #include "dirent-util.h" #include "discover-image.h" #include "dissect-image.h" +#include "errno-list.h" #include "fd-util.h" #include "fileio.h" #include "fs-util.h" @@ -591,7 +592,7 @@ static int unit_file_is_active( static int portable_changes_add( PortableChange **changes, size_t *n_changes, - PortableChangeType type, + int type_or_errno, /* PORTABLE_COPY, PORTABLE_SYMLINK, … if positive, or errno if negative */ const char *path, const char *source) { @@ -601,6 +602,11 @@ static int portable_changes_add( assert(path); assert(!changes == !n_changes); + if (type_or_errno >= 0) + assert(type_or_errno < _PORTABLE_CHANGE_TYPE_MAX); + else + assert(type_or_errno >= -ERRNO_MAX); + if (!changes) return 0; @@ -624,7 +630,7 @@ static int portable_changes_add( } c[(*n_changes)++] = (PortableChange) { - .type = type, + .type_or_errno = type_or_errno, .path = TAKE_PTR(p), .source = TAKE_PTR(s), }; @@ -635,7 +641,7 @@ static int portable_changes_add( static int portable_changes_add_with_prefix( PortableChange **changes, size_t *n_changes, - PortableChangeType type, + int type_or_errno, const char *prefix, const char *path, const char *source) { @@ -653,7 +659,7 @@ static int portable_changes_add_with_prefix( source = prefix_roota(prefix, source); } - return portable_changes_add(changes, n_changes, type, path, source); + return portable_changes_add(changes, n_changes, type_or_errno, path, source); } void portable_changes_free(PortableChange *changes, size_t n_changes) { @@ -1417,7 +1423,7 @@ static const char* const portable_change_type_table[_PORTABLE_CHANGE_TYPE_MAX] = [PORTABLE_WRITE] = "write", }; -DEFINE_STRING_TABLE_LOOKUP(portable_change_type, PortableChangeType); +DEFINE_STRING_TABLE_LOOKUP(portable_change_type, int); static const char* const portable_state_table[_PORTABLE_STATE_MAX] = { [PORTABLE_DETACHED] = "detached", diff --git a/src/portable/portable.h b/src/portable/portable.h index 291d9377ef0..5694bd2b623 100644 --- a/src/portable/portable.h +++ b/src/portable/portable.h @@ -24,7 +24,9 @@ typedef enum PortableFlags { PORTABLE_REATTACH = 1 << 3, } PortableFlags; -typedef enum PortableChangeType { +/* This enum is anonymous, since we usually store it in an 'int', as we overload it with negative errno + * values. */ +enum { PORTABLE_COPY, PORTABLE_SYMLINK, PORTABLE_UNLINK, @@ -32,7 +34,7 @@ typedef enum PortableChangeType { PORTABLE_MKDIR, _PORTABLE_CHANGE_TYPE_MAX, _PORTABLE_CHANGE_TYPE_INVALID = -EINVAL, -} PortableChangeType; +}; typedef enum PortableState { PORTABLE_DETACHED, @@ -47,7 +49,7 @@ typedef enum PortableState { } PortableState; typedef struct PortableChange { - int type; /* PortableFileChangeType or negative error number */ + int type_or_errno; /* PORTABLE_COPY, PORTABLE_SYMLINK, … if positive, errno if negative */ char *path; char *source; } PortableChange; @@ -68,8 +70,8 @@ int portable_get_profiles(char ***ret); void portable_changes_free(PortableChange *changes, size_t n_changes); -const char *portable_change_type_to_string(PortableChangeType t) _const_; -PortableChangeType portable_change_type_from_string(const char *t) _pure_; +const char *portable_change_type_to_string(int t) _const_; +int portable_change_type_from_string(const char *t) _pure_; const char *portable_state_to_string(PortableState t) _const_; PortableState portable_state_from_string(const char *t) _pure_; diff --git a/src/portable/portabled-bus.c b/src/portable/portabled-bus.c index fa143b71c0f..8de8bfc247c 100644 --- a/src/portable/portabled-bus.c +++ b/src/portable/portabled-bus.c @@ -457,8 +457,11 @@ static int reply_portable_compose_message(sd_bus_message *reply, const PortableC return r; for (i = 0; i < n_changes; i++) { + if (changes[i].type_or_errno < 0) + continue; + r = sd_bus_message_append(reply, "(sss)", - portable_change_type_to_string(changes[i].type), + portable_change_type_to_string(changes[i].type_or_errno), changes[i].path, changes[i].source); if (r < 0) diff --git a/src/portable/portabled-image-bus.c b/src/portable/portabled-image-bus.c index 75bb4d80c16..018cc9b818a 100644 --- a/src/portable/portabled-image-bus.c +++ b/src/portable/portabled-image-bus.c @@ -483,7 +483,7 @@ static int normalize_portable_changes( } changes[n_changes++] = (PortableChange) { - .type = changes_detached[i].type, + .type_or_errno = changes_detached[i].type_or_errno, .path = TAKE_PTR(path), .source = TAKE_PTR(source), };