On 10/28/25 11:38, Alyssa Ross wrote:
Demi Marie Obenour <demiobenour@gmail.com> writes:
This adapts programs using sd_notify for use with s6 readiness notification. stdin and stdout are hard-coded for simplicity.
Signed-off-by: Demi Marie Obenour <demiobenour@gmail.com> --- 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 | 114 ++++++++++++++++++++++++++++ 4 files changed, 120 insertions(+)
Looks correct, so just some convention/readability things. If you're happy with all my comments I can just change them all myself if you prefer not to send a new version of the patch — up to you.
See below. I think the EAGAIN/EWOULDBLOCK checks and the MSG_DONTWAIT are good practices in general. You are correct that in this particular case they aren't needed, at least unless poll() can produce spurious wakeups. That said, if you prefer to change them feel free to do so. Making the changes on commit would be great. Saving a round of review is always a good thing.
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 <demiobenour@gmail.com> + +executable('sd-notify-adapter', 'sd-notify-adapter.c', install: true) 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..10f4e05eb602491540a792c7fb5620d66d5bb989 --- /dev/null +++ b/tools/sd-notify-adapter/sd-notify-adapter.c @@ -0,0 +1,114 @@ +// SPDX-License-Identifier: MIT +// SPDX-FileCopyrightText: 2025 Demi Marie Obenour <demiobenour@gmail.com> + +#define _GNU_SOURCE 1
Like I said last time, this should be set by the build system.
Good point.
+#include <assert.h> +#include <errno.h> +#include <limits.h> +#include <signal.h> +#include <stdarg.h> +#include <stddef.h> +#include <stdint.h> +#include <stdio.h> +#include <stdlib.h> +#include <string.h> + +#include <err.h> +#include <fcntl.h> +#include <poll.h> +#include <sys/socket.h> +#include <sys/stat.h> +#include <sys/un.h> +#include <unistd.h> + +#define ARRAY_SIZE(s) (sizeof(s)/sizeof(s[0])) + +enum { + socket_fd, + notification_fd, +}; + +#define READY "READY=1" +#define READY_SIZE (sizeof(READY) - 1) + +static void process_notification(struct iovec *const msg) +{ + ssize_t first_recv_size = recv(socket_fd, msg->iov_base, msg->iov_len, + MSG_DONTWAIT | MSG_TRUNC | MSG_PEEK);
I guess it just doesn't matter either way, but my question about why MSG_DONTWAIT from last time wasn't answered either.
It's good practice to use MSG_DONTWAIT whenever it makes sense to do so. That avoids relying on sockets being in blocking or non-blocking mode. Setting the blocking mode changes the open file description, which means it can affect other processes too. So no-op in this case, but good idea in general.
+ if (first_recv_size == -1) { + if (errno == EINTR) + return; // signal caught + if (errno == EAGAIN || errno == EWOULDBLOCK) + return; // spurious wakeup
The check for these from the second recv was removed. Should this check also be removed? Is returning EAGAIN or EWOULDBLOCK here a valid thing for the kernel to do?
In this particular case it probably isn't needed. However, many other cases *do* need the check: 1. Edge-triggered event sources. 2. Processing messages until no more are available. 3. Event sources that can produce spurious wakeups.
+ err(EXIT_FAILURE, "recv from notification socket"); + } + assert(first_recv_size >= 0);
Worth even checking? Would be a serious contract violation that would break all sorts of things.
Yeah, and this comes from the kernel, not from userspace. So it can't even happen as a result of a bug in this code.
+ size_t size = (size_t)first_recv_size; + if (size == 0) + return; // avoid arithmetic on NULL pointer + if (size > msg->iov_len) { + msg->iov_base = realloc(msg->iov_base, size); + if (msg->iov_base == NULL) + err(EXIT_FAILURE, "allocation failure"); + msg->iov_len = size; + } + ssize_t second_recv_size = recv(socket_fd, msg->iov_base, msg->iov_len, + MSG_CMSG_CLOEXEC | MSG_TRUNC); + if (second_recv_size == -1) { + if (errno == EINTR) + return; + err(EXIT_FAILURE, "recv from notification socket"); + } + assert(first_recv_size == second_recv_size); + for (char *next, *cursor = msg->iov_base, *end = cursor + size; + 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); + if (message_size == READY_SIZE && + memcmp(cursor, READY, READY_SIZE) == 0) { + ssize_t write_size = write(notification_fd, "\n", 1); + if (write_size != 1) + err(EXIT_FAILURE, "writing to notification descriptor"); + exit(0); + } + } +} + +int main(int argc, char **) +{ + if (argc != 1) + errx(EXIT_FAILURE, "stdin is listening socket, stdout is notification pipe"); + // Main event loop. + struct iovec v = { + .iov_base = NULL, + .iov_len = 0, + };
It might be clearer for this to be a static, rather than a variable in main that's only ever used by a function it calls. It took me a while to figure out that it's being reused between calls (for realloc). And then you don't need the initializer either. :)
That is a good idea. I normally dislike global variables, but the disadvantages are much less severe here. This program is single-threaded, and this code will never be used in a library. -- Sincerely, Demi Marie Obenour (she/her/hers)