Allow policy daemon to listen on Unix domain socket and systemd socket activation#3677
Allow policy daemon to listen on Unix domain socket and systemd socket activation#3677ntninja wants to merge 4 commits into
Conversation
Codecov Report❌ Patch coverage is ❌ Your patch check has failed because the patch coverage (25.00%) is below the target coverage (70.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## master #3677 +/- ##
==========================================
- Coverage 83.87% 83.73% -0.14%
==========================================
Files 320 320
Lines 15775 15791 +16
Branches 2091 2095 +4
==========================================
- Hits 13231 13223 -8
- Misses 1843 1864 +21
- Partials 701 704 +3 🚀 New features to boost your workflow:
|
| coro = asyncio.start_server( | ||
| core.new_connection, options["host"], options["port"] | ||
| ) | ||
| if os.environ.get("LISTEN_PID", "") == str(os.getpid()) and os.environ.get("LISTEN_FDS", "").isnumeric(): |
There was a problem hiding this comment.
Why do you rely on those environment variables? Could you use command line arguments instead?
There was a problem hiding this comment.
And actually, could you please explain what those variables mean?
There was a problem hiding this comment.
That is a short implementation of systemd socket activation in Python
For a background see:
- https://0pointer.de/blog/projects/socket-activation.html (developer tutorial)
- https://www.man7.org/linux/man-pages/man3/sd_listen_fds.3.html (equivalent function in
libsystemd)
sd_listen_fds is defined as:
#define SD_LISTEN_FDS_START 3
int sd_listen_fds(int unset_environment) {
int r, fd;
const char *e;
char *p = NULL;
unsigned long l;
if (!(e = getenv("LISTEN_PID"))) {
r = 0;
goto finish;
}
errno = 0;
l = strtoul(e, &p, 10);
if (errno != 0) {
r = -errno;
goto finish;
}
if (!p || *p || l <= 0) {
r = -EINVAL;
goto finish;
}
/* Is this for us? */
if (getpid() != (pid_t) l) {
r = 0;
goto finish;
}
if (!(e = getenv("LISTEN_FDS"))) {
r = 0;
goto finish;
}
errno = 0;
l = strtoul(e, &p, 10);
if (errno != 0) {
r = -errno;
goto finish;
}
if (!p || *p) {
r = -EINVAL;
goto finish;
}
for (fd = SD_LISTEN_FDS_START; fd < SD_LISTEN_FDS_START + (int) l; fd ++) {
int flags;
if ((flags = fcntl(fd, F_GETFD)) < 0) {
r = -errno;
goto finish;
}
if (flags & FD_CLOEXEC)
continue;
if (fcntl(fd, F_SETFD, flags | FD_CLOEXEC) < 0) {
r = -errno;
goto finish;
}
}
r = (int) l;
finish:
if (unset_environment) {
unsetenv("LISTEN_PID");
unsetenv("LISTEN_FDS");
}
return r;
}The code in question reimplements this one function in Python, taking care to:
- Ensure the listen FDs are intended for our process, rather than the parent (safety check)
- Not inherit the listen FDs to child processes after exec (O_CLOEXEC)
- Unset the environment variables after use
The environment variables mean:
LISTEN_PID: The PID of the process expected to perform socket activationLISTEN_FDS: The number of file descriptors inherited to the target process (starting at 3)
There was a problem hiding this comment.
Alternatives to implementing this here include the following Python libraries:
systemd-socketserver– Basically what is implemented here (pure Python)systemd-python– Offical libsystemd Python bindings (C extension module – requires C compiler or distribution packages)cysystemd– Unofficial libsystemd Python bindings using Cython (distributed as statically linked binary wheels)
There was a problem hiding this comment.
Ok I see. Could you cover this with some tests?
There was a problem hiding this comment.
@tonioo: So inline implementation is fine, then? (Just checking to make sure.)
There was a problem hiding this comment.
Yes, it will be easier to maintain
|
We don’t have much time unfortunately (ME/CFS illness), will get to it
eventually…
Am Donnerstag, 13. November 2025, 16:45:45 Mitteleuropäische Normalzeit
schrieb Antoine Nguyen:
… @tonioo commented on this pull request.
> parser.add_argument("--debug", action="/Education?url=https%3A%2F%2Fgithub.com%2Fmodoboa%2Fmodoboa%2Fpull%2Fstore_true", help="Enable
> debug mode")
def handle(self, *args, **options):
"""Entry point."""
loop = asyncio.get_event_loop()
- coro = asyncio.start_server(
- core.new_connection, options["host"], options["port"]
- )
+ if os.environ.get("LISTEN_PID", "") == str(os.getpid()) and
os.environ.get("LISTEN_FDS", "").isnumeric():
@ntninja Any news about tests?
|
|
This PR is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 14 days. |
|
Still planning on doing this, just forgot. |
c529d39 to
bbe58a5
Compare
bbe58a5 to
52b6de6
Compare
1afc518 to
fd8c312
Compare
|
Well, I finally added tests 🥵 (tough) For some opaque reason it seems to fail on CPython 3.10 and 3.11, I have no idea what the problem is there, but it does not seem to be related to the test. |
Socket activation replaces manual socket binding if present and otherwise has no effect.
Redis may still be used in other cases, but it shouldn’t be a needless baseline requirement.
fd8c312 to
e6fb3b4
Compare
|
@ntninja Tests are failing under Python 3.10 and 3.11. Could you check why please? |
Description of the issue/feature this PR addresses:
Current behavior before PR:
Policy daemon must be started on boot and bind on loopback TCP
Desired behavior after PR is merged:
Both limitations are lifted