From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from atuin.qyliss.net (localhost [IPv6:::1]) by atuin.qyliss.net (Postfix) with ESMTP id 5038518070; Thu, 25 Sep 2025 10:29:38 +0000 (UTC) Received: by atuin.qyliss.net (Postfix, from userid 993) id 786711809A; Thu, 25 Sep 2025 10:29:35 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 4.0.1 (2024-03-26) on atuin.qyliss.net X-Spam-Level: X-Spam-Status: No, score=-0.8 required=3.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,DMARC_MISSING,RCVD_IN_DNSWL_LOW,SPF_HELO_PASS autolearn=unavailable autolearn_force=no version=4.0.1 Received: from fout-b8-smtp.messagingengine.com (fout-b8-smtp.messagingengine.com [202.12.124.151]) by atuin.qyliss.net (Postfix) with ESMTPS id 122E918097 for ; Thu, 25 Sep 2025 10:29:34 +0000 (UTC) Received: from phl-compute-06.internal (phl-compute-06.internal [10.202.2.46]) by mailfout.stl.internal (Postfix) with ESMTP id DD4B71D00103; Thu, 25 Sep 2025 06:29:32 -0400 (EDT) Received: from phl-mailfrontend-01 ([10.202.2.162]) by phl-compute-06.internal (MEProxy); Thu, 25 Sep 2025 06:29:32 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=alyssa.is; h=cc :cc:content-type:content-type:date:date:from:from:in-reply-to :in-reply-to:message-id:mime-version:references:reply-to:subject :subject:to:to; s=fm3; t=1758796172; x=1758882572; bh=rKy8DHJpuF Vjuu2a1bY7Mw2k9Azmtbgk3rM67GhltAI=; b=QVvNOAXanZe6XPd0XPJyFyFo3d emH9yxS787Q/+5qojSog4VNGfEx3mIS0xe8fNAZpVmolv66/CPwLqtD0FsGCAMK3 v7JitEfqcFteYohwpiIc/DOCvgrsmCAhkZANcjMcZFJu/v61+irHibM6I24EYSi8 p3LAFZnjG1UcGKLT4xwKkLJrGaLebykkxf/vMohm6+jQxLyc/X/MwDRxiwAAVBew jOtx2Insrzo8ZyeCClGMjdAwmHZHZ6GoNTRDD1NhVulNeDT1Vjtz74g40suy+KJm 9R9sLRQnWLb6ri/3GWKUL97WnYVevrgtpRFyQF+qYv/Bd8vHJ7PGk1QotUfQ== DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d= messagingengine.com; h=cc:cc:content-type:content-type:date:date :feedback-id:feedback-id:from:from:in-reply-to:in-reply-to :message-id:mime-version:references:reply-to:subject:subject:to :to:x-me-proxy:x-me-sender:x-me-sender:x-sasl-enc; s=fm1; t= 1758796172; x=1758882572; bh=rKy8DHJpuFVjuu2a1bY7Mw2k9Azmtbgk3rM 67GhltAI=; b=VN05Ku1hGjhbjC7vwOiTx6kLsCgBHlCbwf0mZcvlM1Z1J1gT2vU oD7D8/nqrzvP26LmgZry11qajE144+Ads6p1jHxi2VpvnUeY5yjMU6sxzfeaspKY dW0Kn2HlLFsMDfZV1NCojXswa1tj1y+9wZzzsAD7NhwWAAFXrBXS5xsO53npsTrC S9LH1YhPYxeE/Kx7XAggsGAc9FPVh8k6mxW8Zu446nZHUpLJqXXg4QLGO6KB2jhw lWoMBtnJSWDcnRFIpr9nfe2LDACZx9sPeW0RQuzw9INaR3kBN8Rk73Vw4X79IreV EzMTwIwMz3UVh3qEgB7NqIjoM/bjvLB9aLg== X-ME-Sender: X-ME-Received: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgeeffedrtdeggdeiiedvhecutefuodetggdotefrod ftvfcurfhrohhfihhlvgemucfhrghsthforghilhdpuffrtefokffrpgfnqfghnecuuegr ihhlohhuthemuceftddtnecusecvtfgvtghiphhivghnthhsucdlqddutddtmdenucfjug hrpefhvfevufgjfhffkfggtgesghdtreertddttdenucfhrhhomheptehlhihsshgrucft ohhsshcuoehhihesrghlhihsshgrrdhisheqnecuggftrfgrthhtvghrnhepieduffeuie elgfetgfdttddtkeekheekgfehkedufeevteegfeeiffetvdetueevnecuvehluhhsthgv rhfuihiivgeptdenucfrrghrrghmpehmrghilhhfrhhomhephhhisegrlhihshhsrgdrih hspdhnsggprhgtphhtthhopedvpdhmohguvgepshhmthhpohhuthdprhgtphhtthhopegu vghmihhosggvnhhouhhrsehgmhgrihhlrdgtohhmpdhrtghpthhtohepuggvvhgvlhessh hpvggtthhruhhmqdhoshdrohhrgh X-ME-Proxy: Feedback-ID: i12284293:Fastmail Received: by mail.messagingengine.com (Postfix) with ESMTPA; Thu, 25 Sep 2025 06:29:32 -0400 (EDT) Received: by mbp.qyliss.net (Postfix, from userid 1000) id 7867424D2EDE; Thu, 25 Sep 2025 12:29:21 +0200 (CEST) From: Alyssa Ross To: Demi Marie Obenour Subject: Re: [PATCH v2 1/3] tools: Add adapter tool for services using sd_notify In-Reply-To: <20250924-udev-v2-1-6089de521b3b@gmail.com> References: <20250924-udev-v2-0-6089de521b3b@gmail.com> <20250924-udev-v2-1-6089de521b3b@gmail.com> Date: Thu, 25 Sep 2025 12:29:20 +0200 Message-ID: <87348addvj.fsf@alyssa.is> MIME-Version: 1.0 Content-Type: multipart/signed; boundary="=-=-="; micalg=pgp-sha512; protocol="application/pgp-signature" Message-ID-Hash: VCJM3EDZROHSSMXRFHKRJ4PCL4UVTGQN X-Message-ID-Hash: VCJM3EDZROHSSMXRFHKRJ4PCL4UVTGQN X-MailFrom: hi@alyssa.is X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; emergency; loop; banned-address; member-moderation; header-match-devel.spectrum-os.org-0; header-match-devel.spectrum-os.org-1; header-match-devel.spectrum-os.org-2; header-match-devel.spectrum-os.org-3; header-match-devel.spectrum-os.org-4; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header CC: Spectrum OS Development X-Mailman-Version: 3.3.9 Precedence: list List-Id: Patches and low-level development discussion Archived-At: List-Archive: List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: --=-=-= Content-Type: text/plain 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! > > 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? > 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. > + > +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? > + 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? > + 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. > + 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. > + // 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? > + > + // 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? > + default: > + assert(0); // Not reached > + } > + } > + } > +} --=-=-= Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iHUEARYKAB0WIQRV/neXydHjZma5XLJbRZGEIw/wogUCaNUZgAAKCRBbRZGEIw/w ov9zAQDFdqm/OBXtJKGo1fNiehuYUM5kYuZvU7SEfhiyc0TGHgEAoBrrFux7u6Wz VnMUwr5DIOV4nhXnAW77QlfhloA5jw0= =HEbo -----END PGP SIGNATURE----- --=-=-=--