From 047d7219fde661698d3487fc49e9878c61eefd77 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Sat, 1 Jul 2017 19:05:36 -0400 Subject: [PATCH 1/3] core: do not print color console message about gc-ed jobs This is just a cosmetic issue. Garbage collection of jobs (especially the ones that we create automatically) is something of an internal implementation detail and should not be made visible to the users. But it's probably still useful to log this in the journal, so the code is rearranged to skip one of the messages if we log to the console and the journal separately, and to keep the message if we log everything to the console. Fixes #6254. --- src/core/job.c | 48 +++++++++++++++++++++++++----------------------- 1 file changed, 25 insertions(+), 23 deletions(-) diff --git a/src/core/job.c b/src/core/job.c index d197ad0da7..8e2039d321 100644 --- a/src/core/job.c +++ b/src/core/job.c @@ -697,20 +697,20 @@ _pure_ static const char *job_get_status_message_format(Unit *u, JobType t, JobR return NULL; } -static void job_print_status_message(Unit *u, JobType t, JobResult result) { - static const struct { - const char *color, *word; - } statuses[_JOB_RESULT_MAX] = { - [JOB_DONE] = { ANSI_GREEN, " OK " }, - [JOB_TIMEOUT] = { ANSI_HIGHLIGHT_RED, " TIME " }, - [JOB_FAILED] = { ANSI_HIGHLIGHT_RED, "FAILED" }, - [JOB_DEPENDENCY] = { ANSI_HIGHLIGHT_YELLOW, "DEPEND" }, - [JOB_SKIPPED] = { ANSI_HIGHLIGHT, " INFO " }, - [JOB_ASSERT] = { ANSI_HIGHLIGHT_YELLOW, "ASSERT" }, - [JOB_UNSUPPORTED] = { ANSI_HIGHLIGHT_YELLOW, "UNSUPP" }, - [JOB_COLLECTED] = { ANSI_HIGHLIGHT, " INFO " }, - }; +static const struct { + const char *color, *word; +} job_print_status_messages [_JOB_RESULT_MAX] = { + [JOB_DONE] = { ANSI_GREEN, " OK " }, + [JOB_TIMEOUT] = { ANSI_HIGHLIGHT_RED, " TIME " }, + [JOB_FAILED] = { ANSI_HIGHLIGHT_RED, "FAILED" }, + [JOB_DEPENDENCY] = { ANSI_HIGHLIGHT_YELLOW, "DEPEND" }, + [JOB_SKIPPED] = { ANSI_HIGHLIGHT, " INFO " }, + [JOB_ASSERT] = { ANSI_HIGHLIGHT_YELLOW, "ASSERT" }, + [JOB_UNSUPPORTED] = { ANSI_HIGHLIGHT_YELLOW, "UNSUPP" }, + /* JOB_COLLECTED */ +}; +static void job_print_status_message(Unit *u, JobType t, JobResult result) { const char *format; const char *status; @@ -722,14 +722,19 @@ static void job_print_status_message(Unit *u, JobType t, JobResult result) { if (t == JOB_RELOAD) return; + if (!job_print_status_messages[result].word) + return; + format = job_get_status_message_format(u, t, result); if (!format) return; if (log_get_show_color()) - status = strjoina(statuses[result].color, statuses[result].word, ANSI_NORMAL); + status = strjoina(job_print_status_messages[result].color, + job_print_status_messages[result].word, + ANSI_NORMAL); else - status = statuses[result].word; + status = job_print_status_messages[result].word; if (result != JOB_DONE) manager_flip_auto_status(u->manager, true); @@ -766,10 +771,9 @@ static void job_log_status_message(Unit *u, JobType t, JobResult result) { assert(t >= 0); assert(t < _JOB_TYPE_MAX); - /* Skip this if it goes to the console. since we already print - * to the console anyway... */ - - if (log_on_console()) + /* Skip printing if output goes to the console, and job_print_status_message() + will actually print something to the console. */ + if (log_on_console() && job_print_status_messages[result].word) return; format = job_get_status_message_format(u, t, result); @@ -1310,9 +1314,8 @@ bool job_check_gc(Job *j) { return true; } - /* If we are going down, but something else is orederd After= us, then it needs to wait for us */ - if (IN_SET(j->type, JOB_STOP, JOB_RESTART)) { - + /* If we are going down, but something else is ordered After= us, then it needs to wait for us */ + if (IN_SET(j->type, JOB_STOP, JOB_RESTART)) SET_FOREACH(other, j->unit->dependencies[UNIT_AFTER], i) { if (!other->job) continue; @@ -1322,7 +1325,6 @@ bool job_check_gc(Job *j) { return true; } - } /* The logic above is kinda the inverse of the job_is_runnable() logic. Specifically, if the job "we" is * ordered before the job "other": From c23218ae25248f3e60127da66536974bf354fd2c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Sun, 2 Jul 2017 10:23:07 -0400 Subject: [PATCH 2/3] core/loopback-setup: make log messages nicer Under nspawn, systemd would print: Got address error code: Operation not permitted Got address error code: Operation not permitted Got start error code: Operation not permitted which is quite unclear out of context. Change that to: Failed to add address 127.0.0.1 to loopback interface: Operation not permitted Failed to add address ::1 to loopback interface: Operation not permitted Failed to bring loopback interface up: Operation not permitted --- src/core/loopback-setup.c | 50 +++++++++++++++------------------------ 1 file changed, 19 insertions(+), 31 deletions(-) diff --git a/src/core/loopback-setup.c b/src/core/loopback-setup.c index a7b801c47d..c167305ca9 100644 --- a/src/core/loopback-setup.c +++ b/src/core/loopback-setup.c @@ -31,9 +31,10 @@ struct state { unsigned n_messages; int rcode; + const char *title; }; -static int start_loopback_handler(sd_netlink *rtnl, sd_netlink_message *m, void *userdata) { +static int generic_handler(sd_netlink *rtnl, sd_netlink_message *m, void *userdata) { struct state *s = userdata; assert(s); @@ -41,7 +42,7 @@ static int start_loopback_handler(sd_netlink *rtnl, sd_netlink_message *m, void s->n_messages--; errno = 0; - log_debug_errno(sd_netlink_message_get_errno(m), "Got start error code: %m"); + log_debug_errno(sd_netlink_message_get_errno(m), "Failed to %s: %m", s->title); s->rcode = sd_netlink_message_get_errno(m); @@ -63,7 +64,7 @@ static int start_loopback(sd_netlink *rtnl, struct state *s) { if (r < 0) return r; - r = sd_netlink_call_async(rtnl, req, start_loopback_handler, s, LOOPBACK_SETUP_TIMEOUT_USEC, NULL); + r = sd_netlink_call_async(rtnl, req, generic_handler, s, LOOPBACK_SETUP_TIMEOUT_USEC, NULL); if (r < 0) return r; @@ -71,20 +72,6 @@ static int start_loopback(sd_netlink *rtnl, struct state *s) { return 0; } -static int generic_handler(sd_netlink *rtnl, sd_netlink_message *m, void *userdata) { - struct state *s = userdata; - - assert(s); - assert(s->n_messages > 0); - s->n_messages--; - - /* Note that we don't really care whether the addresses could be added or not */ - errno = 0; - log_debug_errno(sd_netlink_message_get_errno(m), "Got address error code: %m"); - - return 0; -} - static int add_ipv4_address(sd_netlink *rtnl, struct state *s) { _cleanup_(sd_netlink_message_unrefp) sd_netlink_message *req = NULL; int r; @@ -177,7 +164,9 @@ static bool check_loopback(sd_netlink *rtnl) { int loopback_setup(void) { _cleanup_(sd_netlink_unrefp) sd_netlink *rtnl = NULL; - struct state state = {}; + struct state state_4 = { .title = "add address 127.0.0.1 to loopback interface" }, + state_6 = { .title = "add address ::1 to loopback interface"}, + state_up = { .title = "bring loopback interface up" }; int r; r = sd_netlink_open(&rtnl); @@ -190,19 +179,19 @@ int loopback_setup(void) { * * https://github.com/systemd/systemd/issues/5641 */ - r = add_ipv4_address(rtnl, &state); + r = add_ipv4_address(rtnl, &state_4); if (r < 0) return log_error_errno(r, "Failed to enqueue IPv4 loopback address add request: %m"); - r = add_ipv6_address(rtnl, &state); + r = add_ipv6_address(rtnl, &state_6); if (r < 0) - return log_error_errno(r, "Failed to enqueue IPv4 loopback address add request: %m"); + return log_error_errno(r, "Failed to enqueue IPv6 loopback address add request: %m"); - r = start_loopback(rtnl, &state); + r = start_loopback(rtnl, &state_up); if (r < 0) return log_error_errno(r, "Failed to enqueue loopback interface start request: %m"); - while (state.n_messages > 0) { + while (state_4.n_messages + state_6.n_messages + state_up.n_messages > 0) { r = sd_netlink_wait(rtnl, LOOPBACK_SETUP_TIMEOUT_USEC); if (r < 0) return log_error_errno(r, "Failed to wait for netlink event: %m"); @@ -212,16 +201,15 @@ int loopback_setup(void) { return log_warning_errno(r, "Failed to process netlink event: %m"); } - if (state.rcode != 0) { - - /* If we lack the permissions to configure the - * loopback device, but we find it to be already - * configured, let's exit cleanly, in order to - * supported unprivileged containers. */ - if (state.rcode == -EPERM && check_loopback(rtnl)) + /* Note that we don't really care whether the addresses could be added or not */ + if (state_up.rcode != 0) { + /* If we lack the permissions to configure the loopback device, + * but we find it to be already configured, let's exit cleanly, + * in order to supported unprivileged containers. */ + if (state_up.rcode == -EPERM && check_loopback(rtnl)) return 0; - return log_warning_errno(state.rcode, "Failed to configure loopback device: %m"); + return log_warning_errno(state_up.rcode, "Failed to configure loopback device: %m"); } return 0; From bd68e99bd005de29c2cbf6adfb4c6938ee571f61 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Sun, 2 Jul 2017 12:03:25 -0400 Subject: [PATCH 3/3] Be slightly more verbose in error message Including the full path is always useful. Also use PID_FMT in one more place. --- src/basic/cgroup-util.c | 2 +- src/nspawn/nspawn-cgroup.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/basic/cgroup-util.c b/src/basic/cgroup-util.c index c828d10149..6344372610 100644 --- a/src/basic/cgroup-util.c +++ b/src/basic/cgroup-util.c @@ -840,7 +840,7 @@ int cg_attach(const char *controller, const char *path, pid_t pid) { if (r > 0 && streq(controller, SYSTEMD_CGROUP_CONTROLLER)) { r = cg_attach(SYSTEMD_CGROUP_CONTROLLER_LEGACY, path, pid); if (r < 0) - log_warning_errno(r, "Failed to attach %d to compat systemd cgroup %s: %m", pid, path); + log_warning_errno(r, "Failed to attach "PID_FMT" to compat systemd cgroup %s: %m", pid, path); } return 0; diff --git a/src/nspawn/nspawn-cgroup.c b/src/nspawn/nspawn-cgroup.c index d749756437..fd565c09cd 100644 --- a/src/nspawn/nspawn-cgroup.c +++ b/src/nspawn/nspawn-cgroup.c @@ -49,7 +49,7 @@ static int chown_cgroup_path(const char *path, uid_t uid_shift) { "cgroup.subtree_control") if (fchownat(fd, fn, uid_shift, uid_shift, 0) < 0) log_full_errno(errno == ENOENT ? LOG_DEBUG : LOG_WARNING, errno, - "Failed to chown() cgroup file %s, ignoring: %m", fn); + "Failed to chown \"%s/%s\", ignoring: %m", path, fn); return 0; }