Socket activation for `foot --server` #890
Manually merged
dnkl
merged 5 commits from VannTen/foot:socket_activation
into master
3 months ago
Loading…
Reference in new issue
There is no content yet.
Delete Branch 'VannTen/foot:socket_activation'
Deleting a branch is permanent. It CANNOT be undone. Continue?
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 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 useLOG_DBG()
instead.if (listen_pid == NULL || *listen_pid == '\0'
|| listen_fds == NULL || *listen_fds == '\0')
return -1;
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.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);
Why
!(errno == 0)
? Why not justerrno != 0
, and drop the parenthesis?}
long number_of_fds = strtol(listen_fds, NULL, 10);
if (!(errno == 0)) {
Same here
}
for (int fd = LISTEN_FDS_START;fd < LISTEN_FDS_START + number_of_fds; fd++)
{
style: curly brace on the same line as the
for
statement.for (int fd = LISTEN_FDS_START;fd < LISTEN_FDS_START + number_of_fds; fd++)
{
int flags = fcntl(fd, F_GETFD);
errno = 0;
This line can be dropped? No need to reset
errno
.errno = 0;
if (flags == -1) {
LOG_ERRNO("Failed to get file descriptors flag for UNIX socket");
return (-1);
Why the parenthesis?
LOG_ERRNO("Failed to get file descriptors flag for UNIX socket");
return (-1);
}
if (fcntl(fd, F_SETFD, flags | FD_CLOEXEC)) {
fcntl()
returns anint
, not a boolean. ForF_SETFD
, the return value is:Thus, a check for
!= -1
is reasonable.I think you mean
== -1
, right? We want to log when an error occurs.if (listen_fds() == 1)
{
fd = LISTEN_FDS_START;
LOG_INFO("We've been started by socket activation, using passed socket");
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.
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.
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.
No problem, I'll do it this way then (setting it ourselves)
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.
[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).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.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 callunsetenv
to clean these env vars from its environment.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 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.I think it's just the first file descriptor possible after stdin/out/err.
[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 importWAYLAND_DISPLAY
into the systemd socket activation env.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?
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.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.
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.
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 towayland-1
is such a good idea...You are right. sway-systemd doesn't currently support that case.
https://github.com/alebastr/sway-systemd/pull/12
Regarding systemd services files :
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'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 :
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.
Socket activation for `foot --server`to WIP: Socket activation for `foot --server` 4 months agoI'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.
WIP: Socket activation for `foot --server`to Socket activation for `foot --server` 4 months agoSee 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.
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*.
Further down, there's a section listing environment variables used (and/or set) by foot.
LISTEN_PID
andLISTEN_FDS
should be added to that list.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.
configuration = configuration_data()
configuration.set('bindir', join_paths(get_option('prefix'), get_option('bindir')))
Can you move this into the
if systemd.found()
block below?goto err;
flags = fcntl(LISTEN_FDS_START, F_GETFL);
if (flags < 0) {
LOG_ERR("Failed to get file status flags for passed socket");
Use
LOG_ERRNO()
}
if ((flags & O_NONBLOCK) != O_NONBLOCK) {
LOG_ERR("Passed socket is not in non-blocking mode");
return false;
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.
int socket_option = 0;
socklen_t len;
for (size_t i = 0; i < sizeof(socket_options) / sizeof(*socket_options) ; i++) {
util.h
defines the macroALEN()
, that you can use instead of manually doingsizeof(array) / sizeof(array[0])
.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");
Use
LOG_ERRNO()
I agree, foot should not unlink in this case.
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 ignoreLISTEN_FDS
).For systemd, this means the foot socket unit should set
--server=3
. This would also mean we can remove theLISTEN_FDS_START
macro.For example, I think this would make e.g. s6 integration easy.
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 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.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 whatdockerd -H
does ?I'd rather do that once when it's done, but if you prefer I'll rebase, no problems.
@markstos
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 ?Keep it simple, for now. If it's a number, assume it's an FD. This is how the
--print-pid
option works.That's fine, but be aware CI will keep failing on a bad fcft dep until you do.
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;
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...");
.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));
Please use
xassert()
instead. It's likeassert()
, but prints a backtrace on failure.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 orfoot --server=-
/foot --server=/dev/stdin
?Correct. But it is also common to redirect it before starting the daemon.
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 :)
Understood, I'll implement it that way.
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?
*--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
I would suggest this:
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:
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.f4e0eeece7
to2a9130f570
3 months agoDone and rebased !
I've kept the full history in socket_activation_full_history on my fork if needed.
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/
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:
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: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 getgnome-keyring-daemon
and asystemd
foot
service to work together.I have this in
~/.config/fish/conf.d/999-sway.conf
:@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.Good catch. I've included a note in the man page.
2a9130f570
to2ec4a5bf83
3 months agoAlmost 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).
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
nit: double spaces between "support" and "socket".
2ec4a5bf83
to522f4e522e
3 months agoa65804139d
into master manually 3 months agoThanks!
Reviewers
a65804139d
.