Re: [PATCH v3 1/2] tools: Add adapter tool for services using sd_notify

[Alyssa Ross <hi@alyssa.is>]

[-- Attachment #1: Type: text/plain, Size: 7404 bytes --]

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 | 127 ++++++++++++++++++++++++++++
>  4 files changed, 133 insertions(+)
>
> diff --git a/tools/default.nix b/tools/default.nix
> index f1157f0072f58c2ad9e741ca60bab2ed6507b72d..3634ef8ee892697b1bc7fff259eaccd54fddcd2f 100644
> --- a/tools/default.nix
> +++ b/tools/default.nix
> @@ -74,6 +74,7 @@ stdenv.mkDerivation (finalAttrs: {
>        ./lsvm
>        ./start-vmm
>        ./subprojects
> +      ./sd-notify-adapter

Sort.

>      ] ++ lib.optionals driverSupport [
>        ./xdp-forwarder
>      ]));
> diff --git a/tools/meson.build b/tools/meson.build
> index 17b4c16c07c9c6847306c47fbb7fe54f5a6e8cc5..d679afa64966d7b237f332096281a9bc9ef76e94 100644
> --- a/tools/meson.build
> +++ b/tools/meson.build
> @@ -23,6 +23,7 @@ if get_option('host')
>  
>    subdir('lsvm')
>    subdir('start-vmm')
> +  subdir('sd-notify-adapter')
>  endif
>  
>  if get_option('app')
> 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

Atypical license.

> +# 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..2767dea30b8db69986d9c2a02d6be28d205b3b4b
> --- /dev/null
> +++ b/tools/sd-notify-adapter/sd-notify-adapter.c
> @@ -0,0 +1,127 @@
> +// SPDX-License-Identifier: MIT
> +// SPDX-FileCopyrightText: 2025 Demi Marie Obenour <demiobenour@gmail.com>
> +
> +#define _GNU_SOURCE 1

This should be done in the build system, like how other programs in
Spectrum do it.  (That way we can't forget it needs to be set before any
headers are included.)

> +#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 <sysexits.h>

I used to like sysexits.h too, but the BSD systems they originally came
from now consider them bad practice and discourage them in new code, so
that's why other code in Spectrum doesn't use them, and just uses
EXIT_FAILURE.

> +#include <unistd.h>
> +
> +#define ARRAY_SIZE(s) (sizeof(s)/sizeof(s[0]))
> +
> +static bool ready;
> +
> +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 data = recv(socket_fd, msg->iov_base, msg->iov_len,
> +	                    MSG_DONTWAIT | MSG_TRUNC | MSG_PEEK);

Why MSG_DONTWAIT?  We know there's data waiting for us.

I find "data" to be quite a confusing name for a size.

> +	if (data == -1) {
> +		if (errno == EINTR) {
> +			return; // signal caught
> +		}
> +		if (errno == EAGAIN || errno == EWOULDBLOCK) {
> +			return; // spurious wakeup
> +		}
> +		err(EX_OSERR, "recv from notification socket");
> +	}
> +	assert(data >= 0 && data <= INT_MAX);

Why do we care about the upper range?  We cast it to size_t after this,
not int.

> +	size_t size = (size_t)data;
> +	if (size == 0)
> +		return; // avoid arithmetic on NULL pointer
> +	if (size > msg->iov_len) {
> +		char *b = (size == 0 ? malloc(size) : realloc(msg->iov_base, size));

We already know size != 0 at this point.

> +		if (b == NULL) {
> +			err(EX_OSERR, "allocation failure");
> +		}
> +		msg->iov_base = b;

We could just msg->iov_base = realloc(…), without the b variable.

> +		msg->iov_len = size;
> +	}
> +	data = recv(socket_fd, msg->iov_base, msg->iov_len,
> +	            MSG_CMSG_CLOEXEC | MSG_DONTWAIT | MSG_TRUNC);
> +	if (data < 0) {
> +		if (errno == EINTR || errno == EAGAIN || errno == EWOULDBLOCK)
> +			return;
> +		err(EX_OSERR, "recv from notification socket");
> +	}

EAGAIN or EWOULDBLOCK would be quite unexpected.

> +	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) {
> +			data = write(notification_fd, "\n", 1);
> +			if (data != 1) {
> +				err(EX_OSERR, "writing to notification descriptor");
> +			}
> +			exit(0);
> +		}
> +	}
> +}
> +
> +int main(int argc, char **argv [[gnu::unused]]) {
> +	if (argc != 1) {
> +		errx(EX_USAGE, "stdin is listening socket, stdout is notification pipe");
> +	}

This is C23 — you can just write int main(int argc, char **).

> +	// Main event loop.
> +	struct iovec v = {
> +		.iov_base = NULL,
> +		.iov_len = 0,
> +	};
> +	for (;;) {
> +		struct pollfd p[] = {
> +			{
> +				.fd = socket_fd,
> +				.events = POLLIN,
> +				.revents = 0,
> +			},
> +			{
> +				.fd = notification_fd,
> +				.events = 0,
> +				.revents = 0,
> +			},
> +		};
> +		int r = poll(p, 2, -1);

Might as well use ARRAY_SIZE here, given we have it.

> +		if (r < 0) {
> +			if (errno == EINTR)
> +				continue;
> +			err(EX_OSERR, "poll");
> +		}
> +		if (p[0].revents) {
> +			if (p[0].revents & POLLERR)
> +				errx(EX_OSERR, "unexpected POLLERR");
> +			if (p[0].revents & POLLIN)
> +				process_notification(&v);
> +			break;

break?  So when we get a non-READY message, we just exit?

> +		}
> +		if (p[1].revents) {
> +			if (ready) {
> +				// Normal exit
> +				return 0;
> +			}

ready is never written, so this never happens.

> +			errx(EX_PROTOCOL, "s6 closed its pipe before the child was ready");
> +		}
> +	}
> +}
>
> -- 
> 2.51.0

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 227 bytes --]