On 9/25/25 06:29, Alyssa Ross wrote: > Demi Marie Obenour writes: > >> This adapts programs using sd_notify for use with s6 readiness >> notification. >> >> I chose to use Linux-specific epoll(7). It makes the code simpler and >> more readable. Also, stdin and stdout are hardcoded. This is in the >> interest of simplicity. > > I personally find poll to be more readable when working with a fixed set > of descriptors, but it's up to you! poll can actually fail with ENOMEM, which is one of the reasons I used epoll. >> >> Signed-off-by: Demi Marie Obenour >> --- >> systemd readiness notification has two >> strict advantages over the s6 version: >> >> 1. It allows reliable reloading. >> 2. It allows providing a status message that the service manager >> can show in status output. >> >> s6 would actually benefit from both of these features. >> --- >> Changes since v1: >> >> - Hard-code file descriptors. >> - Run wrapper as background process. >> - Massively reduce code size. >> - Use // instead of /* */ for comments. >> - Check that the notification FD is a pipe and that the listening socket >> is a socket. >> - Rely on s6-ipc-socketbinder to create the listening socket. >> - Do not unlink the listening socket. >> --- >> tools/default.nix | 1 + >> tools/meson.build | 1 + >> tools/sd-notify-adapter/meson.build | 4 + >> tools/sd-notify-adapter/sd-notify-adapter.c | 206 ++++++++++++++++++++++++++++ >> 4 files changed, 212 insertions(+) >> >> diff --git a/tools/sd-notify-adapter/meson.build b/tools/sd-notify-adapter/meson.build >> new file mode 100644 >> index 0000000000000000000000000000000000000000..6032a3a7704d49cae0655b43d0189444d3b15e4d >> --- /dev/null >> +++ b/tools/sd-notify-adapter/meson.build >> @@ -0,0 +1,4 @@ >> +# SPDX-License-Identifier: ISC >> +# SPDX-FileCopyrightText: 2025 Demi Marie Obenour >> + >> +executable('sd-notify-adapter', 'sd-notify-adapter.c', install: true) > > Why the non-standard license? Mistake :) >> diff --git a/tools/sd-notify-adapter/sd-notify-adapter.c b/tools/sd-notify-adapter/sd-notify-adapter.c >> new file mode 100644 >> index 0000000000000000000000000000000000000000..661e3f41e57dae97a5cfaeb3a7088b0c67235563 >> --- /dev/null >> +++ b/tools/sd-notify-adapter/sd-notify-adapter.c >> @@ -0,0 +1,206 @@ >> +// SPDX-License-Identifier: MIT >> +// SPDX-FileCopyrightText: 2025 Demi Marie Obenour >> +// check_posix and check_posix_bool are based on playpen.c, which has >> +// the license: >> +// >> +// Copyright 2014 Daniel Micay >> +// >> +// Permission is hereby granted, free of charge, to any person obtaining a >> +// copy of this software and associated documentation files (the >> +// "Software"), to deal in the Software without restriction, including >> +// without limitation the rights to use, copy, modify, merge, publish, >> +// distribute, sublicense, and/or sell copies of the Software, and to >> +// permit persons to whom the Software is furnished to do so, subject to >> +// the following conditions: >> +// >> +// The above copyright notice and this permission notice shall be included >> +// in all copies or substantial portions of the Software. >> +// >> +// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS >> +// OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF >> +// MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. >> +// IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY >> +// CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, >> +// TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE >> +// SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE. >> + >> +#define _GNU_SOURCE 1 >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> + >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> + >> +#define ARRAY_SIZE(s) (sizeof(s)/sizeof(s[0])) >> + >> +// TODO: does this need to have credit given to Daniel Micay? >> +[[gnu::format(printf, 2, 3), gnu::warn_unused_result]] >> +static intmax_t check_posix(intmax_t arg, const char *fmt, ...) { >> + if (arg >= 0) >> + return arg; >> + assert(arg == -1); >> + va_list a; >> + va_start(a, fmt); >> + verr(EX_OSERR, fmt, a); >> +} >> + >> +#define check_posix(arg, message, ...) \ >> + ((__typeof__(arg))check_posix(arg, message, ## __VA_ARGS__)) >> + >> +// And same here >> +[[gnu::format(printf, 2, 3)]] >> +static void check_posix_bool(intmax_t arg, const char *fmt, ...) { >> + if (arg != -1) { >> + assert(arg == 0); >> + return; >> + } >> + va_list a; >> + va_start(a, fmt); >> + verr(EX_OSERR, fmt, a); >> + va_end(a); // Not reached >> +} > > I would prefer that we do manual error checks in the style of other C > code in Spectrum. Then we don't have to worry about licensing of these > helpers, and also don't have the problem of how to share them between > multiple files later on. It's likely that readers are also going to be > more familiar with simple error checks. Will change. >> + >> +static bool ready; >> + >> +enum { >> + socket_fd, >> + notification_fd, >> +}; >> + >> +static void >> +process_notification(struct iovec *const msg, const char *const initial_buffer) { >> + ssize_t data = recv(socket_fd, msg->iov_base, msg->iov_len, >> + MSG_DONTWAIT | MSG_TRUNC | MSG_PEEK); >> + if (data == -1) { >> + if (errno == EINTR) { >> + return; // signal caught >> + } >> + if (errno == EAGAIN || errno == EWOULDBLOCK) { >> + return; // spurious wakeup >> + } >> + } >> + size_t size = (size_t)check_posix(data, "recv"); >> + if (size > (size_t)INT_MAX) { >> + // cannot happen on Linux, don't bother implementing >> + size = (size_t)INT_MAX; >> + } > > If it can't happen, why do we branch on it? G >> + if (size > msg->iov_len) { >> + char *b = (msg->iov_base == initial_buffer) ? >> + malloc(size) : realloc(msg->iov_base, size); >> + if (b != NULL) { >> + msg->iov_base = b; >> + msg->iov_len = size; >> + } >> + } > > Wouldn't it be simpler to pass an empty iov, then allocate whatever size > we need here, than to have to handle sometimes having a stack-allocated > buffer and sometimes not? It would >> + size = (size_t)check_posix(recv(socket_fd, msg->iov_base, msg->iov_len, >> + MSG_CMSG_CLOEXEC | MSG_DONTWAIT | MSG_TRUNC), >> + "recv"); >> + const char *cursor = msg->iov_base; >> + const char *const end = cursor + size; >> + for (char *next; cursor != NULL; cursor = (next == NULL ? NULL : next + 1)) { >> + next = memchr(cursor, '\n', (size_t)(end - cursor)); >> + size_t message_size = (size_t)((next == NULL ? end : next) - cursor); >> + >> + // TODO: avoid repeating sizeof(string) > > Yeah, let's maybe pull the message we're looking for out into a constant. I agree. >> + if (message_size == sizeof("READY=1") - 1 && >> + memcmp(cursor, "READY=1", sizeof("READY=1") - 1) == 0) { >> + if (check_posix(write(notification_fd, "\n", 1), "write") != 1) >> + assert(0); >> + exit(0); >> + } >> + } >> +} >> + >> +int main(int argc, char **argv [[gnu::unused]]) { >> + if (argc != 1) { >> + errx(EX_USAGE, "stdin is listening socket, stdout is notification pipe"); >> + } >> + struct stat info; >> + check_posix_bool(fstat(notification_fd, &info), "fstat"); >> + if (!S_ISFIFO(info.st_mode)) { >> + errx(EX_USAGE, "notification descriptor is not a pipe"); >> + } >> + int value; >> + socklen_t len = sizeof(value); >> + int status = getsockopt(socket_fd, SOL_SOCKET, SO_DOMAIN, &value, &len); >> + if (status == -1 && errno == ENOTSOCK) { >> + errx(EX_USAGE, "socket fd is not a socket"); >> + } >> + check_posix_bool(status, "getsockopt"); >> + assert(len == sizeof(value)); >> + if (value != AF_UNIX) { >> + errx(EX_USAGE, "socket fd must be AF_UNIX socket"); >> + } >> + check_posix_bool(getsockopt(socket_fd, SOL_SOCKET, SO_TYPE, &value, &len), >> + "getsockopt"); >> + assert(len == sizeof(value)); >> + if (value != SOCK_DGRAM) { >> + errx(EX_USAGE, "socket must be datagram socket"); >> + } >> + > > I think these checks are overly defensive. It's going to be very > difficult to use this program wrong given it's always going to be used > in the same way in run scripts. I'd rather have less code, which will > make it easier to understand what the actual functionality of the > program is. Will fix. >> + // Ignore SIGPIPE. >> + struct sigaction act = { }; >> + act.sa_handler = SIG_IGN; >> + check_posix_bool(sigaction(SIGPIPE, &act, NULL), "sigaction(SIGPIPE)"); > > Wouldn't SIGPIPE be useful here? Isn't the default behavior of exiting > on SIGPIPE exactly what we'd want to do? Good point. I should ignore SIGPIPE in the run script, though. systemd does this by default. >> + >> + // Open file descriptors. >> + int epoll_fd = check_posix(epoll_create1(EPOLL_CLOEXEC), "epoll_create1"); >> + if (epoll_fd < 3) { >> + errx(EX_USAGE, "Invoked with file descriptor 0, 1, or 2 closed"); >> + } >> + struct epoll_event event = { .events = EPOLLIN, .data.u64 = socket_fd }; >> + check_posix_bool(epoll_ctl(epoll_fd, EPOLL_CTL_ADD, socket_fd, &event), >> + "epoll_ctl"); >> + event = (struct epoll_event) { .events = 0, .data.u64 = notification_fd }; >> + check_posix_bool(epoll_ctl(epoll_fd, EPOLL_CTL_ADD, notification_fd, &event), >> + "epoll_ctl"); >> + >> + // Main event loop. >> + char buf[sizeof("READY=1\n") - 1]; >> + struct iovec v = { >> + .iov_base = buf, >> + .iov_len = sizeof(buf), >> + }; >> + for (;;) { >> + struct epoll_event out_event[2] = {}; >> + int epoll_wait_result = >> + check_posix(epoll_wait(epoll_fd, out_event, ARRAY_SIZE(out_event), -1), >> + "epoll_wait"); >> + for (int i = 0; i < epoll_wait_result; ++i) { >> + switch (out_event[i].data.u64) { >> + case socket_fd: >> + if (out_event[i].events != EPOLLIN) { >> + errx(EX_PROTOCOL, "Unexpected event from epoll() on notification socket"); >> + } >> + process_notification(&v, buf); >> + break; >> + case notification_fd: >> + if (out_event[i].events != EPOLLERR) { >> + errx(EX_SOFTWARE, "Unexpected event from epoll() on supervison pipe"); >> + } >> + if (ready) { >> + // Normal exit >> + return 0; >> + } >> + errx(EX_PROTOCOL, "s6 closed its pipe before the child was ready"); >> + break; > > Why do we need to poll on notification_fd at all? If it closes early, > we get a write fail or a SIGPIPE, and we exit with a failure or are > killed, which is fine, right? If it closes early, but systemd-udevd never sends READY=1, the program uselessly hangs around. -- Sincerely, Demi Marie Obenour (she/her/hers)