Re: [PATCH v2 1/3] tools: Add adapter tool for services using sd_notify

[Demi Marie Obenour <demiobenour@gmail.com>]

[-- Attachment #1.1.1: Type: text/plain, Size: 11489 bytes --]

On 9/25/25 06:29, Alyssa Ross wrote:
> Demi Marie Obenour <demiobenour@gmail.com> 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 <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 | 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 <demiobenour@gmail.com>
>> +
>> +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 <demiobenour@gmail.com>
>> +// 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 <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 <sys/epoll.h>
>> +#include <sys/socket.h>
>> +#include <sys/stat.h>
>> +#include <sys/un.h>
>> +#include <sysexits.h>
>> +#include <unistd.h>
>> +
>> +#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)

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 7253 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]