Socket activation for `foot --server` #890

Manually merged
dnkl merged 5 commits from VannTen/foot:socket_activation into master 6 months ago

This implement the use case in #604 with socket activation.
I've added systemd unit files to testing the change more easily (changing ExecStart=/path/to/dev/build/of/foot --server should be enough), but as requested in the linked issue, the approach should be usable with other supervision daemon if needed.

I'm not sure of my usage of the LOG_* macros, so feel free to correct me if those are not used correctly.

Regarding the systemd unit files (which I think could be shipped in foot packages), I used template unit in order to support multiples distinct sessions (and WAYLAND_DISPLAY). Unfortunately, I don't see how that could play nicely with systemctl enable, so currently the socket has to be started manually.

I'm also not sure of where else I should edit the documentation ?

Open to any feedback :)

This implement the use case in #604 with socket activation. I've added systemd unit files to testing the change more easily (changing ExecStart=/path/to/dev/build/of/foot --server should be enough), but as requested in the linked issue, the approach should be usable with other supervision daemon if needed. I'm not sure of my usage of the LOG_* macros, so feel free to correct me if those are not used correctly. Regarding the systemd unit files (which I think could be shipped in foot packages), I used template unit in order to support multiples distinct sessions (and WAYLAND_DISPLAY). Unfortunately, I don't see how that could play nicely with `systemctl enable`, so currently the socket has to be started manually. I'm also not sure of where else I should edit the documentation ? Open to any feedback :)
VannTen added 3 commits 7 months ago
22d6ca06b1 Make foot able to receive a socket from its parent
69e2e038e0 Adding systemd unit files for socket activation
75afad8ad2 Document socket activation feature with systemd
dnkl requested changes 7 months ago
dnkl left a comment
Owner

This is just a quick review - I haven't tested the code yet. Overall it looks good though.

Re. the LOG_*() macros, I'd say it looks fine. The ones about whether we're using already activated sockets, or are creating our own, could perhaps use LOG_DBG() instead.

This is just a quick review - I haven't tested the code yet. Overall it looks good though. Re. the `LOG_*()` macros, I'd say it looks fine. The ones about whether we're using already activated sockets, or are creating our own, could perhaps use `LOG_DBG()` instead.
server.c Outdated
if (listen_pid == NULL || *listen_pid == '\0'
|| listen_fds == NULL || *listen_fds == '\0')
return -1;
dnkl commented 7 months ago
Owner

style: || at the end of the previous line.

I also tend to add curly braces when the if statement is split up over multiple lines, for improved readability.

style: `||` at the end of the previous line. I also tend to add curly braces when the `if` statement is split up over multiple lines, for improved readability.
dnkl marked this conversation as resolved
server.c Outdated
errno = 0;
long pid = strtol(listen_pid, NULL, 10);
if (!(errno == 0) || getpid() != pid) {
LOG_ERR("Wrong LISTEN_PID environment variable ('%s'), aborting socket activation", listen_pid);
dnkl commented 7 months ago
Owner

Why !(errno == 0)? Why not just errno != 0, and drop the parenthesis?

Why `!(errno == 0)`? Why not just `errno != 0`, and drop the parenthesis?
dnkl marked this conversation as resolved
server.c Outdated
}
long number_of_fds = strtol(listen_fds, NULL, 10);
if (!(errno == 0)) {
dnkl commented 7 months ago
Owner

Same here

Same here
dnkl marked this conversation as resolved
server.c Outdated
}
for (int fd = LISTEN_FDS_START;fd < LISTEN_FDS_START + number_of_fds; fd++)
{
dnkl commented 7 months ago
Owner

style: curly brace on the same line as the for statement.

style: curly brace on the same line as the `for` statement.
dnkl marked this conversation as resolved
server.c Outdated
for (int fd = LISTEN_FDS_START;fd < LISTEN_FDS_START + number_of_fds; fd++)
{
int flags = fcntl(fd, F_GETFD);
errno = 0;
dnkl commented 7 months ago
Owner

This line can be dropped? No need to reset errno.

This line can be dropped? No need to reset `errno`.
dnkl marked this conversation as resolved
server.c Outdated
errno = 0;
if (flags == -1) {
LOG_ERRNO("Failed to get file descriptors flag for UNIX socket");
return (-1);
dnkl commented 7 months ago
Owner

Why the parenthesis?

Why the parenthesis?
dnkl marked this conversation as resolved
server.c Outdated
LOG_ERRNO("Failed to get file descriptors flag for UNIX socket");
return (-1);
}
if (fcntl(fd, F_SETFD, flags | FD_CLOEXEC)) {
dnkl commented 7 months ago
Owner

fcntl() returns an int, not a boolean. For F_SETFD, the return value is:

       F_SETFD     Value other than -1.

Thus, a check for != -1 is reasonable.

`fcntl()` returns an `int`, not a boolean. For `F_SETFD`, the return value is: > ``` > F_SETFD Value other than -1. > ``` Thus, a check for `!= -1` is reasonable.

I think you mean == -1, right? We want to log when an error occurs.

I think you mean `== -1`, right? We want to log when an error occurs.
dnkl marked this conversation as resolved
server.c Outdated
if (listen_fds() == 1)
{
fd = LISTEN_FDS_START;
LOG_INFO("We've been started by socket activation, using passed socket");
dnkl commented 7 months ago
Owner

The socket needs to be made non-blocking. This is probably the best place to do that. Even if systemd guarantees it already is (does it?), other socket activation mechanisms may not.

The socket needs to be made non-blocking. This is probably the best place to do that. Even if systemd guarantees it already is (does it?), other socket activation mechanisms may not.
Poster

Systemd allows to configure this in the service file with NonBlocking=true. I've updated the code to check for that.
This makes me think it should be the responsability of the socket activation mechanism, while foot would check the flag rather than set it. No hard opinion on this though; I can change that without problems.

Systemd allows to configure this in the service file with `NonBlocking=true`. I've updated the code to check for that. This makes me think it should be the responsability of the socket activation mechanism, while foot would check the flag rather than set it. No hard opinion on this though; I can change that without problems.
dnkl commented 6 months ago
Owner

As mentioned in the review, I think we should set non-blocking ourselves, and not bother with checking.

The reason is that a socket activator can't, and shouldn't make assumptions (even if those assumptions are configurable) on how the application will use the socket.

As mentioned in the review, I think we _should_ set non-blocking ourselves, and not bother with checking. The reason is that a socket activator can't, and shouldn't make assumptions (even if those assumptions are configurable) on how the application will use the socket.
Poster

No problem, I'll do it this way then (setting it ourselves)

No problem, I'll do it this way then (setting it ourselves)
VannTen marked this conversation as resolved
Owner

Oh, and we definitely want a CHANGELOG entry for this!

Oh, and we definitely want a CHANGELOG entry for this!

Thanks for this feature.

I didn't see code that actually installs the system files.

I see this has some documentation in the README, but it seems worth mentioning in doc/foot.1.scd and doc/footclient.1.scd as well. In particular, users should be advised of the syntax to run the "enable" command to activate the behavior.

Thanks for this feature. I didn't see code that actually installs the system files. I see this has some documentation in the README, but it seems worth mentioning in doc/foot.1.scd and doc/footclient.1.scd as well. In particular, users should be advised of the syntax to run the "enable" command to activate the behavior.
ericonr reviewed 7 months ago
[Service]
ExecStart=/usr/bin/foot --server

The path shouldn't be hardcoded. You should try generating the file based on the program path that meson creates (basically have foot-server@.service.in and replace @PATH_TO_FOOT@ in the file with the one meson creates).

The path shouldn't be hardcoded. You should try generating the file based on the program path that meson creates (basically have `foot-server@.service.in` and replace `@PATH_TO_FOOT@` in the file with the one meson creates).
Poster

I think the last commit (95fb4e7601) address that, but I'm no meson expert so I'm not sure it's the correct / idiomatic way to do it.

I think the last commit (https://codeberg.org/dnkl/foot/commit/95fb4e76015c626c320324cb0b7192736e77d49f) address that, but I'm no meson expert so I'm not sure it's the correct / idiomatic way to do it.
VannTen marked this conversation as resolved
server.c Outdated
long pid = strtol(listen_pid, NULL, 10);
if (!(errno == 0) || getpid() != pid) {
LOG_ERR("Wrong LISTEN_PID environment variable ('%s'), aborting socket activation", listen_pid);
return -1;

Given that you're printing an error here for mismatched LISTEN_PID and current pid, which a bunch of other software might also do, I think foot should call unsetenv to clean these env vars from its environment.

Given that you're printing an error here for mismatched `LISTEN_PID` and current pid, which a bunch of other software might also do, I think foot should call `unsetenv` to clean these env vars from its environment.
dnkl marked this conversation as resolved
server.c Outdated
return -1;
}
for (int fd = LISTEN_FDS_START;fd < LISTEN_FDS_START + number_of_fds; fd++)

You're missing a whitespace between ;fd.

That said, why does this need to be a loop? Shouldn't systemd provide us with only one socket, therefore only one file descriptor?

And LISTEN_FDS_START should probably get a comment to explain what it means (the first file descriptor that will be used for passing a socket to the child) -- which feels like a horrible API, they could have just used one more env var to specify that instead of hardcoding it ;-;

You're missing a whitespace between `;fd`. That said, why does this need to be a loop? Shouldn't systemd provide us with only one socket, therefore only one file descriptor? And `LISTEN_FDS_START` should probably get a comment to explain what it means (the first file descriptor that will be used for passing a socket to the child) -- which feels like a horrible API, they could have just used one more env var to specify that instead of hardcoding it ;-;
Poster

That said, why does this need to be a loop? Shouldn't systemd provide us with only one socket, therefore only one file descriptor?

You're right. I implemented the interface in man sd_listen_fds without enough reflexion ; since we only need file descriptor there is no need to loop.

And LISTEN_FDS_START should probably get a comment to explain what it means (the first file descriptor that will be used for passing a socket to the child) -- which feels like a horrible API, they could have just used one more env var to specify that instead of hardcoding it ;-;

I think it's just the first file descriptor possible after stdin/out/err.

> That said, why does this need to be a loop? Shouldn't systemd provide us with only one socket, therefore only one file descriptor? You're right. I implemented the interface in `man sd_listen_fds` without enough reflexion ; since we only need file descriptor there is no need to loop. > And LISTEN_FDS_START should probably get a comment to explain what it means (the first file descriptor that will be used for passing a socket to the child) -- which feels like a horrible API, they could have just used one more env var to specify that instead of hardcoding it ;-; I think it's just the first file descriptor possible after stdin/out/err.
dnkl marked this conversation as resolved
emersion reviewed 7 months ago
[Service]
ExecStart=/usr/bin/foot --server
Environment=WAYLAND_DISPLAY=%i

This doesn't wait for the compositor to come up before starting footserver.

Another approach is to use ConditionEnvironment and let users import WAYLAND_DISPLAY into the systemd socket activation env.

This doesn't wait for the compositor to come up before starting footserver. Another approach is to use `ConditionEnvironment` and let users import `WAYLAND_DISPLAY` into the systemd socket activation env.

This doesn't wait for the compositor to come up before starting footserver.

Would calling the foot socket before the compositor comes up be a problem in practice? How is user going to end up trying to connect to the foot socket if Wayland isn't up yet to launch terminals?

Another approach is to use ConditionEnvironment and let users import WAYLAND_DISPLAY into the systemd socket activation env.

The sway-systemd projects handles importing WAYLAND_DISPLAY into the systemd environment. It works through a shell script that's called by Sway once Sway has started:

https://github.com/alebastr/sway-systemd/blob/main/src/session.sh

I think it could also be fair to expect that people have set WAYLAND_DISPLAY in the systemd environment by using sway-systemd or their own DIY solution.

If ConditionEnvironment= is used, it could be good include a comment that it requires systemd 246 or later. That version was released in July 2020.

> This doesn't wait for the compositor to come up before starting footserver. Would calling the foot socket before the compositor comes up be a problem in practice? How is user going to end up trying to connect to the foot socket if Wayland isn't up yet to launch terminals? > Another approach is to use `ConditionEnvironment` and let users import `WAYLAND_DISPLAY` into the systemd socket activation env. The `sway-systemd` projects handles importing WAYLAND_DISPLAY into the systemd environment. It works through a shell script that's called by Sway once Sway has started: https://github.com/alebastr/sway-systemd/blob/main/src/session.sh I think it could also be fair to expect that people have set WAYLAND_DISPLAY in the systemd environment by using `sway-systemd` or their own DIY solution. If `ConditionEnvironment=` is used, it could be good include a comment that it requires systemd 246 or later. That version was released in July 2020.
Poster

The problem with importing WAYLAND_DISPLAY into the systemd user-instance is that we can't support multiples WAYLAND_DISPLAY and one foot-server for each (several sesssions, or nested compositor uses would require this, right ?)

The problem with importing WAYLAND_DISPLAY into the systemd user-instance is that we can't support multiples WAYLAND_DISPLAY and one foot-server for each (several sesssions, or nested compositor uses would require this, right ?)

I think it's fairly hard to deal with nested Wayland sessions anyway with systemd. Maybe we can just ship two different services (and sockets for that matter), one that uses $WAYLAND_DISPLAY from systemd environment and this one, which can be started per wayland session.

I think it's fairly hard to deal with nested Wayland sessions anyway with systemd. Maybe we can just ship two different services (and sockets for that matter), one that uses $WAYLAND_DISPLAY from systemd environment and this one, which can be started per wayland session.
Poster

I think it's fairly hard to deal with nested Wayland sessions anyway with systemd.

Yeah, it's even kinda out-of-scope from the systemd point of view : https://systemd.io/DESKTOP_ENVIRONMENTS/
I have some ideas to build something with systemd run and per-session scope units, but that's not really relevant to foot.

Two units seems a good compromise, will do that, with the conditional check for the non-templated one.

> I think it's fairly hard to deal with nested Wayland sessions anyway with systemd. Yeah, it's even kinda out-of-scope from the systemd point of view : https://systemd.io/DESKTOP_ENVIRONMENTS/ I have some ideas to build something with systemd run and per-session scope units, but that's not really relevant to foot. Two units seems a good compromise, will do that, with the conditional check for the non-templated one.
Poster

Hum. That's not as easy as I thought, unless we fix the socket path. As far as I can tell, socket units don't support environment variables in ListenStream. I'm not sure if fixing the socket path to wayland-1 is such a good idea...

Hum. That's not as easy as I thought, unless we fix the socket path. As far as I can tell, socket units don't support environment variables in `ListenStream`. I'm not sure if fixing the socket path to `wayland-1` is such a good idea...

The problem with importing WAYLAND_DISPLAY into the systemd user-instance is that we can't support multiples WAYLAND_DISPLAY and one foot-server for each (several sesssions, or nested compositor uses would require this, right ?)

You are right. sway-systemd doesn't currently support that case.
https://github.com/alebastr/sway-systemd/pull/12

> The problem with importing WAYLAND_DISPLAY into the systemd user-instance is that we can't support multiples WAYLAND_DISPLAY and one foot-server for each (several sesssions, or nested compositor uses would require this, right ?) You are right. sway-systemd doesn't currently support that case. https://github.com/alebastr/sway-systemd/pull/12
VannTen added 5 commits 7 months ago
698fe3b990 Validate the socket setup in dedicated function
3290b4c161 Unset environment containing socket fd information
60ab71e356 Document LISTEN_FDS_START
Poster

Regarding systemd services files :

I didn't see code that actually installs the system files.

The path shouldn't be hardcoded. You should try generating the file based on the program path that meson creates (basically have foot-server@.service.in and replace @PATH_TO_FOOT@ in the file with the one meson creates).

The unit files should be in /usr/lib/systemd/user/; and it should be the build system / packaging installing them.
I'm not familiar with meson so I'll need to check how that's done (both the templating and the install part).

I see this has some documentation in the README, but it seems worth mentioning in doc/foot.1.scd and doc/footclient.1.scd as well. In particular, users should be advised of the syntax to run the "enable" command to activate the behavior.

I've updated doc/foot.1.scd, but I think I still need to rewrite that, it's not completely clear right now.

@dnkl I think I addressed your remarks; please let me know if I missed some.
Also, would you prefer I do incremental commits and rebase when the PR is in good shape or rebase right away ?

I've change listen_fds function name and purpose : instead of juste replicating what sd_listen_fds do (in libsystemd), it also validate that the socket is fit for our purposes (I've been inspired to do that by the sd_is_socket_unix function, mostly).

Things I still need to do :

  • adressing the server_destroy part. When using socket activation, foot should not unlink the socket, I think.
  • I'm not sure how to work with the configured sock_path. Should I check the passed socket has the same path, or just overwrite it ?
Regarding systemd services files : > I didn't see code that actually installs the system files. > The path shouldn't be hardcoded. You should try generating the file based on the program path that meson creates (basically have `foot-server@.service.in` and replace `@PATH_TO_FOOT@` in the file with the one meson creates). The unit files should be in `/usr/lib/systemd/user/`; and it should be the build system / packaging installing them. I'm not familiar with meson so I'll need to check how that's done (both the templating and the install part). > I see this has some documentation in the README, but it seems worth mentioning in doc/foot.1.scd and doc/footclient.1.scd as well. In particular, users should be advised of the syntax to run the "enable" command to activate the behavior. I've updated `doc/foot.1.scd`, but I think I still need to rewrite that, it's not completely clear right now. @dnkl I think I addressed your remarks; please let me know if I missed some. Also, would you prefer I do incremental commits and rebase when the PR is in good shape or rebase right away ? I've change listen_fds function name and purpose : instead of juste replicating what sd_listen_fds do (in libsystemd), it also validate that the socket is fit for our purposes (I've been inspired to do that by the `sd_is_socket_unix` function, mostly). Things I still need to do : - adressing the server_destroy part. When using socket activation, foot should not unlink the socket, I think. - I'm not sure how to work with the configured sock_path. Should I check the passed socket has the same path, or just overwrite it ?
Owner

Please do individual commits, and rebase once everything is done.

Also, heads up: it'll be roughly a week before I can review this. Don't expect much input until then.

Please do individual commits, and rebase once everything is done. Also, heads up: it'll be roughly a week before I can review this. Don't expect much input until then.
VannTen changed title from Socket activation for `foot --server` to WIP: Socket activation for `foot --server` 7 months ago
VannTen added 1 commit 7 months ago
4958ec4e63 Don't delete socket on exit when socket-activated
Poster

I've addressed the last two point in a minimal way : I set the socket_path to NULL, which avoid having it unlinked in server_destroy. If that's not enough I can rework that.

I've addressed the last two point in a minimal way : I set the socket_path to NULL, which avoid having it unlinked in server_destroy. If that's not enough I can rework that.
VannTen added 1 commit 7 months ago
95fb4e7601 Include systemd units files in build system
VannTen changed title from WIP: Socket activation for `foot --server` to Socket activation for `foot --server` 7 months ago
dnkl requested changes 6 months ago
dnkl left a comment
Owner

See comments below.

I can't really comment on the systemd specifics, but if everyone else is happy, then I'm happy. But, I'm going to throw in a disclaimer here; I won't be able to maintain the systemd service files. If there are issues in the future, that the community cannot fix, then I'll be forced to remove them.

See comments below. I can't really comment on the systemd specifics, but if everyone else is happy, then I'm happy. But, I'm going to throw in a disclaimer here; I won't be able to maintain the systemd service files. If there are issues in the future, that the community cannot fix, then I'll be forced to remove them.
doc/foot.1.scd Outdated
the provided path will be ignored by *foot(1)*.
When using systemd socket-activation, you'll need to start (not enable) the
socket *foot-server@$WAYLAND\_DISPLAY.socket*.
dnkl commented 6 months ago
Owner

Further down, there's a section listing environment variables used (and/or set) by foot. LISTEN_PID and LISTEN_FDS should be added to that list.

Further down, there's a section listing environment variables used (and/or set) by foot. `LISTEN_PID` and `LISTEN_FDS` should be added to that list.
Poster

I'm gonna wait until we settle on the way to get the socket, since these variables won't be used if we got the --server={FD} path.

I'm gonna wait until we settle on the way to get the socket, since these variables won't be used if we got the --server={FD} path.
VannTen marked this conversation as resolved
meson.build Outdated
configuration = configuration_data()
configuration.set('bindir', join_paths(get_option('prefix'), get_option('bindir')))
dnkl commented 6 months ago
Owner

Can you move this into the if systemd.found() block below?

Can you move this into the `if systemd.found()` block below?
dnkl marked this conversation as resolved
server.c Outdated
goto err;
flags = fcntl(LISTEN_FDS_START, F_GETFL);
if (flags < 0) {
LOG_ERR("Failed to get file status flags for passed socket");
dnkl commented 6 months ago
Owner

Use LOG_ERRNO()

Use `LOG_ERRNO()`
dnkl marked this conversation as resolved
server.c Outdated
}
if ((flags & O_NONBLOCK) != O_NONBLOCK) {
LOG_ERR("Passed socket is not in non-blocking mode");
return false;
dnkl commented 6 months ago
Owner

I really think we should just ignore the current mode and just set non-blocking ourselves. The socket activator cannot know how the application (foot) will use the socket, and can thus not make any assumptions. That is, it should be perfectly fine for us to set non-blocking ourselves. If not, I'd consider the socket activator broken.

I recommend skipping the check and instead just set non-blocking.

I really think we should just ignore the current mode and just set non-blocking ourselves. The socket activator cannot know how the application (foot) will use the socket, and can thus not make any assumptions. That is, it should be perfectly fine for us to set non-blocking ourselves. If not, I'd consider the socket activator broken. I recommend skipping the check and instead just set non-blocking.
dnkl marked this conversation as resolved
server.c Outdated
int socket_option = 0;
socklen_t len;
for (size_t i = 0; i < sizeof(socket_options) / sizeof(*socket_options) ; i++) {
dnkl commented 6 months ago
Owner

util.h defines the macro ALEN(), that you can use instead of manually doing sizeof(array) / sizeof(array[0]).

`util.h` defines the macro `ALEN()`, that you can use instead of manually doing `sizeof(array) / sizeof(array[0])`.
dnkl marked this conversation as resolved
server.c Outdated
len = sizeof(socket_option);
if (getsockopt(LISTEN_FDS_START, SOL_SOCKET, socket_options[i], &socket_option, &len) == -1 ||
len != sizeof(socket_option)) {
LOG_ERR("Failed to read socket option from passed file descriptor");
dnkl commented 6 months ago
Owner

Use LOG_ERRNO()

Use `LOG_ERRNO()`
dnkl marked this conversation as resolved
Owner

adressing the server_destroy part. When using socket activation, foot should not unlink the socket, I think.

I agree, foot should not unlink in this case.

I'm not sure how to work with the configured sock_path. Should I check the passed socket has the same path, or just overwrite it ?

To increase compatibility with other socket activators, I'm tempted to say that --server={FD} means "assume socket activation". In all other cases, we create the socket (and thus ignore LISTEN_FDS).

For systemd, this means the foot socket unit should set --server=3. This would also mean we can remove the LISTEN_FDS_START macro.

> adressing the server_destroy part. When using socket activation, foot should not unlink the socket, I think. I agree, foot should not unlink in this case. > I'm not sure how to work with the configured sock_path. Should I check the passed socket has the same path, or just overwrite it ? To increase compatibility with other socket activators, I'm tempted to say that `--server={FD}` means "assume socket activation". In all other cases, we create the socket (and thus ignore `LISTEN_FDS`). For systemd, this means the foot socket unit should set `--server=3`. This would also mean we can remove the `LISTEN_FDS_START` macro.
Owner

To increase compatibility with other socket activators, I'm tempted to say that --server={FD} means "assume socket activation". In all other cases, we create the socket (and thus ignore LISTEN_FDS).

For example, I think this would make e.g. s6 integration easy.

> To increase compatibility with other socket activators, I'm tempted to say that --server={FD} means "assume socket activation". In all other cases, we create the socket (and thus ignore LISTEN_FDS). For example, I think this would make e.g. s6 integration easy.
dnkl added the
enhancement
label 6 months ago
Owner

Foot-1.11 has been released. This PR needs to be rebased. Pay attention to the changelog - the changes here should go into 'unreleased', not 1.11.

Foot-1.11 has been released. This PR needs to be rebased. Pay attention to the changelog - the changes here should go into 'unreleased', not 1.11.
Poster

I haven't forgotten this PR, but I have a lot on my plate right now. I hope to address your review for next week.

I haven't forgotten this PR, but I have a lot on my plate right now. I hope to address your review for next week.

@VannTen, thanks in advance for your work on t his.

I've been running foot --server directly from Sway and it sometimes crashes ( #935 ), and then doesn't get restarted.

Once it's switched to systemd management I'll be able to set foot --server to automatically restart after it crashes.

@VannTen, thanks in advance for your work on t his. I've been running `foot --server` directly from Sway and it sometimes crashes ( https://codeberg.org/dnkl/foot/issues/935 ), and then doesn't get restarted. Once it's switched to systemd management I'll be able to set `foot --server` to automatically restart after it crashes.
Poster

To increase compatibility with other socket activators, I'm tempted to say that --server={FD} means "assume socket activation". In all other cases, we create the socket (and thus ignore LISTEN_FDS).

For systemd, this means the foot socket unit should set --server=3. This would also mean we can remove the LISTEN_FDS_START macro.

How should {FD} be distinguished from a path ? Do we simply assume if it's a number, it's an FD, or should I add somehthing like what dockerd -H does ?

-H,  --host=[unix:///var/run/docker.sock]: tcp://[host:port] to bind or unix://[/path/to/socket] to use.

    The socket(s) to bind to in daemon mode specified using one or more
     tcp://host:port, unix:///path/to/socket, fd://* or fd://socketfd.

Foot-1.11 has been released. This PR needs to be rebased. Pay attention to the changelog - the changes here should go into 'unreleased', not 1.11.

I'd rather do that once when it's done, but if you prefer I'll rebase, no problems.

@markstos

Once it's switched to systemd management I'll be able to set foot --server to automatically restart after it crashes.

I think until it's merged you can just put a user unit in your ~/.config, or use systemd-run --user -p Restart=on-failure foot --server from your sway config ?

> To increase compatibility with other socket activators, I'm tempted to say that `--server={FD}` means "assume socket activation". In all other cases, we create the socket (and thus ignore `LISTEN_FDS`). > > For systemd, this means the foot socket unit should set `--server=3`. This would also mean we can remove the `LISTEN_FDS_START` macro. How should `{FD}` be distinguished from a path ? Do we simply assume if it's a number, it's an FD, or should I add somehthing like what `dockerd -H` does ? ``` -H,  --host=[unix:///var/run/docker.sock]: tcp://[host:port] to bind or unix://[/path/to/socket] to use. The socket(s) to bind to in daemon mode specified using one or more tcp://host:port, unix:///path/to/socket, fd://* or fd://socketfd. ``` > Foot-1.11 has been released. This PR needs to be rebased. Pay attention to the changelog - the changes here should go into 'unreleased', not 1.11. I'd rather do that once when it's done, but if you prefer I'll rebase, no problems. @markstos > Once it's switched to systemd management I'll be able to set `foot --server` to automatically restart after it crashes. I think until it's merged you can just put a user unit in your ~/.config, or use `systemd-run --user -p Restart=on-failure foot --server` from your sway config ?
Owner

How should {FD} be distinguished from a path ? Do we simply assume if it's a number, it's an FD, or should I add somehthing like what dockerd -H does ?

Keep it simple, for now. If it's a number, assume it's an FD. This is how the --print-pid option works.

I'd rather do that once when it's done, but if you prefer I'll rebase, no problems.

That's fine, but be aware CI will keep failing on a bad fcft dep until you do.

> How should {FD} be distinguished from a path ? Do we simply assume if it's a number, it's an FD, or should I add somehthing like what dockerd -H does ? Keep it simple, for now. If it's a number, assume it's an FD. This is how the `--print-pid` option works. > I'd rather do that once when it's done, but if you prefer I'll rebase, no problems. That's fine, but be aware CI will keep failing on a bad fcft dep until you do.
dnkl requested changes 6 months ago
server.c Outdated
long pid = strtol(listen_pid, NULL, 10);
if (errno != 0 || getpid() != pid) {
LOG_ERR("Wrong LISTEN_PID environment variable ('%s'), aborting socket activation", listen_pid);
return false;
dnkl commented 6 months ago
Owner

Sorry I didn't notice/mention this before; for consistency with the rest of the code, all LOG_*() should use lower-cased messages.

I.e, in this particular case: LOG_ERR("wrong LISTEN_PID environment variable...");.

Sorry I didn't notice/mention this before; for consistency with the rest of the code, all `LOG_*()` should use lower-cased messages. I.e, in this particular case: `LOG_ERR("wrong LISTEN_PID environment variable...");`.
VannTen marked this conversation as resolved
server.c Outdated
char const * const socket_options_names[] = { "SO_DOMAIN", "SO_ACCEPTCONN", "SO_TYPE" };
assert(ALEN(socket_options) == ALEN(socket_options_values));
assert(ALEN(socket_options) == ALEN(socket_options_names));
dnkl commented 6 months ago
Owner

Please use xassert() instead. It's like assert(), but prints a backtrace on failure.

Please use `xassert()` instead. It's like `assert()`, but prints a backtrace on failure.
VannTen marked this conversation as resolved
Poster

I checked out s6 documentation regarding socket-activation.
If I get it right, they simply use stdin. ( When you want to run a daemon myserverd that accepts clients connecting to /my/socket, run s6-fdholder-retrieve /service/fdholder/s unix:/my/socket myserverd. myserverd will be executed with /my/socket as its standard input.)
And in fact, systemd support the exact same thing, it just needs to be specified in the service file.
This would also be compatible with inetd.
(you mentionned that on the issue... I should have read the systemd man more thoroughly)

The limitation is that we can only have one socket, but I don't think it's a problem for foot. On the plus side, I think the whole LISTEN_FDS can then go away (I need to check if systemd does not expose those variable when using stdin for the socket).


If implemented this way, would it be better to have foot --server=0 as mentionned or foot --server=- / foot --server=/dev/stdin ?

I checked out [s6 documentation regarding socket-activation](https://skarnet.org/software/s6/socket-activation.html). If I get it right, they simply use stdin. (` When you want to run a daemon myserverd that accepts clients connecting to /my/socket, run s6-fdholder-retrieve /service/fdholder/s unix:/my/socket myserverd. myserverd will be executed with /my/socket as its standard input.`) And in fact, systemd support the exact same thing, it just needs to be specified in the service file. This would also be compatible with inetd. (you mentionned that on the issue... I should have read the systemd man more thoroughly) The limitation is that we can only have one socket, but I don't think it's a problem for foot. On the plus side, I think the whole LISTEN_FDS can then go away (I need to check if systemd does not expose those variable when using stdin for the socket). ---- If implemented this way, would it be better to have `foot --server=0` as mentionned or `foot --server=-` / `foot --server=/dev/stdin` ?
Owner

If I get it right, they simply use stdin.

Correct. But it is also common to redirect it before starting the daemon.

And in fact, systemd support the exact same thing, it just needs to be specified in the service file.
This would also be compatible with inetd.

As long as we're not relying on a systemd/inetd compatibility thingy...

I don't think we should assume the socket is passed via stdin. There may be yet other socket activation mechanisms that behave differently.

To the best of my knowledge, accepting an FD number as a parameter on the command line is the most generic way to deal with as many possibilities as possible.

Therefore, I think we should stick to --server=FD.

As for the systemd .socket file details, I hope others, who've already lent their expertise here, can help out :)

> If I get it right, they simply use stdin. Correct. But it is also common to redirect it before starting the daemon. > And in fact, systemd support the exact same thing, it just needs to be specified in the service file. > This would also be compatible with inetd. As long as we're not relying on a systemd/inetd compatibility thingy... I don't think we should assume the socket is passed via stdin. There may be yet other socket activation mechanisms that behave differently. To the best of my knowledge, accepting an FD number as a parameter on the command line is the most generic way to deal with as many possibilities as possible. Therefore, I think we should stick to `--server=FD`. As for the systemd .socket file details, I hope others, who've already lent their expertise here, can help out :)
Poster

To the best of my knowledge, accepting an FD number as a parameter on the command line is the most generic way to deal with as many possibilities as possible.

Therefore, I think we should stick to --server=FD.

Understood, I'll implement it that way.

> To the best of my knowledge, accepting an FD number as a parameter on the command line is the most generic way to deal with as many possibilities as possible. > > Therefore, I think we should stick to `--server=FD`. Understood, I'll implement it that way.
dnkl requested changes 6 months ago
dnkl left a comment
Owner

One tiny nit, then this is good on my side.

@markstos @Scrumplex @ericonr do either of you have anything more to add regarding the systemd service?

One tiny nit, then this is good on my side. @markstos @Scrumplex @ericonr do either of you have anything more to add regarding the systemd service?
doc/foot.1.scd Outdated
*--server-socket* option in *footclient*(1) and point it to your
custom socket path.
If PATH is a number, foot will interpret it as a socket provided by a
dnkl commented 6 months ago
Owner

I would suggest this:

@@ -81,7 +81,7 @@ the foot command line
        Initial working directory for the client application. Default:
        _CWD of foot_.
 
-*-s*,*--server*[=_PATH_]
+*-s*,*--server*[=_PATH_|_FD_]
        Run as a server. In this mode, a single foot instance hosts
        multiple terminals (windows). Use *footclient*(1) to launch new
        terminals.

and then re-write the description accordingly: something like "If the argument is a number..."

I would suggest this: ```diff @@ -81,7 +81,7 @@ the foot command line Initial working directory for the client application. Default: _CWD of foot_. -*-s*,*--server*[=_PATH_] +*-s*,*--server*[=_PATH_|_FD_] Run as a server. In this mode, a single foot instance hosts multiple terminals (windows). Use *footclient*(1) to launch new terminals. ``` and then re-write the description accordingly: something like "If the argument is a number..."

I've been following this. The feature looks good to me as a systemd user.

Although I've been able to get a sufficient amount of systemd integration with the tip to add this to my Sway config file:

exec systemd-run --user --property=Restart=on-failure foot --server

The primary difference I see between that approach and socket activation is that the solution above starts the foot server as soon as I start sway, while socket-activation starts the server just-in-time on first use.

Since I use a terminal in almost every Sway session and the server doesn't take long to start, there is little practical difference.

I've been following this. The feature looks good to me as a systemd user. Although I've been able to get a sufficient amount of systemd integration with the tip to add this to my Sway config file: ``` exec systemd-run --user --property=Restart=on-failure foot --server ``` The primary difference I see between that approach and socket activation is that the solution above starts the foot server as soon as I start sway, while socket-activation starts the server just-in-time on first use. Since I use a terminal in almost every Sway session and the server doesn't take long to start, there is little practical difference.

LGTM as well. I think it's quite flexible as it is. Most users will probably just enable foot-server@wayland-1.socket, while more advanced users have a way to utilize this in nested sessions.

LGTM as well. I think it's quite flexible as it is. Most users will probably just enable `foot-server@wayland-1.socket`, while more advanced users have a way to utilize this in nested sessions.
VannTen force-pushed socket_activation from f4e0eeece7 to 2a9130f570 6 months ago
Poster

Done and rebased !
I've kept the full history in socket_activation_full_history on my fork if needed.

Done and rebased ! I've kept the full history in socket_activation_full_history on my fork if needed.
markstos reviewed 6 months ago
doc/foot.1.scd Outdated
use that socket as it's own.
Two systemd units (foot-server@.{service,socket}) are provided to use that
feature with systemd. They need to be instancied with the value of

Typo: s/instancied/instantiated/

Typo: s/instancied/instantiated/

I lost some hours today trying to track down why my SSH authentication broke.

The root cause turned out to be a subtle but critical behavior change that happens when you run foot as a systemd service.

The issue is that, by design, systemd runs services in a "clean" environment, with only environment variables that either explicitly set or inherited from the systemd user service manager, which itself only contains environment variables that are explicitly set there.

In a Sway config, if you do:

exec foot --server

Then all of Sway's exported environment variables will be pass into the foot server as a child process and onto the foot clients. But if you start foot by systemd through socket activation or like this:

exec systemd-run --user --property=Restart=on-failure foot --server 

Then none of those variables are passed through, and you may wonder why you get SSH errors after enabling foot's systemd socket activation feature.

I've proposed a fix narrowly for the SSH_AUTH_SOCK issue by having the sway-systemd project automatically pass this specific variable through to systemd-managed units:

https://github.com/alebastr/sway-systemd/pull/17

But users could still be confused when the same issue affects other variables.

So I think a documentation note is called for to warn users about this behavior change when using systemd socket activation. If they are also using sway-systemd, they can the feature to explicitly pass through extra variables to sessions:

https://github.com/alebastr/sway-systemd/blob/main/src/session.sh#L50

Other users can use systemctl --user import-environment VARIABLE... to pass other environment variables to the systemd user service.

I lost some hours today trying to track down why my SSH authentication broke. The root cause turned out to be a subtle but critical behavior change that happens when you run `foot` as a systemd service. The issue is that, by design, systemd runs services in a "clean" environment, with only environment variables that either explicitly set or inherited from the systemd user service manager, which itself only contains environment variables that are explicitly set there. In a Sway config, if you do: ``` exec foot --server ``` Then all of Sway's exported environment variables will be pass into the foot server as a child process and onto the foot clients. But if you start `foot` by systemd through socket activation or like this: ``` exec systemd-run --user --property=Restart=on-failure foot --server ``` Then none of those variables are passed through, and you may wonder why you get SSH errors after enabling foot's systemd socket activation feature. I've proposed a fix narrowly for the SSH_AUTH_SOCK issue by having the `sway-systemd` project automatically pass this specific variable through to systemd-managed units: https://github.com/alebastr/sway-systemd/pull/17 But users could still be confused when the same issue affects other variables. So I think a documentation note is called for to warn users about this behavior change when using systemd socket activation. If they are also using `sway-systemd`, they can the feature to explicitly pass through extra variables to sessions: https://github.com/alebastr/sway-systemd/blob/main/src/session.sh#L50 Other users can use `systemctl --user import-environment VARIABLE...` to pass other environment variables to the systemd user service.

Last night I thought I figured out how to get the foot systemd service to work with gnome-keyring-daemon, but apparently I only get it partially fixed. I've got the environment variables set in foot now, but I've not able to figure how to get gnome-keyring-daemon and a systemd foot service to work together.

I have this in ~/.config/fish/conf.d/999-sway.conf:

# If running from tty1 and a graphical session has not already been started, start Sway
set TTY1 (tty)
if status --is-login && test "$TTY1" = "/dev/tty1"  && test -z $WAYLAND_DISPLAY
  set --global --export (gnome-keyring-daemon --start | string split "=")
  set --global --export DESKTOP_SESSION "sway"
  set --global --export TERMINAL "foot"

  /sbin/sway
end
Last night I thought I figured out how to get the foot systemd service to work with `gnome-keyring-daemon`, but apparently I only get it partially fixed. I've got the environment variables set in foot now, but I've not able to figure how to get `gnome-keyring-daemon` and a `systemd` `foot` service to work together. I have this in `~/.config/fish/conf.d/999-sway.conf`: ``` # If running from tty1 and a graphical session has not already been started, start Sway set TTY1 (tty) if status --is-login && test "$TTY1" = "/dev/tty1" && test -z $WAYLAND_DISPLAY set --global --export (gnome-keyring-daemon --start | string split "=") set --global --export DESKTOP_SESSION "sway" set --global --export TERMINAL "foot" /sbin/sway end ```
Owner

So I think a documentation note is called for to warn users about this behavior change when using systemd socket activation. If they are also using sway-systemd, they can the feature to explicitly pass through extra variables to sessions:

@VannTen I agree with this. I think a generic note, reminding people that a foot --server started via socket activation may be missing several environment variables. Maybe also mention how to pass-through variables in systemd.

> So I think a documentation note is called for to warn users about this behavior change when using systemd socket activation. If they are also using sway-systemd, they can the feature to explicitly pass through extra variables to sessions: @VannTen I agree with this. I think a generic note, reminding people that a `foot --server` started via socket activation may be missing several environment variables. Maybe also mention how to pass-through variables in systemd.
Poster

Good catch. I've included a note in the man page.

Good catch. I've included a note in the man page.
VannTen force-pushed socket_activation from 2a9130f570 to 2ec4a5bf83 6 months ago
dnkl requested changes 6 months ago
dnkl left a comment
Owner

Almost there...

I think we should add "systemd" to the build-time dependecy list in INSTALL.md, and describe what happens when it's available (i.e. that foot will install the systemd service files).

Almost there... I think we should add "systemd" to the build-time dependecy list in [INSTALL.md](https://codeberg.org/dnkl/foot/src/commit/f76c9e77f1bd2c2c844a703542e939bfb0e77512/INSTALL.md#user-content-building), and describe what happens when it's available (i.e. that foot will install the systemd service files).
README.md Outdated
want to launch a new terminal.
Foot support socket activation, which means `foot --server` will only be
started the first time you'll run `footclient`. (systemd user units are
dnkl commented 6 months ago
Owner

nit: double spaces between "support" and "socket".

nit: double spaces between "support" and "socket".
dnkl marked this conversation as resolved
VannTen force-pushed socket_activation from 2ec4a5bf83 to 522f4e522e 6 months ago
dnkl approved these changes 6 months ago
dnkl merged commit a65804139d into master manually 6 months ago
Owner

Thanks!

Thanks!

Reviewers

dnkl approved these changes 6 months ago
ci/woodpecker/pr/woodpecker Pipeline was successful
The pull request has been manually merged as a65804139d.
Sign in to join this conversation.
Loading…
There is no content yet.