From c413bb28df0996be99fd6b3f2335dfe8739d62fb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Tue, 8 Sep 2020 12:51:23 +0200 Subject: [PATCH 1/3] tree-wide: correct cases where return log_{error,warning} is used without value In various cases, we would say 'return log_warning()' or 'return log_error()'. Those functions return 0 if no error is passed in. For log_warning or log_error this doesn't make sense, and we generally want to propagate the error. In the few cases where the error should be ignored, I think it's better to split it in two, and call 'return 0' on a separate line. --- src/core/execute.c | 17 +++++++++++++---- src/escape/escape.c | 8 +++++--- src/resolve/resolved-manager.c | 3 ++- src/systemctl/systemctl.c | 6 ++++-- src/update-done/update-done.c | 2 +- src/update-utmp/update-utmp.c | 6 ++++-- 6 files changed, 29 insertions(+), 13 deletions(-) diff --git a/src/core/execute.c b/src/core/execute.c index e02a55e2220..16aa4b30189 100644 --- a/src/core/execute.c +++ b/src/core/execute.c @@ -6281,8 +6281,13 @@ int exec_runtime_deserialize_one(Manager *m, const char *value, FDSet *fds) { n = strcspn(v, " "); buf = strndupa(v, n); - if (safe_atoi(buf, &fdpair[0]) < 0 || !fdset_contains(fds, fdpair[0])) - return log_debug("Unable to process exec-runtime netns fd specification."); + + r = safe_atoi(buf, &fdpair[0]); + if (r < 0) + return log_debug_errno(r, "Unable to parse exec-runtime specification netns-socket-0=%s: %m", buf); + if (!fdset_contains(fds, fdpair[0])) + return log_debug_errno(SYNTHETIC_ERRNO(EBADF), + "exec-runtime specification netns-socket-0= refers to unknown fd %d: %m", fdpair[0]); fdpair[0] = fdset_remove(fds, fdpair[0]); if (v[n] != ' ') goto finalize; @@ -6295,8 +6300,12 @@ int exec_runtime_deserialize_one(Manager *m, const char *value, FDSet *fds) { n = strcspn(v, " "); buf = strndupa(v, n); - if (safe_atoi(buf, &fdpair[1]) < 0 || !fdset_contains(fds, fdpair[1])) - return log_debug("Unable to process exec-runtime netns fd specification."); + r = safe_atoi(buf, &fdpair[1]); + if (r < 0) + return log_debug_errno(r, "Unable to parse exec-runtime specification netns-socket-1=%s: %m", buf); + if (!fdset_contains(fds, fdpair[0])) + return log_debug_errno(SYNTHETIC_ERRNO(EBADF), + "exec-runtime specification netns-socket-1= refers to unknown fd %d: %m", fdpair[1]); fdpair[1] = fdset_remove(fds, fdpair[1]); } diff --git a/src/escape/escape.c b/src/escape/escape.c index 0c543a90f6b..3f3dc0a8947 100644 --- a/src/escape/escape.c +++ b/src/escape/escape.c @@ -211,14 +211,16 @@ static int run(int argc, char *argv[]) { if (r < 0) return log_error_errno(r, "Failed to extract instance: %m"); if (isempty(name)) - return log_error("Unit %s is missing the instance name.", *i); + return log_error_errno(SYNTHETIC_ERRNO(EINVAL), + "Unit %s is missing the instance name.", *i); r = unit_name_template(*i, &template); if (r < 0) return log_error_errno(r, "Failed to extract template: %m"); if (arg_template && !streq(arg_template, template)) - return log_error("Unit %s template %s does not match specified template %s.", - *i, template, arg_template); + return log_error_errno(SYNTHETIC_ERRNO(EINVAL), + "Unit %s template %s does not match specified template %s.", + *i, template, arg_template); } else { name = strdup(*i); if (!name) diff --git a/src/resolve/resolved-manager.c b/src/resolve/resolved-manager.c index ddd336b4897..fc3ab98cd63 100644 --- a/src/resolve/resolved-manager.c +++ b/src/resolve/resolved-manager.c @@ -350,7 +350,8 @@ static int determine_hostname(char **full_hostname, char **llmnr_hostname, char #if HAVE_LIBIDN2 r = idn2_to_unicode_8z8z(label, &utf8, 0); if (r != IDN2_OK) - return log_error("Failed to undo IDNA: %s", idn2_strerror(r)); + return log_error_errno(SYNTHETIC_ERRNO(EUCLEAN), + "Failed to undo IDNA: %s", idn2_strerror(r)); assert(utf8_is_valid(utf8)); r = strlen(utf8); diff --git a/src/systemctl/systemctl.c b/src/systemctl/systemctl.c index e22efb92ac5..f23bc6b234b 100644 --- a/src/systemctl/systemctl.c +++ b/src/systemctl/systemctl.c @@ -2086,8 +2086,10 @@ static int parse_proc_cmdline_item(const char *key, const char *value, void *dat if (streq(key, "systemd.unit")) { if (proc_cmdline_value_missing(key, value)) return 0; - if (!unit_name_is_valid(value, UNIT_NAME_PLAIN|UNIT_NAME_INSTANCE)) - return log_warning("Unit name specified on %s= is not valid, ignoring: %s", key, value); + if (!unit_name_is_valid(value, UNIT_NAME_PLAIN|UNIT_NAME_INSTANCE)) { + log_warning("Unit name specified on %s= is not valid, ignoring: %s", key, value); + return 0; + } return free_and_strdup_warn(ret, key); diff --git a/src/update-done/update-done.c b/src/update-done/update-done.c index c001802dc91..e9d589e0e53 100644 --- a/src/update-done/update-done.c +++ b/src/update-done/update-done.c @@ -31,7 +31,7 @@ static int apply_timestamp(const char *path, struct timespec *ts) { r = write_string_file_atomic_label_ts(path, message, ts); if (r == -EROFS) - return log_debug("Cannot create \"%s\", file system is read-only.", path); + return log_debug_errno(r, "Cannot create \"%s\", file system is read-only.", path); if (r < 0) return log_error_errno(r, "Failed to write \"%s\": %m", path); return 0; diff --git a/src/update-utmp/update-utmp.c b/src/update-utmp/update-utmp.c index 47354d50129..4ab90a63ed8 100644 --- a/src/update-utmp/update-utmp.c +++ b/src/update-utmp/update-utmp.c @@ -187,8 +187,10 @@ static int on_runlevel(Context *c) { runlevel = get_current_runlevel(c); if (runlevel < 0) return runlevel; - if (runlevel == 0) - return log_warning("Failed to get new runlevel, utmp update skipped."); + if (runlevel == 0) { + log_warning("Failed to get new runlevel, utmp update skipped."); + return 0; + } if (previous == runlevel) return 0; From 44f0dd628ce4ca9565b0e02b8cb63ed8272529cd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Tue, 8 Sep 2020 13:18:25 +0200 Subject: [PATCH 2/3] basic/log: make log_{info,warning,...} return void log_debug still returns 0. I think it is legitimate to use 'return log_debug()' to return 0. It is different than the other functions, since we often want to supress errors logged at debug level. This case is quite common in the codebase and we could use 'return log_debug_errno()' to make the code more consise. For all other variants, a separate return line is required. Previous commit changes all the non-conforming instances, now we can make it mandatory. --- src/basic/log.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/basic/log.h b/src/basic/log.h index 15807d3029f..137d21005d4 100644 --- a/src/basic/log.h +++ b/src/basic/log.h @@ -234,12 +234,12 @@ void log_assert_failed_return_realm( #define log_full_errno(level, error, ...) \ log_full_errno_realm(LOG_REALM, (level), (error), __VA_ARGS__) -#define log_full(level, ...) log_full_errno((level), 0, __VA_ARGS__) +#define log_full(level, ...) (void) log_full_errno((level), 0, __VA_ARGS__) int log_emergency_level(void); /* Normal logging */ -#define log_debug(...) log_full(LOG_DEBUG, __VA_ARGS__) +#define log_debug(...) log_full_errno(LOG_DEBUG, 0, __VA_ARGS__) #define log_info(...) log_full(LOG_INFO, __VA_ARGS__) #define log_notice(...) log_full(LOG_NOTICE, __VA_ARGS__) #define log_warning(...) log_full(LOG_WARNING, __VA_ARGS__) From 1c5950bd61e3e1ea67ad7428a32929c9768e2d8e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Tue, 8 Sep 2020 13:19:53 +0200 Subject: [PATCH 3/3] sleep: reword some debug messages I think the sentences sound more natural this way. --- src/sleep/sleep.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/src/sleep/sleep.c b/src/sleep/sleep.c index cf7e238ad4c..6bb1a04793d 100644 --- a/src/sleep/sleep.c +++ b/src/sleep/sleep.c @@ -160,14 +160,13 @@ static int lock_all_homes(void) { r = sd_bus_call(bus, m, DEFAULT_TIMEOUT_USEC, &error, NULL); if (r < 0) { - if (bus_error_is_unknown_service(&error)) - return log_debug("systemd-homed is not running, skipping locking of home directories."); + if (!bus_error_is_unknown_service(&error)) + return log_error_errno(r, "Failed to lock home directories: %s", bus_error_message(&error, r)); - return log_error_errno(r, "Failed to lock home directories: %s", bus_error_message(&error, r)); + return log_debug("systemd-homed is not running, locking of home directories skipped."); } - log_debug("Successfully requested for all home directories to be locked."); - return 0; + return log_debug("Successfully requested locking of all home directories."); } static int execute(char **modes, char **states) {