[agent] add generic readiness notification fd support - #3478
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces support for generic readiness notification for fd-based supervisors (such as s6 or dinit) in src/agent/application.cpp. It reads the OTBR_NOTIFY_FD environment variable, parses it to obtain a file descriptor, writes a newline character to signal readiness, and then closes the file descriptor. Necessary standard headers are also included. There are no review comments, and I have no feedback to provide.
88380eb to
bb60222
Compare
Allow fd-based service supervisors (s6, dinit, ...) to be notified of agent readiness: when built with the OTBR_NOTIFY_FD option (default ON, mirroring OTBR_NOTIFY_UPSTART) and the OTBR_NOTIFY_FD environment variable is set, write a newline to that file descriptor once the agent is ready, at the same point where systemd/Upstart readiness is signalled. See https://skarnet.org/software/s6/notifywhenup.html for the protocol.
| option(OTBR_NOTIFY_FD "Support readiness notification via file descriptor (s6, dinit, ...)." ON) | ||
| if(OTBR_NOTIFY_FD) | ||
| target_compile_definitions(otbr-config INTERFACE OTBR_ENABLE_NOTIFY_FD=1) | ||
| endif() |
There was a problem hiding this comment.
If we want to avoid too many compile time options, this change could also be part of the default build since the code only becomes active if OTBR_NOTIFY_FD is set.
There was a problem hiding this comment.
Makes sense to me. I agree with simplifying the build config.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3478 +/- ##
===========================================
- Coverage 55.77% 44.45% -11.32%
===========================================
Files 87 144 +57
Lines 6890 17390 +10500
Branches 0 1433 +1433
===========================================
+ Hits 3843 7731 +3888
- Misses 3047 9109 +6062
- Partials 0 550 +550 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| #include <errno.h> | ||
| #include <stdlib.h> | ||
| #include <string.h> | ||
| #include <unistd.h> |
There was a problem hiding this comment.
nit: can we move this above HAVE_LIBSYSTEMD above?
| char *end = nullptr; | ||
| long notifyFd = strtol(notifyFdEnv, &end, 10); | ||
|
|
||
| if (end != notifyFdEnv && *end == '\0' && notifyFd >= 0) | ||
| { | ||
| ssize_t rval; |
There was a problem hiding this comment.
Suggest adding bounds checking and errno validation for strtol, and ensure the file descriptor does not point to standard I/O streams (0, 1, or 2).
| char *end = nullptr; | |
| long notifyFd = strtol(notifyFdEnv, &end, 10); | |
| if (end != notifyFdEnv && *end == '\0' && notifyFd >= 0) | |
| { | |
| ssize_t rval; | |
| errno = 0; | |
| long notifyFdLong = strtol(notifyFdEnv, &end, 10); | |
| if (end != notifyFdEnv && *end == '\0' && errno == 0 && notifyFdLong >= 3 && notifyFdLong <= INT_MAX) | |
| { | |
| int notifyFd = static_cast<int>(notifyFdLong); | |
| ssize_t rval; | |
| // ... use `notifyFd` directly without repeated static_cast |
| long notifyFd = strtol(notifyFdEnv, &end, 10); | ||
|
|
||
| if (end != notifyFdEnv && *end == '\0' && notifyFd >= 0) | ||
| { | ||
| ssize_t rval; |
There was a problem hiding this comment.
Suggest adding bounds checking and errno validation for strtol, and ensure the file descriptor does not point to standard I/O streams (0, 1, or 2).
| long notifyFd = strtol(notifyFdEnv, &end, 10); | |
| if (end != notifyFdEnv && *end == '\0' && notifyFd >= 0) | |
| { | |
| ssize_t rval; | |
| errno = 0; | |
| long notifyFdLong = strtol(notifyFdEnv, &end, 10); | |
| if (end != notifyFdEnv && *end == '\0' && errno == 0 && notifyFdLong >= 3 && notifyFdLong <= INT_MAX) | |
| { | |
| int notifyFd = static_cast<int>(notifyFdLong); | |
| ssize_t rval; | |
| // ... use `notifyFd` directly without repeated static_cast |
| } | ||
| #endif | ||
|
|
||
| #if OTBR_ENABLE_NOTIFY_FD |
There was a problem hiding this comment.
Should we move this after signal(SIGPIPE, SIG_IGN) below?
| otbrLogWarning("Failed to notify readiness on file descriptor %ld: %s", notifyFd, strerror(errno)); | ||
| } | ||
|
|
||
| close(static_cast<int>(notifyFd)); |
There was a problem hiding this comment.
Application::Run() can be executed multiple times within the same process lifecycle (e.g., after a software reset handled via otPlatReset).
When notifyFd is closed during the initial run, the environment variable OTBR_NOTIFY_FD remains set. On a subsequent re-run of Application::Run(), the code will attempt to write to and close that file descriptor number again. By that time, the OS may have reassigned that file descriptor number to a new socket or DBus connection, causing data corruption and closing an active resource.
Should we call unsetenv() after closing the notifyFd?
2 participants
Motivation
otbr-agent already notifies systemd (via
sd_notify(READY=1)) and Upstart (viaSIGSTOP) when it is ready. Other service supervisors — s6, dinit, and similar — use a simpler, fd-based readiness protocol: the daemon writes a newline to a file descriptor provided by the supervisor (see https://skarnet.org/software/s6/notifywhenup.html).This PR adds support for that protocol, mirroring the existing Upstart integration: a new CMake option
OTBR_NOTIFY_FD(defaultON, likeOTBR_NOTIFY_UPSTART) compiles the support in, and at runtime the agent writes a newline to the file descriptor given in theOTBR_NOTIFY_FDenvironment variable — at the same point inApplication::Run()where systemd/Upstart readiness is signalled. Behavior is unchanged when the variable is not set, and no new dependencies are needed.This allows supervising otbr-agent under s6/s6-overlay (e.g. in containers) with exact, event-driven readiness instead of polling for the control socket: dependent services (
ot-ctlbased configuration, otbr-web) can be started exactly when the agent is initialized.Testing
ON(default) andOFF(code compiled out).Notify readiness on file descriptor 3., and dependent services start afterwards.OTBR_NOTIFY_FDis unset, and that an invalid value only logs a warning.🤖 Generated with Claude Code