~kennylevinsen/seatd-devel

This thread contains a patchset. You're looking at the original emails, but you may wish to use the patch review UI. Review patch
6 3

[PATCH 0/2] seatd: Add systemd socket activation

Michael Tretter <m.tretter@pengutronix.de>
Details
Message ID
<20230127104421.4075465-1-m.tretter@pengutronix.de>
DKIM signature
missing
Download raw message
seatd already provides a systemd.service to start and run seatd. As seatd
needs to run as root while it may be preferable to run users of the seat as
non-root user, it is not possible to add a dependency from other
systemd.services to the seatd.service. Therefore, the seatd.service always
needs to run on such systems even if no other process actually needs it.

Socket activation allows to start the seatd service only when some other
process tries to connect to the socket.

Michael

Michael Tretter (2):
  seatd: Initialize server before removing socket
  seatd: Add support for systemd.socket activation

 contrib/systemd/seatd.socket |  9 +++++++
 meson.build                  |  9 ++++++-
 seatd/seatd.c                | 51 +++++++++++++++++++++++++++---------
 3 files changed, 56 insertions(+), 13 deletions(-)
 create mode 100644 contrib/systemd/seatd.socket

--
2.30.2

[PATCH 1/2] seatd: Initialize server before removing socket

Michael Tretter <m.tretter@pengutronix.de>
Details
Message ID
<20230127104421.4075465-2-m.tretter@pengutronix.de>
In-Reply-To
<20230127104421.4075465-1-m.tretter@pengutronix.de> (view parent)
DKIM signature
missing
Download raw message
Patch: +10 -10
Removing an existing socket is a preparation for creating a new socket.
Initializing the service between removing a left-over socket and
creating a new socket makes error handling difficult.

This is a preparation for adding systemd socket activation.
---
 seatd/seatd.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/seatd/seatd.c b/seatd/seatd.c
index f88e6c96aa71..bc200f0587dd 100644
--- a/seatd/seatd.c
+++ b/seatd/seatd.c
@@ -152,33 +152,33 @@ int main(int argc, char *argv[]) {
	log_init();
	libseat_set_log_level(level);

	struct server server = {0};
	if (server_init(&server) == -1) {
		log_errorf("server_init failed: %s", strerror(errno));
		return 1;
	}

	int ret = 1;
	struct stat st;
	if (lstat(SEATD_DEFAULTPATH, &st) == 0) {
		if (!S_ISSOCK(st.st_mode)) {
			log_errorf("Non-socket file found at socket path %s, refusing to start",
				   SEATD_DEFAULTPATH);
			return 1;
			goto error_server;
		} else if (!unlink_existing_socket) {
			log_errorf("Socket file found at socket path %s, refusing to start",
				   SEATD_DEFAULTPATH);
			return 1;
			goto error_server;
		} else {
			// We only do this if the socket path is not user specified
			log_infof("Removing leftover socket at %s", SEATD_DEFAULTPATH);
			if (unlink(SEATD_DEFAULTPATH) == -1) {
				log_errorf("Could not remove leftover socket: %s", strerror(errno));
				return 1;
				goto error_server;
			}
		}
	}

	struct server server = {0};
	if (server_init(&server) == -1) {
		log_errorf("server_init failed: %s", strerror(errno));
		return 1;
	}

	int ret = 1;
	int socket_fd = open_socket(SEATD_DEFAULTPATH, uid, gid);
	if (socket_fd == -1) {
		log_error("Could not create server socket");
-- 
2.30.2

[PATCH 2/2] seatd: Add support for systemd.socket activation

Michael Tretter <m.tretter@pengutronix.de>
Details
Message ID
<20230127104421.4075465-3-m.tretter@pengutronix.de>
In-Reply-To
<20230127104421.4075465-1-m.tretter@pengutronix.de> (view parent)
DKIM signature
missing
Download raw message
Patch: +46 -3
Allow to start seatd on demand using systemd.socket activation. This is
especially helpful to start a seatd service as root user only if a user
starts an application that needs a seat.
---
 contrib/systemd/seatd.socket |  9 +++++++++
 meson.build                  |  9 ++++++++-
 seatd/seatd.c                | 31 +++++++++++++++++++++++++++++--
 3 files changed, 46 insertions(+), 3 deletions(-)
 create mode 100644 contrib/systemd/seatd.socket

diff --git a/contrib/systemd/seatd.socket b/contrib/systemd/seatd.socket
new file mode 100644
index 000000000000..e922def28ef0
--- /dev/null
+++ b/contrib/systemd/seatd.socket
@@ -0,0 +1,9 @@
[Unit]
Description=Seatd Socket
Before=sockets.target

[Socket]
ListenStream=%t/seatd.sock

[Install]
WantedBy=sockets.target
diff --git a/meson.build b/meson.build
index 661b39ab2c9b..5da8bb1b55eb 100644
--- a/meson.build
+++ b/meson.build
@@ -215,12 +215,19 @@ libseat = declare_dependency(
meson.override_dependency('libseat', libseat)

if with_server
	libsystemd = dependency('libsystemd', required: false)
	seatd_c_args = []
	if libsystemd.found()
		seatd_c_args += '-DHAVE_LIBSYSTEMD'
	endif

	executable(
		'seatd',
		[ server_files, 'seatd/seatd.c' ],
		include_directories: [include_directories('.', 'include')],
		install: true,
		dependencies: [realtime],
		dependencies: [realtime, libsystemd],
		c_args: seatd_c_args,
	)
	executable(
		'seatd-launch',
diff --git a/seatd/seatd.c b/seatd/seatd.c
index bc200f0587dd..e4dcb7878c23 100644
--- a/seatd/seatd.c
+++ b/seatd/seatd.c
@@ -10,6 +10,10 @@
#include <sys/stat.h>
#include <sys/un.h>

#if defined(HAVE_LIBSYSTEMD)
#include <systemd/sd-daemon.h>
#endif

#include "client.h"
#include "log.h"
#include "poller.h"
@@ -17,6 +21,26 @@

#define LISTEN_BACKLOG 16

#if defined(HAVE_LIBSYSTEMD)
static int accept_socket(const char *path)
{
	int n = sd_listen_fds(0);
	int i;
	for (i = 0; i < n; ++i) {
		if (sd_is_socket_unix(SD_LISTEN_FDS_START + i, SOCK_STREAM, 1, path, 0) > 0)
			return SD_LISTEN_FDS_START + i;
	}

	return -1;
}
#else
static inline int accept_socket(const char *path)
{
	(void) path;
	return -1;
}
#endif

static int open_socket(const char *path, int uid, int gid) {
	union {
		struct sockaddr_un unix;
@@ -158,6 +182,8 @@ int main(int argc, char *argv[]) {
		return 1;
	}

	int socket_fd = accept_socket(SEATD_DEFAULTPATH);

	int ret = 1;
	struct stat st;
	if (lstat(SEATD_DEFAULTPATH, &st) == 0) {
@@ -169,7 +195,7 @@ int main(int argc, char *argv[]) {
			log_errorf("Socket file found at socket path %s, refusing to start",
				   SEATD_DEFAULTPATH);
			goto error_server;
		} else {
		} else if (socket_fd == -1) {
			// We only do this if the socket path is not user specified
			log_infof("Removing leftover socket at %s", SEATD_DEFAULTPATH);
			if (unlink(SEATD_DEFAULTPATH) == -1) {
@@ -179,7 +205,8 @@ int main(int argc, char *argv[]) {
		}
	}

	int socket_fd = open_socket(SEATD_DEFAULTPATH, uid, gid);
	if (socket_fd == -1)
		socket_fd = open_socket(SEATD_DEFAULTPATH, uid, gid);
	if (socket_fd == -1) {
		log_error("Could not create server socket");
		goto error_server;
-- 
2.30.2
Details
Message ID
<Q5Y7PR.8D4Q26FZPTUT3@kl.wtf>
In-Reply-To
<20230127104421.4075465-1-m.tretter@pengutronix.de> (view parent)
DKIM signature
missing
Download raw message
I wonder if we could do this in a non-systemd-specific manner, e.g. 
with a command-line argument that $LISTEN_FDS can be passed to.

What is the main intended use-case? I imagine a system with seatd set 
to start on boot will generally be a graphical terminal where seatd is 
always needed.
Michael Tretter <m.tretter@pengutronix.de>
Details
Message ID
<20230210162424.GA29504@pengutronix.de>
In-Reply-To
<Q5Y7PR.8D4Q26FZPTUT3@kl.wtf> (view parent)
DKIM signature
missing
Download raw message
On Sun, 29 Jan 2023 00:20:14 +0100, Kenny Levinsen wrote:
> I wonder if we could do this in a non-systemd-specific manner, e.g. with a
> command-line argument that $LISTEN_FDS can be passed to.

Do you want to avoid the systemd dependency in the implementation or do you
want da different interface for the caller?

I would like to start seatd with the systemd-socket-activation and would
prefer make it as easy as possible from systemd to do so.

> 
> What is the main intended use-case? I imagine a system with seatd set to
> start on boot will generally be a graphical terminal where seatd is always
> needed.

It is meant as an alternative to seatd-launch to avoid the suid logic when
starting a (Weston) compositor and allow to simplify the user services.
Otherwise, if seatd is started as a system service, the user services would
always need to depend on the multi-user target to make sure that seatd is
running, as user services cannot depend on system services.

The systemd-socket-activation would allow to start the user session and a
graphical application earlier than finishing the multi-user target.

Michael
Details
Message ID
<-mCgl_cQuhku-Y-scY2I_LmaVvUzuEyhTXPUpN7bpJWZZSIR3RFi2lTscTsZtJhEHX1QCSMnpfw7wFrFPhRXIrB5a1Cyvp8ey0bnQEi0blI=@emersion.fr>
In-Reply-To
<20230210162424.GA29504@pengutronix.de> (view parent)
DKIM signature
missing
Download raw message
On Friday, February 10th, 2023 at 17:24, Michael Tretter <m.tretter@pengutronix.de> wrote:

> On Sun, 29 Jan 2023 00:20:14 +0100, Kenny Levinsen wrote:
> 
> > I wonder if we could do this in a non-systemd-specific manner, e.g. with a
> > command-line argument that $LISTEN_FDS can be passed to.
> 
> Do you want to avoid the systemd dependency in the implementation or do you
> want da different interface for the caller?
> 
> I would like to start seatd with the systemd-socket-activation and would
> prefer make it as easy as possible from systemd to do so.

IMHO adding `--listen-fd=$LISTEN_FDS` to the unit's command isn't too
difficult. Also it only needs to be done by the package maintainer.

> > What is the main intended use-case? I imagine a system with seatd set to
> > start on boot will generally be a graphical terminal where seatd is always
> > needed.
> 
> It is meant as an alternative to seatd-launch to avoid the suid logic when
> starting a (Weston) compositor and allow to simplify the user services.
> Otherwise, if seatd is started as a system service, the user services would
> always need to depend on the multi-user target to make sure that seatd is
> running, as user services cannot depend on system services.
> 
> The systemd-socket-activation would allow to start the user session and a
> graphical application earlier than finishing the multi-user target.

When running with a service supervisor, please don't use seatd-launch.
seatd-launch is only intended for simple one-off use-cases.

What stops you from writing a seatd unit? Do you need to know when seatd
is ready to accept client connections?

Are you aware of the -n flag? It should be possible to integrate with
systemd, perhaps with a small patch to write READY=1 instead of just a
newline.

In any case, I think a flag to specify a listen FD could make sense.
Michael Tretter <m.tretter@pengutronix.de>
Details
Message ID
<20230213080845.GB20859@pengutronix.de>
In-Reply-To
<-mCgl_cQuhku-Y-scY2I_LmaVvUzuEyhTXPUpN7bpJWZZSIR3RFi2lTscTsZtJhEHX1QCSMnpfw7wFrFPhRXIrB5a1Cyvp8ey0bnQEi0blI=@emersion.fr> (view parent)
DKIM signature
missing
Download raw message
On Fri, 10 Feb 2023 17:02:52 +0000, Simon Ser wrote:
> On Friday, February 10th, 2023 at 17:24, Michael Tretter <m.tretter@pengutronix.de> wrote:
> 
> > On Sun, 29 Jan 2023 00:20:14 +0100, Kenny Levinsen wrote:
> > 
> > > I wonder if we could do this in a non-systemd-specific manner, e.g. with a
> > > command-line argument that $LISTEN_FDS can be passed to.
> > 
> > Do you want to avoid the systemd dependency in the implementation or do you
> > want da different interface for the caller?
> > 
> > I would like to start seatd with the systemd-socket-activation and would
> > prefer make it as easy as possible from systemd to do so.
> 
> IMHO adding `--listen-fd=$LISTEN_FDS` to the unit's command isn't too
> difficult. Also it only needs to be done by the package maintainer.

Agreed, but it would be even easier for the package maintainer, if nothing has
to be done at all:) Anyway, I will implement it without systemd and pass the
listen fd with a command-line argument.

> 
> > > What is the main intended use-case? I imagine a system with seatd set to
> > > start on boot will generally be a graphical terminal where seatd is always
> > > needed.
> > 
> > It is meant as an alternative to seatd-launch to avoid the suid logic when
> > starting a (Weston) compositor and allow to simplify the user services.
> > Otherwise, if seatd is started as a system service, the user services would
> > always need to depend on the multi-user target to make sure that seatd is
> > running, as user services cannot depend on system services.
> > 
> > The systemd-socket-activation would allow to start the user session and a
> > graphical application earlier than finishing the multi-user target.
> 
> When running with a service supervisor, please don't use seatd-launch.
> seatd-launch is only intended for simple one-off use-cases.
> 
> What stops you from writing a seatd unit? Do you need to know when seatd
> is ready to accept client connections?

I am already using a seatd unit (basically the one that is already provided in
the repository), but the issue is specifying the dependency between a systemd
_user_ unit (for the application) and the systemd _system_ unit for seatd.

> 
> Are you aware of the -n flag? It should be possible to integrate with
> systemd, perhaps with a small patch to write READY=1 instead of just a
> newline.

That could be added to the example systemd unit, but it only works between
systemd system units. And with socket activation, it's not even needed.

> 
> In any case, I think a flag to specify a listen FD could make sense.
> 
Reply to thread Export thread (mbox)