From 9021ff17e2a6e08e0675bc69c8b18e2ddb63de4a Mon Sep 17 00:00:00 2001 From: Zbigniew Jędrzejewski-Szmek Date: Sat, 22 Oct 2016 18:53:24 -0400 Subject: pid1: only log about added fd if it was really added If it was a duplicate, log nothing. --- src/core/service.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/core/service.c b/src/core/service.c index ee4f4983fc..c699a31941 100644 --- a/src/core/service.c +++ b/src/core/service.c @@ -368,6 +368,8 @@ static int service_add_fd_store(Service *s, int fd, const char *name) { ServiceFDStore *fs; int r; + /* fd is always consumed if we return >= 0 */ + assert(s); assert(fd >= 0); @@ -379,9 +381,8 @@ static int service_add_fd_store(Service *s, int fd, const char *name) { if (r < 0) return r; if (r > 0) { - /* Already included */ safe_close(fd); - return 1; + return 0; /* fd already included */ } } @@ -409,7 +410,7 @@ static int service_add_fd_store(Service *s, int fd, const char *name) { LIST_PREPEND(fd_store, s->fd_store, fs); s->n_fd_store++; - return 1; + return 1; /* fd newly stored */ } static int service_add_fd_store_set(Service *s, FDSet *fds, const char *name) { @@ -430,10 +431,9 @@ static int service_add_fd_store_set(Service *s, FDSet *fds, const char *name) { r = service_add_fd_store(s, fd, name); if (r < 0) return log_unit_error_errno(UNIT(s), r, "Couldn't add fd to fd store: %m"); - if (r > 0) { + if (r > 0) log_unit_debug(UNIT(s), "Added fd to fd store."); - fd = -1; - } + fd = -1; } if (fdset_size(fds) > 0) @@ -2336,7 +2336,7 @@ static int service_deserialize_item(Unit *u, const char *key, const char *value, r = service_add_fd_store(s, fd, t); if (r < 0) log_unit_error_errno(u, r, "Failed to add fd to store: %m"); - else if (r > 0) + else fdset_remove(fds, fd); } -- cgit v1.2.3-54-g00ecf From 16f70d6362536c7432f0fd0af0155a5cd95c4a76 Mon Sep 17 00:00:00 2001 From: Zbigniew Jędrzejewski-Szmek Date: Sat, 22 Oct 2016 20:01:37 -0400 Subject: pid1: nicely log when doing operation on stored fds Should help with debugging #4408. --- src/core/service.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/core/service.c b/src/core/service.c index c699a31941..0775bd6a9f 100644 --- a/src/core/service.c +++ b/src/core/service.c @@ -360,6 +360,10 @@ static int on_fd_store_io(sd_event_source *e, int fd, uint32_t revents, void *us assert(fs); /* If we get either EPOLLHUP or EPOLLERR, it's time to remove this entry from the fd store */ + log_unit_debug(UNIT(fs->service), + "Received %s on stored fd %d (%s), closing.", + revents & EPOLLERR ? "EPOLLERR" : "EPOLLHUP", + fs->fd, strna(fs->fdname)); service_fd_store_unlink(fs); return 0; } @@ -432,7 +436,7 @@ static int service_add_fd_store_set(Service *s, FDSet *fds, const char *name) { if (r < 0) return log_unit_error_errno(UNIT(s), r, "Couldn't add fd to fd store: %m"); if (r > 0) - log_unit_debug(UNIT(s), "Added fd to fd store."); + log_unit_debug(UNIT(s), "Added fd %u (%s) to fd store.", fd, strna(name)); fd = -1; } @@ -1225,6 +1229,7 @@ static int service_spawn( return r; n_fds = r; + log_unit_debug(UNIT(s), "Passing %i fds to service", n_fds); } r = service_arm_timer(s, usec_add(now(CLOCK_MONOTONIC), timeout)); -- cgit v1.2.3-54-g00ecf From 99bdcdc7fc32def6cd6eab69c3869661b4b1888b Mon Sep 17 00:00:00 2001 From: Zbigniew Jędrzejewski-Szmek Date: Sat, 22 Oct 2016 20:32:59 -0400 Subject: man: add a note that FDSTORE=1 requires epoll-compatible fds Let's say that this was not obvious from our man page. --- man/sd_notify.xml | 41 +++++++++++++++++++---------------------- man/systemd.service.xml | 4 ++-- 2 files changed, 21 insertions(+), 24 deletions(-) diff --git a/man/sd_notify.xml b/man/sd_notify.xml index 025fbec6c1..94542b80b8 100644 --- a/man/sd_notify.xml +++ b/man/sd_notify.xml @@ -205,28 +205,25 @@ FDSTORE=1 - Stores additional file descriptors in the - service manager. File descriptors sent this way will be - maintained per-service by the service manager and be passed - again using the usual file descriptor passing logic on the - next invocation of the service (see - sd_listen_fds3). - This is useful for implementing service restart schemes where - services serialize their state to /run, - push their file descriptors to the system manager, and are - then restarted, retrieving their state again via socket - passing and /run. Note that the service - manager will accept messages for a service only if - FileDescriptorStoreMax= is set to non-zero - for it (defaults to zero). See - systemd.service5 - for details. Multiple arrays of file descriptors may be sent - in separate messages, in which case the arrays are combined. - Note that the service manager removes duplicate file - descriptors before passing them to the service. Use - sd_pid_notify_with_fds() to send messages - with FDSTORE=1, see - below. + Stores additional file descriptors in the service manager. File + descriptors sent this way will be maintained per-service by the service manager + and will be passed again using the usual file descriptor passing logic on the next + invocation of the service, see + sd_listen_fds3. + This is useful for implementing service restart schemes where services serialize + their state to /run, push their file descriptors to the + system manager, and are then restarted, retrieving their state again via socket + passing and /run. Note that the service manager will accept + messages for a service only if FileDescriptorStoreMax= is set + to non-zero for it (defaults to zero, see + systemd.service5). + File descriptors must be pollable, see + epoll_ctl2. + Multiple arrays of file descriptors may be sent in separate messages, in which + case the arrays are combined. Note that the service manager removes duplicate + file descriptors before passing them to the service. Use + sd_pid_notify_with_fds() to send messages with + FDSTORE=1, see below. diff --git a/man/systemd.service.xml b/man/systemd.service.xml index 90b1312603..5c65957bda 100644 --- a/man/systemd.service.xml +++ b/man/systemd.service.xml @@ -852,13 +852,13 @@ serialized to /run and the file descriptors passed to the service manager, to allow restarts without losing state. Defaults to 0, i.e. no file descriptors - may be stored in the service manager by default. All file + may be stored in the service manager. All file descriptors passed to the service manager from a specific service are passed back to the service's main process on the next service restart. Any file descriptors passed to the service manager are automatically closed when POLLHUP or POLLERR is seen on them, or when the service is fully stopped - and no job queued or being executed for it. + and no job is queued or being executed for it. -- cgit v1.2.3-54-g00ecf From f0bfbfac43b7faa68ef1bb2ad659c191b9ec85d2 Mon Sep 17 00:00:00 2001 From: Zbigniew Jędrzejewski-Szmek Date: Sat, 22 Oct 2016 22:16:02 -0400 Subject: core: when restarting services, don't close fds We would close all the stored fds in service_release_resources(), which of course broke the whole concept of storing fds over service restart. Fixes #4408. --- src/core/service.c | 22 +++++++++++++++------- src/core/unit.c | 6 ++++-- src/core/unit.h | 2 +- 3 files changed, 20 insertions(+), 10 deletions(-) diff --git a/src/core/service.c b/src/core/service.c index 0775bd6a9f..00ce500fd3 100644 --- a/src/core/service.c +++ b/src/core/service.c @@ -289,7 +289,17 @@ static void service_fd_store_unlink(ServiceFDStore *fs) { free(fs); } -static void service_release_resources(Unit *u) { +static void service_release_fd_store(Service *s) { + assert(s); + + log_unit_debug(UNIT(s), "Releasing all stored fds"); + while (s->fd_store) + service_fd_store_unlink(s->fd_store); + + assert(s->n_fd_store == 0); +} + +static void service_release_resources(Unit *u, bool inactive) { Service *s = SERVICE(u); assert(s); @@ -297,16 +307,14 @@ static void service_release_resources(Unit *u) { if (!s->fd_store && s->stdin_fd < 0 && s->stdout_fd < 0 && s->stderr_fd < 0) return; - log_unit_debug(u, "Releasing all resources."); + log_unit_debug(u, "Releasing resources."); s->stdin_fd = safe_close(s->stdin_fd); s->stdout_fd = safe_close(s->stdout_fd); s->stderr_fd = safe_close(s->stderr_fd); - while (s->fd_store) - service_fd_store_unlink(s->fd_store); - - assert(s->n_fd_store == 0); + if (inactive) + service_release_fd_store(s); } static void service_done(Unit *u) { @@ -350,7 +358,7 @@ static void service_done(Unit *u) { s->timer_event_source = sd_event_source_unref(s->timer_event_source); - service_release_resources(u); + service_release_resources(u, true); } static int on_fd_store_io(sd_event_source *e, int fd, uint32_t revents, void *userdata) { diff --git a/src/core/unit.c b/src/core/unit.c index cabb1050a8..8e6cef103b 100644 --- a/src/core/unit.c +++ b/src/core/unit.c @@ -302,6 +302,7 @@ int unit_set_description(Unit *u, const char *description) { bool unit_check_gc(Unit *u) { UnitActiveState state; + bool inactive; assert(u); if (u->job) @@ -311,16 +312,17 @@ bool unit_check_gc(Unit *u) { return true; state = unit_active_state(u); + inactive = state == UNIT_INACTIVE; /* If the unit is inactive and failed and no job is queued for * it, then release its runtime resources */ if (UNIT_IS_INACTIVE_OR_FAILED(state) && UNIT_VTABLE(u)->release_resources) - UNIT_VTABLE(u)->release_resources(u); + UNIT_VTABLE(u)->release_resources(u, inactive); /* But we keep the unit object around for longer when it is * referenced or configured to not be gc'ed */ - if (state != UNIT_INACTIVE) + if (!inactive) return true; if (u->no_gc) diff --git a/src/core/unit.h b/src/core/unit.h index adcdee6db6..e01ec7a775 100644 --- a/src/core/unit.h +++ b/src/core/unit.h @@ -372,7 +372,7 @@ struct UnitVTable { /* When the unit is not running and no job for it queued we * shall release its runtime resources */ - void (*release_resources)(Unit *u); + void (*release_resources)(Unit *u, bool inactive); /* Invoked on every child that died */ void (*sigchld_event)(Unit *u, pid_t pid, int code, int status); -- cgit v1.2.3-54-g00ecf From bff653e3970bb79832568ae86b095ee530b62302 Mon Sep 17 00:00:00 2001 From: Evgeny Vereshchagin Date: Thu, 20 Oct 2016 13:18:12 +0000 Subject: tests: add test that journald keeps fds over termination by signal This test fails before previous commit, and passes with it. --- test/TEST-04-JOURNAL/test-journal.sh | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/test/TEST-04-JOURNAL/test-journal.sh b/test/TEST-04-JOURNAL/test-journal.sh index 6646eccfa7..493ff00ce0 100755 --- a/test/TEST-04-JOURNAL/test-journal.sh +++ b/test/TEST-04-JOURNAL/test-journal.sh @@ -59,4 +59,12 @@ sleep 3 systemctl stop forever-print-hola [[ ! -f "/i-lose-my-logs" ]] +# https://github.com/systemd/systemd/issues/4408 +rm -f /i-lose-my-logs +systemctl start forever-print-hola +sleep 3 +systemctl kill --signal=SIGKILL systemd-journald +sleep 3 +[[ ! -f "/i-lose-my-logs" ]] + touch /testok -- cgit v1.2.3-54-g00ecf From b09246352f751c95ab06f598b639f6f6ca0e67b1 Mon Sep 17 00:00:00 2001 From: Zbigniew Jędrzejewski-Szmek Date: Wed, 2 Nov 2016 15:00:54 -0400 Subject: pid1: fix fd memleak when we hit FileDescriptorStoreMax limit Since service_add_fd_store() already does the check, remove the redundant check from service_add_fd_store_set(). Also, print a warning when repopulating FDStore after daemon-reexec and we hit the limit. This is a user visible issue, so we should not discard fds silently. (Note that service_deserialize_item is impacted by the return value from service_add_fd_store(), but we rely on the general error message, so the caller does not need to be modified, and does not show up in the diff.) --- src/core/service.c | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/src/core/service.c b/src/core/service.c index 00ce500fd3..a7274a758f 100644 --- a/src/core/service.c +++ b/src/core/service.c @@ -386,7 +386,9 @@ static int service_add_fd_store(Service *s, int fd, const char *name) { assert(fd >= 0); if (s->n_fd_store >= s->n_fd_store_max) - return 0; + return -EXFULL; /* Our store is full. + * Use this errno rather than E[NM]FILE to distinguish from + * the case where systemd itself hits the file limit. */ LIST_FOREACH(fd_store, fs, s->fd_store) { r = same_fd(fs->fd, fd); @@ -430,10 +432,7 @@ static int service_add_fd_store_set(Service *s, FDSet *fds, const char *name) { assert(s); - if (fdset_size(fds) <= 0) - return 0; - - while (s->n_fd_store < s->n_fd_store_max) { + while (fdset_size(fds) > 0) { _cleanup_close_ int fd = -1; fd = fdset_steal_first(fds); @@ -441,16 +440,17 @@ static int service_add_fd_store_set(Service *s, FDSet *fds, const char *name) { break; r = service_add_fd_store(s, fd, name); + if (r == -EXFULL) + return log_unit_warning_errno(UNIT(s), r, + "Cannot store more fds than FileDescriptorStoreMax=%u, closing remaining.", + s->n_fd_store_max); if (r < 0) - return log_unit_error_errno(UNIT(s), r, "Couldn't add fd to fd store: %m"); + return log_unit_error_errno(UNIT(s), r, "Failed to add fd to store: %m"); if (r > 0) log_unit_debug(UNIT(s), "Added fd %u (%s) to fd store.", fd, strna(name)); fd = -1; } - if (fdset_size(fds) > 0) - log_unit_warning(UNIT(s), "Tried to store more fds than FileDescriptorStoreMax=%u allows, closing remaining.", s->n_fd_store_max); - return 0; } -- cgit v1.2.3-54-g00ecf