From 108dfff2c7aebadb78e485ed564caf559367bf7c Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 22 Nov 2022 12:14:33 +0100 Subject: [PATCH 01/10] tree-wide: modernizations with RET_NERRNO() --- src/shared/ask-password-api.c | 30 +++++++++++++----------------- src/shared/barrier.c | 4 +--- 2 files changed, 14 insertions(+), 20 deletions(-) diff --git a/src/shared/ask-password-api.c b/src/shared/ask-password-api.c index e7db23c201..1ad5ddd503 100644 --- a/src/shared/ask-password-api.c +++ b/src/shared/ask-password-api.c @@ -235,8 +235,7 @@ int ask_password_plymouth( if (notify < 0) return -errno; - r = inotify_add_watch(notify, flag_file, IN_ATTRIB); /* for the link count */ - if (r < 0) + if (inotify_add_watch(notify, flag_file, IN_ATTRIB) < 0) /* for the link count */ return -errno; } @@ -244,8 +243,7 @@ int ask_password_plymouth( if (fd < 0) return -errno; - r = connect(fd, &sa.sa, SOCKADDR_UN_LEN(sa.un)); - if (r < 0) + if (connect(fd, &sa.sa, SOCKADDR_UN_LEN(sa.un)) < 0) return -errno; if (FLAGS_SET(flags, ASK_PASSWORD_ACCEPT_CACHED)) { @@ -469,10 +467,9 @@ int ask_password_tty( new_termios.c_cc[VMIN] = 1; new_termios.c_cc[VTIME] = 0; - if (tcsetattr(ttyfd, TCSADRAIN, &new_termios) < 0) { - r = -errno; + r = RET_NERRNO(tcsetattr(ttyfd, TCSADRAIN, &new_termios)); + if (r < 0) goto finish; - } reset_tty = true; } @@ -496,11 +493,11 @@ int ask_password_tty( else timeout = USEC_INFINITY; - if (flag_file) - if (access(flag_file, F_OK) < 0) { - r = -errno; + if (flag_file) { + r = RET_NERRNO(access(flag_file, F_OK)); + if (r < 0) goto finish; - } + } r = ppoll_usec(pollfd, notify >= 0 ? 2 : 1, timeout); if (r == -EINTR) @@ -752,10 +749,10 @@ int ask_password_agent( r = -errno; goto finish; } - if (inotify_add_watch(notify, "/run/systemd/ask-password", IN_ATTRIB /* for mtime */) < 0) { - r = -errno; + + r = RET_NERRNO(inotify_add_watch(notify, "/run/systemd/ask-password", IN_ATTRIB /* for mtime */)); + if (r < 0) goto finish; - } } fd = mkostemp_safe(temp); @@ -818,10 +815,9 @@ int ask_password_agent( final[sizeof(final)-10] = 's'; final[sizeof(final)-9] = 'k'; - if (rename(temp, final) < 0) { - r = -errno; + r = RET_NERRNO(rename(temp, final)); + if (r < 0) goto finish; - } zero(pollfd); pollfd[FD_SOCKET].fd = socket_fd; diff --git a/src/shared/barrier.c b/src/shared/barrier.c index cbe54a60cd..d76a61a5db 100644 --- a/src/shared/barrier.c +++ b/src/shared/barrier.c @@ -92,7 +92,6 @@ */ int barrier_create(Barrier *b) { _unused_ _cleanup_(barrier_destroyp) Barrier *staging = b; - int r; assert(b); @@ -104,8 +103,7 @@ int barrier_create(Barrier *b) { if (b->them < 0) return -errno; - r = pipe2(b->pipe, O_CLOEXEC | O_NONBLOCK); - if (r < 0) + if (pipe2(b->pipe, O_CLOEXEC | O_NONBLOCK) < 0) return -errno; staging = NULL; From 3022916b4d2483452c3ddbbac9ee7c4372b1cb46 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Mon, 21 Nov 2022 17:42:04 +0100 Subject: [PATCH 02/10] sd-bus: handle -EINTR return from bus_poll() In sd_bus_wait(), let's convert EINTR to a return code of 0, thus asking the caller do loop again and enter sd_bus_process() again (which will not find any queued events). This way we'll not return an error on something that isn't really an error. This should typically make sure things are properly handled by the caller, magically, without eating up the event entirely, and still giving the caller time to run some code if they want. --- src/libsystemd/sd-bus/bus-socket.c | 5 ++++- src/libsystemd/sd-bus/sd-bus.c | 18 +++++++++++++++--- 2 files changed, 19 insertions(+), 4 deletions(-) diff --git a/src/libsystemd/sd-bus/bus-socket.c b/src/libsystemd/sd-bus/bus-socket.c index c94befef73..253f41c636 100644 --- a/src/libsystemd/sd-bus/bus-socket.c +++ b/src/libsystemd/sd-bus/bus-socket.c @@ -1308,8 +1308,11 @@ int bus_socket_process_opening(sd_bus *b) { assert(b->state == BUS_OPENING); events = fd_wait_for_event(b->output_fd, POLLOUT, 0); - if (events < 0) + if (events < 0) { + if (ERRNO_IS_TRANSIENT(events)) + return 0; return events; + } if (!(events & (POLLOUT|POLLERR|POLLHUP))) return 0; diff --git a/src/libsystemd/sd-bus/sd-bus.c b/src/libsystemd/sd-bus/sd-bus.c index ba5ef7de00..c75276f4ba 100644 --- a/src/libsystemd/sd-bus/sd-bus.c +++ b/src/libsystemd/sd-bus/sd-bus.c @@ -2465,8 +2465,11 @@ _public_ int sd_bus_call( left = UINT64_MAX; r = bus_poll(bus, true, left); - if (r < 0) + if (r < 0) { + if (ERRNO_IS_TRANSIENT(r)) + continue; goto fail; + } if (r == 0) { r = -ETIMEDOUT; goto fail; @@ -3321,6 +3324,7 @@ static int bus_poll(sd_bus *bus, bool need_more, uint64_t timeout_usec) { } _public_ int sd_bus_wait(sd_bus *bus, uint64_t timeout_usec) { + int r; assert_return(bus, -EINVAL); assert_return(bus = bus_resolve(bus), -ENOPKG); @@ -3335,7 +3339,11 @@ _public_ int sd_bus_wait(sd_bus *bus, uint64_t timeout_usec) { if (bus->rqueue_size > 0) return 0; - return bus_poll(bus, false, timeout_usec); + r = bus_poll(bus, false, timeout_usec); + if (r < 0 && ERRNO_IS_TRANSIENT(r)) + return 1; /* treat EINTR as success, but let's exit, so that the caller will call back into us soon. */ + + return r; } _public_ int sd_bus_flush(sd_bus *bus) { @@ -3377,8 +3385,12 @@ _public_ int sd_bus_flush(sd_bus *bus) { return 0; r = bus_poll(bus, false, UINT64_MAX); - if (r < 0) + if (r < 0) { + if (ERRNO_IS_TRANSIENT(r)) + continue; + return r; + } } } From 7c75f34131772781f690860de797d3e35fd0bed9 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 22 Nov 2022 12:18:07 +0100 Subject: [PATCH 03/10] stdio-bridge: don't be bothered with EINTR We handle signals via signal handlers, hence no need to be concerned about EINTR. --- src/stdio-bridge/stdio-bridge.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/stdio-bridge/stdio-bridge.c b/src/stdio-bridge/stdio-bridge.c index 3c5ba074c7..6e8f2bbe3c 100644 --- a/src/stdio-bridge/stdio-bridge.c +++ b/src/stdio-bridge/stdio-bridge.c @@ -242,8 +242,11 @@ static int run(int argc, char *argv[]) { }; r = ppoll_usec(p, ELEMENTSOF(p), t); - if (r < 0) + if (r < 0) { + if (ERRNO_IS_TRANSIENT(r)) /* don't be bothered by signals, i.e. EINTR */ + continue; return log_error_errno(r, "ppoll() failed: %m"); + } } return 0; From 6976bf5cd614761eb4bd57d39e24f7eca1d6b863 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 22 Nov 2022 12:42:46 +0100 Subject: [PATCH 04/10] varlink: also handle EINTR gracefully when waiting for EIO via ppoll() --- src/shared/varlink.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/src/shared/varlink.c b/src/shared/varlink.c index 4f7ac97689..4d2cfee491 100644 --- a/src/shared/varlink.c +++ b/src/shared/varlink.c @@ -1025,7 +1025,7 @@ static void handle_revents(Varlink *v, int revents) { if ((revents & (POLLOUT|POLLHUP)) == 0) return; - varlink_log(v, "Anynchronous connection completed."); + varlink_log(v, "Asynchronous connection completed."); v->connecting = false; } else { /* Note that we don't care much about POLLIN/POLLOUT here, we'll just try reading and writing @@ -1075,6 +1075,9 @@ int varlink_wait(Varlink *v, usec_t timeout) { return events; r = fd_wait_for_event(fd, events, t); + if (r < 0 && ERRNO_IS_TRANSIENT(r)) /* Treat EINTR as not a timeout, but also nothing happened, and + * the caller gets a chance to call back into us */ + return 1; if (r <= 0) return r; @@ -1161,8 +1164,12 @@ int varlink_flush(Varlink *v) { } r = fd_wait_for_event(v->fd, POLLOUT, USEC_INFINITY); - if (r < 0) + if (r < 0) { + if (ERRNO_IS_TRANSIENT(r)) + continue; + return varlink_log_errno(v, r, "Poll failed on fd: %m"); + } assert(r != 0); From 69858785335afffc51bc03127beb53332c0fb983 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 22 Nov 2022 13:00:48 +0100 Subject: [PATCH 05/10] sd-netlink: handle EINTR from poll() gracefully, as success --- src/libsystemd/sd-netlink/sd-netlink.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/libsystemd/sd-netlink/sd-netlink.c b/src/libsystemd/sd-netlink/sd-netlink.c index feb751a848..b99abae640 100644 --- a/src/libsystemd/sd-netlink/sd-netlink.c +++ b/src/libsystemd/sd-netlink/sd-netlink.c @@ -464,13 +464,18 @@ static int netlink_poll(sd_netlink *nl, bool need_more, usec_t timeout_usec) { } int sd_netlink_wait(sd_netlink *nl, uint64_t timeout_usec) { + int r; + assert_return(nl, -EINVAL); assert_return(!netlink_pid_changed(nl), -ECHILD); if (nl->rqueue_size > 0) return 0; - return netlink_poll(nl, false, timeout_usec); + r = netlink_poll(nl, false, timeout_usec); + if (r < 0 && ERRNO_IS_TRANSIENT(r)) /* Convert EINTR to "something happened" and give user a chance to run some code before calling back into us */ + return 1; + return r; } static int timeout_compare(const void *a, const void *b) { From 6d66a221685c15798e796d9738f73fdb1fdccdb2 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 22 Nov 2022 12:28:19 +0100 Subject: [PATCH 06/10] resolved: handle -EINTR returned from fd_wait_for_event() better We might get signals for various reasons (for example, somebody asking us to reload caches via a signal), hence let's handle this gracefully. --- src/resolve/resolved-manager.c | 20 ++++++++++++++++---- 1 file changed, 16 insertions(+), 4 deletions(-) diff --git a/src/resolve/resolved-manager.c b/src/resolve/resolved-manager.c index f62efa87aa..1c9048670b 100644 --- a/src/resolve/resolved-manager.c +++ b/src/resolve/resolved-manager.c @@ -868,11 +868,14 @@ int manager_recv(Manager *m, int fd, DnsProtocol protocol, DnsPacket **ret) { } static int sendmsg_loop(int fd, struct msghdr *mh, int flags) { + usec_t end; int r; assert(fd >= 0); assert(mh); + end = usec_add(now(CLOCK_MONOTONIC), SEND_TIMEOUT_USEC); + for (;;) { if (sendmsg(fd, mh, flags) >= 0) return 0; @@ -881,20 +884,26 @@ static int sendmsg_loop(int fd, struct msghdr *mh, int flags) { if (errno != EAGAIN) return -errno; - r = fd_wait_for_event(fd, POLLOUT, SEND_TIMEOUT_USEC); - if (r < 0) + r = fd_wait_for_event(fd, POLLOUT, LESS_BY(end, now(CLOCK_MONOTONIC))); + if (r < 0) { + if (ERRNO_IS_TRANSIENT(r)) + continue; return r; + } if (r == 0) return -ETIMEDOUT; } } static int write_loop(int fd, void *message, size_t length) { + usec_t end; int r; assert(fd >= 0); assert(message); + end = usec_add(now(CLOCK_MONOTONIC), SEND_TIMEOUT_USEC); + for (;;) { if (write(fd, message, length) >= 0) return 0; @@ -903,9 +912,12 @@ static int write_loop(int fd, void *message, size_t length) { if (errno != EAGAIN) return -errno; - r = fd_wait_for_event(fd, POLLOUT, SEND_TIMEOUT_USEC); - if (r < 0) + r = fd_wait_for_event(fd, POLLOUT, LESS_BY(end, now(CLOCK_MONOTONIC))); + if (r < 0) { + if (ERRNO_IS_TRANSIENT(r)) + continue; return r; + } if (r == 0) return -ETIMEDOUT; } From f3d9278f38f0a9e03ed29215f27d8ca21c1fa6a1 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 22 Nov 2022 12:55:10 +0100 Subject: [PATCH 07/10] homed: handle EINTR gracefully when waiting for device node --- src/home/homework-luks.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/home/homework-luks.c b/src/home/homework-luks.c index 97fb5a1051..5e1d5bbd65 100644 --- a/src/home/homework-luks.c +++ b/src/home/homework-luks.c @@ -2017,9 +2017,12 @@ static int wait_for_devlink(const char *path) { if (w >= until) return log_error_errno(SYNTHETIC_ERRNO(ETIMEDOUT), "Device link %s still hasn't shown up, giving up.", path); - r = fd_wait_for_event(inotify_fd, POLLIN, usec_sub_unsigned(until, w)); - if (r < 0) + r = fd_wait_for_event(inotify_fd, POLLIN, until - w); + if (r < 0) { + if (ERRNO_IS_TRANSIENT(r)) + continue; return log_error_errno(r, "Failed to watch inotify: %m"); + } (void) flush_fd(inotify_fd); } From 80b780ba178a84b248ecee47eef82358480c9492 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 22 Nov 2022 12:56:38 +0100 Subject: [PATCH 08/10] utmp-wtmp: fix error in case isatty() fails --- src/shared/utmp-wtmp.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/shared/utmp-wtmp.c b/src/shared/utmp-wtmp.c index d2c8473c60..20add0e81b 100644 --- a/src/shared/utmp-wtmp.c +++ b/src/shared/utmp-wtmp.c @@ -292,8 +292,10 @@ static int write_to_terminal(const char *tty, const char *message) { assert(message); fd = open(tty, O_WRONLY|O_NONBLOCK|O_NOCTTY|O_CLOEXEC); - if (fd < 0 || !isatty(fd)) + if (fd < 0) return -errno; + if (!isatty(fd)) + return -ENOTTY; p = message; left = strlen(message); From 22ecfa83123dbfa2322346ac4e25ad2193a3b10c Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 22 Nov 2022 12:56:55 +0100 Subject: [PATCH 09/10] utmp-wtmp: handle EINTR gracefully when waiting to write to tty --- src/shared/utmp-wtmp.c | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/src/shared/utmp-wtmp.c b/src/shared/utmp-wtmp.c index 20add0e81b..37a5bf7990 100644 --- a/src/shared/utmp-wtmp.c +++ b/src/shared/utmp-wtmp.c @@ -12,6 +12,7 @@ #include #include "alloc-util.h" +#include "errno-util.h" #include "fd-util.h" #include "hostname-util.h" #include "io-util.h" @@ -300,7 +301,7 @@ static int write_to_terminal(const char *tty, const char *message) { p = message; left = strlen(message); - end = now(CLOCK_MONOTONIC) + TIMEOUT_USEC; + end = usec_add(now(CLOCK_MONOTONIC), TIMEOUT_USEC); while (left > 0) { ssize_t n; @@ -308,19 +309,21 @@ static int write_to_terminal(const char *tty, const char *message) { int k; t = now(CLOCK_MONOTONIC); - if (t >= end) return -ETIME; k = fd_wait_for_event(fd, POLLOUT, end - t); - if (k < 0) + if (k < 0) { + if (ERRNO_IS_TRANSIENT(k)) + continue; return k; + } if (k == 0) return -ETIME; n = write(fd, p, left); if (n < 0) { - if (errno == EAGAIN) + if (ERRNO_IS_TRANSIENT(errno)) continue; return -errno; From ffbcc8d423671ad2fe827e4823a8032dc1f0a8b3 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 22 Nov 2022 15:23:34 +0100 Subject: [PATCH 10/10] io-util: document EINTR situation a bit --- src/basic/io-util.c | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/src/basic/io-util.c b/src/basic/io-util.c index cdad939aa6..f642beca3a 100644 --- a/src/basic/io-util.c +++ b/src/basic/io-util.c @@ -161,6 +161,21 @@ int ppoll_usec(struct pollfd *fds, size_t nfds, usec_t timeout) { assert(fds || nfds == 0); + /* This is a wrapper around ppoll() that does primarily two things: + * + * ✅ Takes a usec_t instead of a struct timespec + * + * ✅ Guarantees that if an invalid fd is specified we return EBADF (i.e. converts POLLNVAL to + * EBADF). This is done because EBADF is a programming error usually, and hence should bubble up + * as error, and not be eaten up as non-error POLLNVAL event. + * + * ⚠️ ⚠️ ⚠️ Note that this function does not add any special handling for EINTR. Don't forget + * poll()/ppoll() will return with EINTR on any received signal always, there is no automatic + * restarting via SA_RESTART available. Thus, typically you want to handle EINTR not as an error, + * but just as reason to restart things, under the assumption you use a more appropriate mechanism + * to handle signals, such as signalfd() or signal handlers. ⚠️ ⚠️ ⚠️ + */ + if (nfds == 0) return 0; @@ -188,6 +203,9 @@ int fd_wait_for_event(int fd, int event, usec_t timeout) { }; int r; + /* ⚠️ ⚠️ ⚠️ Keep in mind you almost certainly want to handle -EINTR gracefully in the caller, see + * ppoll_usec() above! ⚠️ ⚠️ ⚠️ */ + r = ppoll_usec(&pollfd, 1, timeout); if (r <= 0) return r;