From 7dfc71399d2286b37cd61975da196acfd29aca83 Mon Sep 17 00:00:00 2001 From: Mike Yuan Date: Sat, 2 Mar 2024 16:38:17 +0800 Subject: [PATCH 1/4] path-lookup: rename lookup_paths_free -> _done This is stack-allocated, so update to match our usual rules. --- src/analyze/analyze-unit-files.c | 2 +- src/analyze/analyze-unit-paths.c | 2 +- src/basic/path-lookup.c | 5 ++--- src/basic/path-lookup.h | 2 +- src/core/manager.c | 4 ++-- src/libsystemd/sd-path/sd-path.c | 2 +- src/portable/portable.c | 8 ++++---- src/shared/install.c | 28 +++++++++++++-------------- src/ssh-generator/ssh-generator.c | 2 +- src/systemctl/systemctl-edit.c | 4 ++-- src/systemctl/systemctl-enable.c | 2 +- src/systemctl/systemctl-sysv-compat.c | 2 +- src/sysv-generator/sysv-generator.c | 2 +- src/test/test-path-lookup.c | 6 +++--- src/test/test-unit-file.c | 2 +- 15 files changed, 36 insertions(+), 37 deletions(-) diff --git a/src/analyze/analyze-unit-files.c b/src/analyze/analyze-unit-files.c index d9b3313be4..e0c4867c78 100644 --- a/src/analyze/analyze-unit-files.c +++ b/src/analyze/analyze-unit-files.c @@ -15,7 +15,7 @@ static bool strv_fnmatch_strv_or_empty(char* const* patterns, char **strv, int f int verb_unit_files(int argc, char *argv[], void *userdata) { _cleanup_hashmap_free_ Hashmap *unit_ids = NULL, *unit_names = NULL; - _cleanup_(lookup_paths_free) LookupPaths lp = {}; + _cleanup_(lookup_paths_done) LookupPaths lp = {}; char **patterns = strv_skip(argv, 1); const char *k, *dst; char **v; diff --git a/src/analyze/analyze-unit-paths.c b/src/analyze/analyze-unit-paths.c index bb00a4fcd2..17f18e0ab6 100644 --- a/src/analyze/analyze-unit-paths.c +++ b/src/analyze/analyze-unit-paths.c @@ -6,7 +6,7 @@ #include "strv.h" int verb_unit_paths(int argc, char *argv[], void *userdata) { - _cleanup_(lookup_paths_free) LookupPaths paths = {}; + _cleanup_(lookup_paths_done) LookupPaths paths = {}; int r; r = lookup_paths_init_or_warn(&paths, arg_runtime_scope, 0, NULL); diff --git a/src/basic/path-lookup.c b/src/basic/path-lookup.c index 1b31a4e23a..fbcb1edadc 100644 --- a/src/basic/path-lookup.c +++ b/src/basic/path-lookup.c @@ -776,9 +776,8 @@ int lookup_paths_init_or_warn(LookupPaths *lp, RuntimeScope scope, LookupPathsFl return r; } -void lookup_paths_free(LookupPaths *lp) { - if (!lp) - return; +void lookup_paths_done(LookupPaths *lp) { + assert(lp); lp->search_path = strv_free(lp->search_path); diff --git a/src/basic/path-lookup.h b/src/basic/path-lookup.h index 11db264c01..0db2c5a98c 100644 --- a/src/basic/path-lookup.h +++ b/src/basic/path-lookup.h @@ -65,7 +65,7 @@ bool path_is_user_data_dir(const char *path); bool path_is_user_config_dir(const char *path); void lookup_paths_log(LookupPaths *p); -void lookup_paths_free(LookupPaths *p); +void lookup_paths_done(LookupPaths *p); char **generator_binary_paths(RuntimeScope scope); char **env_generator_binary_paths(RuntimeScope scope); diff --git a/src/core/manager.c b/src/core/manager.c index 7a7669f315..d307c07a62 100644 --- a/src/core/manager.c +++ b/src/core/manager.c @@ -1668,7 +1668,7 @@ Manager* manager_free(Manager *m) { free(m->notify_socket); - lookup_paths_free(&m->lookup_paths); + lookup_paths_done(&m->lookup_paths); strv_free(m->transient_environment); strv_free(m->client_environment); @@ -3557,7 +3557,7 @@ int manager_reload(Manager *m) { manager_clear_jobs_and_units(m); lookup_paths_flush_generator(&m->lookup_paths); - lookup_paths_free(&m->lookup_paths); + lookup_paths_done(&m->lookup_paths); exec_shared_runtime_vacuum(m); dynamic_user_vacuum(m, false); m->uid_refs = hashmap_free(m->uid_refs); diff --git a/src/libsystemd/sd-path/sd-path.c b/src/libsystemd/sd-path/sd-path.c index 7290d1c40a..85bca9fd3b 100644 --- a/src/libsystemd/sd-path/sd-path.c +++ b/src/libsystemd/sd-path/sd-path.c @@ -596,7 +596,7 @@ static int get_search(uint64_t type, char ***list) { case SD_PATH_SYSTEMD_SEARCH_SYSTEM_UNIT: case SD_PATH_SYSTEMD_SEARCH_USER_UNIT: { - _cleanup_(lookup_paths_free) LookupPaths lp = {}; + _cleanup_(lookup_paths_done) LookupPaths lp = {}; RuntimeScope scope = type == SD_PATH_SYSTEMD_SEARCH_SYSTEM_UNIT ? RUNTIME_SCOPE_SYSTEM : RUNTIME_SCOPE_USER; diff --git a/src/portable/portable.c b/src/portable/portable.c index 2012dda835..a003ea0352 100644 --- a/src/portable/portable.c +++ b/src/portable/portable.c @@ -182,7 +182,7 @@ static int extract_now( _cleanup_hashmap_free_ Hashmap *unit_files = NULL; _cleanup_(portable_metadata_unrefp) PortableMetadata *os_release = NULL; - _cleanup_(lookup_paths_free) LookupPaths paths = {}; + _cleanup_(lookup_paths_done) LookupPaths paths = {}; _cleanup_close_ int os_release_fd = -EBADF; _cleanup_free_ char *os_release_path = NULL; const char *os_release_id; @@ -1556,7 +1556,7 @@ int portable_attach( _cleanup_ordered_hashmap_free_ OrderedHashmap *extension_images = NULL, *extension_releases = NULL; _cleanup_(portable_metadata_unrefp) PortableMetadata *os_release = NULL; _cleanup_hashmap_free_ Hashmap *unit_files = NULL; - _cleanup_(lookup_paths_free) LookupPaths paths = {}; + _cleanup_(lookup_paths_done) LookupPaths paths = {}; _cleanup_strv_free_ char **valid_prefixes = NULL; _cleanup_(image_unrefp) Image *image = NULL; PortableMetadata *item; @@ -1817,7 +1817,7 @@ int portable_detach( size_t *n_changes, sd_bus_error *error) { - _cleanup_(lookup_paths_free) LookupPaths paths = {}; + _cleanup_(lookup_paths_done) LookupPaths paths = {}; _cleanup_set_free_ Set *unit_files = NULL, *markers = NULL; _cleanup_free_ char *extensions = NULL; _cleanup_closedir_ DIR *d = NULL; @@ -2002,7 +2002,7 @@ static int portable_get_state_internal( PortableState *ret, sd_bus_error *error) { - _cleanup_(lookup_paths_free) LookupPaths paths = {}; + _cleanup_(lookup_paths_done) LookupPaths paths = {}; bool found_enabled = false, found_running = false; _cleanup_set_free_ Set *unit_files = NULL; _cleanup_closedir_ DIR *d = NULL; diff --git a/src/shared/install.c b/src/shared/install.c index c3a94d1912..32f504c327 100644 --- a/src/shared/install.c +++ b/src/shared/install.c @@ -2259,7 +2259,7 @@ int unit_file_mask( InstallChange **changes, size_t *n_changes) { - _cleanup_(lookup_paths_free) LookupPaths lp = {}; + _cleanup_(lookup_paths_done) LookupPaths lp = {}; const char *config_path; int r; @@ -2302,7 +2302,7 @@ int unit_file_unmask( InstallChange **changes, size_t *n_changes) { - _cleanup_(lookup_paths_free) LookupPaths lp = {}; + _cleanup_(lookup_paths_done) LookupPaths lp = {}; _cleanup_set_free_free_ Set *remove_symlinks_to = NULL; _cleanup_strv_free_ char **todo = NULL; const char *config_path; @@ -2414,7 +2414,7 @@ int unit_file_link( InstallChange **changes, size_t *n_changes) { - _cleanup_(lookup_paths_free) LookupPaths lp = {}; + _cleanup_(lookup_paths_done) LookupPaths lp = {}; _cleanup_strv_free_ char **todo = NULL; const char *config_path; size_t n_todo = 0; @@ -2522,7 +2522,7 @@ int unit_file_revert( size_t *n_changes) { _cleanup_set_free_free_ Set *remove_symlinks_to = NULL; - _cleanup_(lookup_paths_free) LookupPaths lp = {}; + _cleanup_(lookup_paths_done) LookupPaths lp = {}; _cleanup_strv_free_ char **todo = NULL; size_t n_todo = 0; int r, q; @@ -2680,7 +2680,7 @@ int unit_file_add_dependency( InstallChange **changes, size_t *n_changes) { - _cleanup_(lookup_paths_free) LookupPaths lp = {}; + _cleanup_(lookup_paths_done) LookupPaths lp = {}; _cleanup_(install_context_done) InstallContext ctx = { .scope = scope }; InstallInfo *info, *target_info; const char *config_path; @@ -2781,7 +2781,7 @@ int unit_file_enable( InstallChange **changes, size_t *n_changes) { - _cleanup_(lookup_paths_free) LookupPaths lp = {}; + _cleanup_(lookup_paths_done) LookupPaths lp = {}; int r; assert(scope >= 0); @@ -2849,7 +2849,7 @@ int unit_file_disable( InstallChange **changes, size_t *n_changes) { - _cleanup_(lookup_paths_free) LookupPaths lp = {}; + _cleanup_(lookup_paths_done) LookupPaths lp = {}; int r; assert(scope >= 0); @@ -2932,7 +2932,7 @@ int unit_file_reenable( InstallChange **changes, size_t *n_changes) { - _cleanup_(lookup_paths_free) LookupPaths lp = {}; + _cleanup_(lookup_paths_done) LookupPaths lp = {}; _cleanup_strv_free_ char **names = NULL, **files = NULL; int r; @@ -2968,7 +2968,7 @@ int unit_file_set_default( InstallChange **changes, size_t *n_changes) { - _cleanup_(lookup_paths_free) LookupPaths lp = {}; + _cleanup_(lookup_paths_done) LookupPaths lp = {}; _cleanup_(install_context_done) InstallContext ctx = { .scope = scope }; InstallInfo *info; const char *new_path; @@ -3000,7 +3000,7 @@ int unit_file_get_default( const char *root_dir, char **name) { - _cleanup_(lookup_paths_free) LookupPaths lp = {}; + _cleanup_(lookup_paths_done) LookupPaths lp = {}; _cleanup_(install_context_done) InstallContext ctx = { .scope = scope }; InstallInfo *info; char *n; @@ -3131,7 +3131,7 @@ int unit_file_get_state( const char *name, UnitFileState *ret) { - _cleanup_(lookup_paths_free) LookupPaths lp = {}; + _cleanup_(lookup_paths_done) LookupPaths lp = {}; int r; assert(scope >= 0); @@ -3558,7 +3558,7 @@ int unit_file_preset( size_t *n_changes) { _cleanup_(install_context_done) InstallContext plus = {}, minus = {}; - _cleanup_(lookup_paths_free) LookupPaths lp = {}; + _cleanup_(lookup_paths_done) LookupPaths lp = {}; _cleanup_(unit_file_presets_done) UnitFilePresets presets = {}; const char *config_path; int r; @@ -3597,7 +3597,7 @@ int unit_file_preset_all( size_t *n_changes) { _cleanup_(install_context_done) InstallContext plus = {}, minus = {}; - _cleanup_(lookup_paths_free) LookupPaths lp = {}; + _cleanup_(lookup_paths_done) LookupPaths lp = {}; _cleanup_(unit_file_presets_done) UnitFilePresets presets = {}; const char *config_path = NULL; int r; @@ -3674,7 +3674,7 @@ int unit_file_get_list( char **states, char **patterns) { - _cleanup_(lookup_paths_free) LookupPaths lp = {}; + _cleanup_(lookup_paths_done) LookupPaths lp = {}; int r; assert(scope >= 0); diff --git a/src/ssh-generator/ssh-generator.c b/src/ssh-generator/ssh-generator.c index 6fdd4ec27f..08532690a9 100644 --- a/src/ssh-generator/ssh-generator.c +++ b/src/ssh-generator/ssh-generator.c @@ -458,7 +458,7 @@ static int run(const char *dest, const char *dest_early, const char *dest_late) if (r < 0) return log_error_errno(r, "Failed to determine if sshd is installed: %m"); - _cleanup_(lookup_paths_free) LookupPaths lp = {}; + _cleanup_(lookup_paths_done) LookupPaths lp = {}; r = lookup_paths_init_or_warn(&lp, RUNTIME_SCOPE_SYSTEM, LOOKUP_PATHS_EXCLUDE_GENERATED, /* root_dir= */ NULL); if (r < 0) return r; diff --git a/src/systemctl/systemctl-edit.c b/src/systemctl/systemctl-edit.c index c851c8546e..15398f8364 100644 --- a/src/systemctl/systemctl-edit.c +++ b/src/systemctl/systemctl-edit.c @@ -15,7 +15,7 @@ int verb_cat(int argc, char *argv[], void *userdata) { _cleanup_hashmap_free_ Hashmap *cached_id_map = NULL, *cached_name_map = NULL; - _cleanup_(lookup_paths_free) LookupPaths lp = {}; + _cleanup_(lookup_paths_done) LookupPaths lp = {}; _cleanup_strv_free_ char **names = NULL; sd_bus *bus; bool first = true; @@ -198,7 +198,7 @@ static int find_paths_to_edit( char **names) { _cleanup_hashmap_free_ Hashmap *cached_id_map = NULL, *cached_name_map = NULL; - _cleanup_(lookup_paths_free) LookupPaths lp = {}; + _cleanup_(lookup_paths_done) LookupPaths lp = {}; _cleanup_free_ char *drop_in_alloc = NULL, *suffix = NULL; const char *drop_in; int r; diff --git a/src/systemctl/systemctl-enable.c b/src/systemctl/systemctl-enable.c index 24a1b3d583..47a19cdd63 100644 --- a/src/systemctl/systemctl-enable.c +++ b/src/systemctl/systemctl-enable.c @@ -143,7 +143,7 @@ int verb_enable(int argc, char *argv[], void *userdata) { bool warn_trigger_ignore_masked = true; /* suppress "used uninitialized" warning */ if (STR_IN_SET(verb, "mask", "unmask")) { - _cleanup_(lookup_paths_free) LookupPaths lp = {}; + _cleanup_(lookup_paths_done) LookupPaths lp = {}; r = lookup_paths_init_or_warn(&lp, arg_runtime_scope, 0, arg_root); if (r < 0) diff --git a/src/systemctl/systemctl-sysv-compat.c b/src/systemctl/systemctl-sysv-compat.c index 2aa1ec6d83..8ee16eb13f 100644 --- a/src/systemctl/systemctl-sysv-compat.c +++ b/src/systemctl/systemctl-sysv-compat.c @@ -111,7 +111,7 @@ int enable_sysv_units(const char *verb, char **args) { int r = 0; #if HAVE_SYSV_COMPAT - _cleanup_(lookup_paths_free) LookupPaths paths = {}; + _cleanup_(lookup_paths_done) LookupPaths paths = {}; unsigned f = 0; SysVUnitEnableState enable_state = SYSV_UNIT_NOT_FOUND; diff --git a/src/sysv-generator/sysv-generator.c b/src/sysv-generator/sysv-generator.c index 5689a60d78..f3b447095a 100644 --- a/src/sysv-generator/sysv-generator.c +++ b/src/sysv-generator/sysv-generator.c @@ -894,7 +894,7 @@ finish: static int run(const char *dest, const char *dest_early, const char *dest_late) { _cleanup_(free_sysvstub_hashmapp) Hashmap *all_services = NULL; - _cleanup_(lookup_paths_free) LookupPaths lp = {}; + _cleanup_(lookup_paths_done) LookupPaths lp = {}; SysvStub *service; int r; diff --git a/src/test/test-path-lookup.c b/src/test/test-path-lookup.c index 431a85965d..cb13fad7c5 100644 --- a/src/test/test-path-lookup.c +++ b/src/test/test-path-lookup.c @@ -13,8 +13,8 @@ static void test_paths_one(RuntimeScope scope) { _cleanup_(rm_rf_physical_and_freep) char *tmp = NULL; - _cleanup_(lookup_paths_free) LookupPaths lp_without_env = {}; - _cleanup_(lookup_paths_free) LookupPaths lp_with_env = {}; + _cleanup_(lookup_paths_done) LookupPaths lp_without_env = {}; + _cleanup_(lookup_paths_done) LookupPaths lp_with_env = {}; char *systemd_unit_path; assert_se(mkdtemp_malloc("/tmp/test-path-lookup.XXXXXXX", &tmp) >= 0); @@ -40,7 +40,7 @@ TEST(paths) { } TEST(user_and_global_paths) { - _cleanup_(lookup_paths_free) LookupPaths lp_global = {}, lp_user = {}; + _cleanup_(lookup_paths_done) LookupPaths lp_global = {}, lp_user = {}; char **u, **g; unsigned k = 0; diff --git a/src/test/test-unit-file.c b/src/test/test-unit-file.c index 9f8787b492..220802e350 100644 --- a/src/test/test-unit-file.c +++ b/src/test/test-unit-file.c @@ -26,7 +26,7 @@ TEST(unit_validate_alias_symlink_and_warn) { } TEST(unit_file_build_name_map) { - _cleanup_(lookup_paths_free) LookupPaths lp = {}; + _cleanup_(lookup_paths_done) LookupPaths lp = {}; _cleanup_hashmap_free_ Hashmap *unit_ids = NULL; _cleanup_hashmap_free_ Hashmap *unit_names = NULL; const char *k, *dst; From b2751cf0394d36c24590b5f7b33e9f864b57ba0d Mon Sep 17 00:00:00 2001 From: Mike Yuan Date: Thu, 29 Feb 2024 18:53:26 +0800 Subject: [PATCH 2/4] shared/install: use RET_GATHER more --- src/shared/install.c | 54 ++++++++++++++++++-------------------------- 1 file changed, 22 insertions(+), 32 deletions(-) diff --git a/src/shared/install.c b/src/shared/install.c index 32f504c327..42b725c076 100644 --- a/src/shared/install.c +++ b/src/shared/install.c @@ -1892,7 +1892,7 @@ static int install_info_symlink_alias( InstallChange **changes, size_t *n_changes) { - int r = 0, q; + int r, ret = 0; assert(info); assert(lp); @@ -1900,20 +1900,17 @@ static int install_info_symlink_alias( STRV_FOREACH(s, info->aliases) { _cleanup_free_ char *alias_path = NULL, *dst = NULL, *dst_updated = NULL; - bool broken; - q = install_name_printf(scope, info, *s, &dst); - if (q < 0) { - install_changes_add(changes, n_changes, q, *s, NULL); - r = r < 0 ? r : q; + r = install_name_printf(scope, info, *s, &dst); + if (r < 0) { + RET_GATHER(ret, install_changes_add(changes, n_changes, r, *s, NULL)); continue; } - q = unit_file_verify_alias(info, dst, &dst_updated, changes, n_changes); - if (q == -ELOOP) - continue; - if (q < 0) { - r = r < 0 ? r : q; + r = unit_file_verify_alias(info, dst, &dst_updated, changes, n_changes); + if (r < 0) { + if (r != -ELOOP) + RET_GATHER(ret, r); continue; } @@ -1921,18 +1918,18 @@ static int install_info_symlink_alias( if (!alias_path) return -ENOMEM; - q = chase(alias_path, lp->root_dir, CHASE_NONEXISTENT, NULL, NULL); - if (q < 0 && q != -ENOENT) { - r = r < 0 ? r : q; + bool broken; + r = chase(alias_path, lp->root_dir, CHASE_NONEXISTENT, /* ret_path = */ NULL, /* ret_fd = */ NULL); + if (r < 0 && r != -ENOENT) { + RET_GATHER(ret, r); continue; } - broken = q == 0; /* symlink target does not exist? */ + broken = r == 0; /* symlink target does not exist? */ - q = create_symlink(lp, info->path, alias_path, force || broken, changes, n_changes); - r = r < 0 ? r : q; + RET_GATHER(ret, create_symlink(lp, info->path, alias_path, force || broken, changes, n_changes)); } - return r; + return ret; } static int install_info_symlink_wants( @@ -1995,10 +1992,7 @@ static int install_info_symlink_wants( q = install_name_printf(scope, info, *s, &dst); if (q < 0) { - install_changes_add(changes, n_changes, q, *s, NULL); - if (r >= 0) - r = q; - + RET_GATHER(r, install_changes_add(changes, n_changes, q, *s, NULL)); continue; } @@ -2010,15 +2004,13 @@ static int install_info_symlink_wants( * 'systemctl enable serial-getty@.service' should fail, the user should specify an * instance like in 'systemctl enable serial-getty@ttyS0.service'. */ - if (file_flags & UNIT_FILE_IGNORE_AUXILIARY_FAILURE) + if (FLAGS_SET(file_flags, UNIT_FILE_IGNORE_AUXILIARY_FAILURE)) continue; if (unit_name_is_valid(dst, UNIT_NAME_ANY)) - q = install_changes_add(changes, n_changes, -EIDRM, dst, n); + RET_GATHER(r, install_changes_add(changes, n_changes, -EIDRM, dst, n)); else - q = install_changes_add(changes, n_changes, -EUCLEAN, dst, NULL); - if (r >= 0) - r = q; + RET_GATHER(r, install_changes_add(changes, n_changes, -EUCLEAN, dst, NULL)); continue; } @@ -2027,7 +2019,7 @@ static int install_info_symlink_wants( if (!path) return -ENOMEM; - q = create_symlink(lp, info->path, path, true, changes, n_changes); + q = create_symlink(lp, info->path, path, /* force = */ true, changes, n_changes); if ((q < 0 && r >= 0) || r == 0) r = q; @@ -2418,7 +2410,7 @@ int unit_file_link( _cleanup_strv_free_ char **todo = NULL; const char *config_path; size_t n_todo = 0; - int r, q; + int r; assert(scope >= 0); assert(scope < _RUNTIME_SCOPE_MAX); @@ -2487,9 +2479,7 @@ int unit_file_link( if (!new_path) return -ENOMEM; - q = create_symlink(&lp, *i, new_path, flags & UNIT_FILE_FORCE, changes, n_changes); - if (q < 0 && r >= 0) - r = q; + RET_GATHER(r, create_symlink(&lp, *i, new_path, flags & UNIT_FILE_FORCE, changes, n_changes)); } return r; From b412274fd0fb5e90632095f54bc0855810d61e93 Mon Sep 17 00:00:00 2001 From: Mike Yuan Date: Thu, 29 Feb 2024 21:04:02 +0800 Subject: [PATCH 3/4] shared/install: use FOREACH_ARRAY at one more place --- src/shared/install.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/shared/install.c b/src/shared/install.c index 42b725c076..ce8adb2e16 100644 --- a/src/shared/install.c +++ b/src/shared/install.c @@ -324,9 +324,9 @@ InstallChangeType install_changes_add( void install_changes_free(InstallChange *changes, size_t n_changes) { assert(changes || n_changes == 0); - for (size_t i = 0; i < n_changes; i++) { - free(changes[i].path); - free(changes[i].source); + FOREACH_ARRAY(i, changes, n_changes) { + free(i->path); + free(i->source); } free(changes); From d41f08bd2ac4cfa58191b64d31ca9e6f3dec7552 Mon Sep 17 00:00:00 2001 From: Mike Yuan Date: Thu, 29 Feb 2024 20:58:17 +0800 Subject: [PATCH 4/4] core,install: generalize install error handling --- src/core/dbus-manager.c | 98 ++++----------- src/shared/install.c | 262 +++++++++++++++++++++++----------------- src/shared/install.h | 9 +- 3 files changed, 184 insertions(+), 185 deletions(-) diff --git a/src/core/dbus-manager.c b/src/core/dbus-manager.c index 00fd801cb3..fbc5fce128 100644 --- a/src/core/dbus-manager.c +++ b/src/core/dbus-manager.c @@ -2304,85 +2304,36 @@ static int send_unit_files_changed(sd_bus *bus, void *userdata) { return sd_bus_send(bus, message, NULL); } -/* Create an error reply, using the error information from changes[] - * if possible, and fall back to generating an error from error code c. - * The error message only describes the first error. - */ static int install_error( sd_bus_error *error, int c, InstallChange *changes, size_t n_changes) { + int r; + + /* Create an error reply, using the error information from changes[] if possible, and fall back to + * generating an error from error code c. The error message only describes the first error. */ + + assert(changes || n_changes == 0); + CLEANUP_ARRAY(changes, n_changes, install_changes_free); - for (size_t i = 0; i < n_changes; i++) + FOREACH_ARRAY(i, changes, n_changes) { + _cleanup_free_ char *err_message = NULL; + const char *bus_error; - /* When making changes here, make sure to also change install_changes_dump() in install.c. */ + if (i->type >= 0) + continue; - switch (changes[i].type) { - case 0 ... _INSTALL_CHANGE_TYPE_MAX: /* not errors */ - break; + r = install_change_dump_error(i, &err_message, &bus_error); + if (r == -ENOMEM) + return r; + if (r < 0) + return sd_bus_error_set_errnof(error, r, "File %s: %m", i->path); - case -EEXIST: - if (changes[i].source) - return sd_bus_error_setf(error, BUS_ERROR_UNIT_EXISTS, - "File %s already exists and is a symlink to %s.", - changes[i].path, changes[i].source); - return sd_bus_error_setf(error, BUS_ERROR_UNIT_EXISTS, - "File %s already exists.", - changes[i].path); - - case -ERFKILL: - return sd_bus_error_setf(error, BUS_ERROR_UNIT_MASKED, - "Unit file %s is masked.", changes[i].path); - - case -EADDRNOTAVAIL: - return sd_bus_error_setf(error, BUS_ERROR_UNIT_GENERATED, - "Unit %s is transient or generated.", changes[i].path); - - case -ETXTBSY: - return sd_bus_error_setf(error, BUS_ERROR_UNIT_BAD_PATH, - "File %s is under the systemd unit hierarchy already.", changes[i].path); - - case -EBADSLT: - return sd_bus_error_setf(error, BUS_ERROR_BAD_UNIT_SETTING, - "Invalid specifier in %s.", changes[i].path); - - case -EIDRM: - return sd_bus_error_setf(error, BUS_ERROR_BAD_UNIT_SETTING, - "Destination unit %s is a non-template unit.", changes[i].path); - - case -EUCLEAN: - return sd_bus_error_setf(error, BUS_ERROR_BAD_UNIT_SETTING, - "\"%s\" is not a valid unit name.", - changes[i].path); - - case -ELOOP: - return sd_bus_error_setf(error, BUS_ERROR_UNIT_LINKED, - "Refusing to operate on alias name or linked unit file: %s", - changes[i].path); - - case -EXDEV: - if (changes[i].source) - return sd_bus_error_setf(error, BUS_ERROR_BAD_UNIT_SETTING, - "Cannot alias %s as %s.", - changes[i].source, changes[i].path); - return sd_bus_error_setf(error, BUS_ERROR_BAD_UNIT_SETTING, - "Invalid unit reference %s.", changes[i].path); - - case -ENOENT: - return sd_bus_error_setf(error, BUS_ERROR_NO_SUCH_UNIT, - "Unit file %s does not exist.", changes[i].path); - - case -EUNATCH: - return sd_bus_error_setf(error, BUS_ERROR_BAD_UNIT_SETTING, - "Cannot resolve specifiers in %s.", changes[i].path); - - default: - assert(changes[i].type < 0); /* other errors */ - return sd_bus_error_set_errnof(error, changes[i].type, "File %s: %m", changes[i].path); - } + return sd_bus_error_set(error, bus_error, err_message); + } return c < 0 ? c : -EINVAL; } @@ -2421,18 +2372,17 @@ static int reply_install_changes_and_free( if (r < 0) return r; - for (size_t i = 0; i < n_changes; i++) { - - if (changes[i].type < 0) { + FOREACH_ARRAY(i, changes, n_changes) { + if (i->type < 0) { bad = true; continue; } r = sd_bus_message_append( reply, "(sss)", - install_change_type_to_string(changes[i].type), - changes[i].path, - changes[i].source); + install_change_type_to_string(i->type), + i->path, + i->source); if (r < 0) return r; diff --git a/src/shared/install.c b/src/shared/install.c index ce8adb2e16..e42f77aa28 100644 --- a/src/shared/install.c +++ b/src/shared/install.c @@ -10,6 +10,7 @@ #include #include "alloc-util.h" +#include "bus-common-errors.h" #include "chase.h" #include "conf-files.h" #include "conf-parser.h" @@ -332,122 +333,163 @@ void install_changes_free(InstallChange *changes, size_t n_changes) { free(changes); } -void install_changes_dump(int r, const char *verb, const InstallChange *changes, size_t n_changes, bool quiet) { - int err = 0; +static void install_change_dump_success(const InstallChange *change) { + assert(change); + assert(change->path); - assert(changes || n_changes == 0); - /* If verb is not specified, errors are not allowed! */ - assert(verb || r >= 0); + switch (change->type) { - for (size_t i = 0; i < n_changes; i++) { - assert(changes[i].path); - /* This tries to tell the compiler that it's safe to use 'verb' in a string format if there - * was an error, but the compiler doesn't care and fails anyway, so strna(verb) is used - * too. */ - assert(verb || changes[i].type >= 0); - verb = strna(verb); + case INSTALL_CHANGE_SYMLINK: + return log_info("Created symlink '%s' %s '%s'.", + change->path, special_glyph(SPECIAL_GLYPH_ARROW_RIGHT), change->source); - /* When making changes here, make sure to also change install_error() in dbus-manager.c. */ + case INSTALL_CHANGE_UNLINK: + return log_info("Removed '%s'.", change->path); - switch (changes[i].type) { - case INSTALL_CHANGE_SYMLINK: - if (!quiet) - log_info("Created symlink %s %s %s.", - changes[i].path, - special_glyph(SPECIAL_GLYPH_ARROW_RIGHT), - changes[i].source); - break; - case INSTALL_CHANGE_UNLINK: - if (!quiet) - log_info("Removed \"%s\".", changes[i].path); - break; - case INSTALL_CHANGE_IS_MASKED: - if (!quiet) - log_info("Unit %s is masked, ignoring.", changes[i].path); - break; - case INSTALL_CHANGE_IS_MASKED_GENERATOR: - if (!quiet) - log_info("Unit %s is masked via a generator and cannot be unmasked.", - changes[i].path); - break; - case INSTALL_CHANGE_IS_DANGLING: - if (!quiet) - log_info("Unit %s is an alias to a unit that is not present, ignoring.", - changes[i].path); - break; - case INSTALL_CHANGE_DESTINATION_NOT_PRESENT: - if (!quiet) - log_warning("Unit %s is added as a dependency to a non-existent unit %s.", - changes[i].source, changes[i].path); - break; - case INSTALL_CHANGE_AUXILIARY_FAILED: - if (!quiet) - log_warning("Failed to enable auxiliary unit %s, ignoring.", changes[i].path); - break; - case -EEXIST: - if (changes[i].source) - err = log_error_errno(changes[i].type, - "Failed to %s unit, file \"%s\" already exists and is a symlink to \"%s\".", - verb, changes[i].path, changes[i].source); - else - err = log_error_errno(changes[i].type, - "Failed to %s unit, file \"%s\" already exists.", - verb, changes[i].path); - break; - case -ERFKILL: - err = log_error_errno(changes[i].type, "Failed to %s unit, unit %s is masked.", - verb, changes[i].path); - break; - case -EADDRNOTAVAIL: - err = log_error_errno(changes[i].type, "Failed to %s unit, unit %s is transient or generated.", - verb, changes[i].path); - break; - case -ETXTBSY: - err = log_error_errno(changes[i].type, "Failed to %s unit, file %s is under the systemd unit hierarchy already.", - verb, changes[i].path); - break; - case -EBADSLT: - err = log_error_errno(changes[i].type, "Failed to %s unit, invalid specifier in \"%s\".", - verb, changes[i].path); - break; - case -EIDRM: - err = log_error_errno(changes[i].type, "Failed to %s %s, destination unit %s is a non-template unit.", - verb, changes[i].source, changes[i].path); - break; - case -EUCLEAN: - err = log_error_errno(changes[i].type, - "Failed to %s unit, \"%s\" is not a valid unit name.", - verb, changes[i].path); - break; - case -ELOOP: - err = log_error_errno(changes[i].type, "Failed to %s unit, refusing to operate on linked unit file %s.", - verb, changes[i].path); - break; - case -EXDEV: - if (changes[i].source) - err = log_error_errno(changes[i].type, "Failed to %s unit, cannot alias %s as %s.", - verb, changes[i].source, changes[i].path); - else - err = log_error_errno(changes[i].type, "Failed to %s unit, invalid unit reference \"%s\".", - verb, changes[i].path); - break; - case -ENOENT: - err = log_error_errno(changes[i].type, "Failed to %s unit, unit %s does not exist.", - verb, changes[i].path); - break; - case -EUNATCH: - err = log_error_errno(changes[i].type, "Failed to %s unit, cannot resolve specifiers in \"%s\".", - verb, changes[i].path); - break; - default: - assert(changes[i].type < 0); - err = log_error_errno(changes[i].type, "Failed to %s unit, file \"%s\": %m", - verb, changes[i].path); - } + case INSTALL_CHANGE_IS_MASKED: + return log_info("Unit %s is masked, ignoring.", change->path); + + case INSTALL_CHANGE_IS_MASKED_GENERATOR: + return log_info("Unit %s is masked via a generator and cannot be unmasked, skipping.", change->path); + + case INSTALL_CHANGE_IS_DANGLING: + return log_info("Unit %s is an alias to a non-existent unit, ignoring.", change->path); + + case INSTALL_CHANGE_DESTINATION_NOT_PRESENT: + return log_warning("Unit %s is added as a dependency to a non-existent unit %s.", + change->source, change->path); + + case INSTALL_CHANGE_AUXILIARY_FAILED: + return log_warning("Failed to enable auxiliary unit %s, ignoring.", change->path); + + default: + assert_not_reached(); } +} - if (r < 0 && err >= 0) - log_error_errno(r, "Failed to %s: %m.", verb); +int install_change_dump_error(const InstallChange *change, char **ret_errmsg, const char **ret_bus_error) { + char *m; + const char *bus_error; + + /* Returns 0: known error and ret_errmsg formatted + * < 0: non-recognizable error */ + + assert(change); + assert(change->path); + assert(change->type < 0); + assert(ret_errmsg); + + switch (change->type) { + + case -EEXIST: + m = strjoin("File '", change->path, "' already exists", + change->source ? " and is a symlink to " : NULL, change->source); + bus_error = BUS_ERROR_UNIT_EXISTS; + break; + + case -ERFKILL: + m = strjoin("Unit ", change->path, " is masked"); + bus_error = BUS_ERROR_UNIT_MASKED; + break; + + case -EADDRNOTAVAIL: + m = strjoin("Unit ", change->path, " is transient or generated"); + bus_error = BUS_ERROR_UNIT_GENERATED; + break; + + case -ETXTBSY: + m = strjoin("File '", change->path, "' is under the systemd unit hierarchy already"); + bus_error = BUS_ERROR_UNIT_BAD_PATH; + break; + + case -EBADSLT: + m = strjoin("Invalid specifier in unit ", change->path); + bus_error = BUS_ERROR_BAD_UNIT_SETTING; + break; + + case -EIDRM: + m = strjoin("Refusing to operate on template unit ", change->source, + " when destination unit ", change->path, " is a non-template unit"); + bus_error = BUS_ERROR_BAD_UNIT_SETTING; + break; + + case -EUCLEAN: + m = strjoin("Invalid unit name ", change->path); + bus_error = BUS_ERROR_BAD_UNIT_SETTING; + break; + + case -ELOOP: + m = strjoin("Refusing to operate on linked unit file ", change->path); + bus_error = BUS_ERROR_UNIT_LINKED; + break; + + case -EXDEV: + if (change->source) + m = strjoin("Cannot alias ", change->source, " as ", change->path); + else + m = strjoin("Invalid unit reference ", change->path); + bus_error = BUS_ERROR_BAD_UNIT_SETTING; + break; + + case -ENOENT: + m = strjoin("Unit ", change->path, " does not exist"); + bus_error = BUS_ERROR_NO_SUCH_UNIT; + break; + + case -EUNATCH: + m = strjoin("Cannot resolve specifiers in unit ", change->path); + bus_error = BUS_ERROR_BAD_UNIT_SETTING; + break; + + default: + return change->type; + } + if (!m) + return -ENOMEM; + + *ret_errmsg = m; + if (ret_bus_error) + *ret_bus_error = bus_error; + + return 0; +} + +void install_changes_dump( + int error, + const char *verb, + const InstallChange *changes, + size_t n_changes, + bool quiet) { + + bool err_logged = false; + int r; + + /* If verb is not specified, errors are not allowed! */ + assert(verb || error >= 0); + assert(changes || n_changes == 0); + + FOREACH_ARRAY(i, changes, n_changes) + if (i->type >= 0) { + if (!quiet) + install_change_dump_success(i); + } else { + _cleanup_free_ char *err_message = NULL; + + assert(verb); + + r = install_change_dump_error(i, &err_message, /* ret_bus_error = */ NULL); + if (r == -ENOMEM) + return (void) log_oom(); + if (r < 0) + log_error_errno(r, "Failed to %s unit %s: %m", verb, i->path); + else + log_error_errno(i->type, "Failed to %s unit: %s", verb, err_message); + + err_logged = true; + } + + if (error < 0 && !err_logged) + log_error_errno(error, "Failed to %s unit: %m.", verb); } /** diff --git a/src/shared/install.h b/src/shared/install.h index 3e2ada45f4..a09557f69f 100644 --- a/src/shared/install.h +++ b/src/shared/install.h @@ -205,7 +205,14 @@ extern const struct hash_ops unit_file_list_hash_ops_free; InstallChangeType install_changes_add(InstallChange **changes, size_t *n_changes, InstallChangeType type, const char *path, const char *source); void install_changes_free(InstallChange *changes, size_t n_changes); -void install_changes_dump(int r, const char *verb, const InstallChange *changes, size_t n_changes, bool quiet); + +int install_change_dump_error(const InstallChange *change, char **ret_errmsg, const char **ret_bus_error); +void install_changes_dump( + int error, + const char *verb, + const InstallChange *changes, + size_t n_changes, + bool quiet); int unit_file_verify_alias( const InstallInfo *info,