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

[Alyssa Ross <hi@alyssa.is>]

[-- Attachment #1: Type: text/plain, Size: 5661 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 | 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.

> 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.

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

> +	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?

> +		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.

> +	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. :)

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