Bug 78701

Created attachment 99035 [details] [review]
[PATCH v3] Implement systemd socket activation

Note that the patch v3 needs the following work:

On src/pulsecore/socket-server.c::socket_server_free():
        unlink(s->filename);

When pulseaudio is socket activated, pulseaudio must not delete the
socket file when quitting ("systemctl restart") because systemd is still
listening on it, and although systemd would still be able to restart
pulseaudio and pass the socket fd, clients would not be able to connect
to it.

Created attachment 99336 [details] [review]
[PATCH v4] Implement systemd socket activation

Changes since v3:
  - do not unlink socket files created by systemd

Shouldn't start-pulseaudio-x11 be modified to not start pulseaudio when socket activation is enabled and running under systemd?

Also, you should probably generate pulseaudio.service with an AC_CONFIG_FILES directive (like start-pulseaudio-x11) and use PA_BINARY instead of @bindir@/pulseaudio.

(In reply to comment #4)
> Shouldn't start-pulseaudio-x11 be modified to not start pulseaudio when
> socket activation is enabled and running under systemd?
> 
> Also, you should probably generate pulseaudio.service with an
> AC_CONFIG_FILES directive (like start-pulseaudio-x11) and use PA_BINARY
> instead of @bindir@/pulseaudio.

AC_CONFIG_FILES doesn't work with @bindir@, because it substitutes it with ${exec_prefix}/bin, which then doesn't work. And systemd always requires an absolute path for executables.

I would prefer you to post patches to the list in the future, because giving comments is easier that way (and the set of potential reviewers is also larger). Attaching patches to bugs in addition to sending them to the mailing list is fine, though (it's easier for non-subscribed people to download the patch from a bug attachment than from the mailing list archive).

daemon/pulseaudio.service should be added to src/.gitignore.

(In reply to comment #5)
> AC_CONFIG_FILES doesn't work with @bindir@, because it substitutes it with
> ${exec_prefix}/bin, which then doesn't work. And systemd always requires an
> absolute path for executables.

Felipe's suggestion was to replace @bindir@/pulseaudio with @PA_BINARY@. Is there some reason why @PA_BINARY@ doesn't work?

Commit message:
> In the future this would allow to drop all the complex server location and
> autospawn code, and just rely on the user manager to do the right thing.

We support many platforms and not all of them have systemd, so dropping the existing server location and autospawning code won't be possible. Those could be disabled when systemd is available, though.

> v3: from Alban Crequy <alban.crequy@collabora.co.uk>
> ...
>   - pulseaudio.service with WantedBy left empty?

What are you trying to say with this? Are you wondering if empty WantedBy makes sense, but you don't dare to change it? I wonder what's the point of empty WantedBy too.

Makefile.am:
> -module_simple_protocol_tcp_la_CFLAGS = -DUSE_TCP_SOCKETS
>  -DUSE_PROTOCOL_SIMPLE $(AM_CFLAGS)
> +module_simple_protocol_tcp_la_CFLAGS = -DUSE_TCP_SOCKETS
>  -DUSE_PROTOCOL_SIMPLE $(SYSTEMD_DAEMON_CFLAGS) $(AM_CFLAGS)

Let's not try to support socket activation for anything else than module-native-protocol-unix at this point. Getting the standard user instance socket activation right is complex enough for one patch.

> -module_native_protocol_unix_la_CFLAGS = -DUSE_UNIX_SOCKETS
> -DUSE_PROTOCOL_NATIVE $(AM_CFLAGS)
> +module_native_protocol_unix_la_CFLAGS = -DUSE_UNIX_SOCKETS
> -DUSE_PROTOCOL_NATIVE $(SYSTEMD_DAEMON_CFLAGS) $(AM_CFLAGS)

The convention is to have $(AM_CFLAGS) before other CFLAGS variables. (There's probably some reason other than just consistency for that convention, but I can't give an example of what can break if the convention isn't followed.)

main.c:
> +    systemd_fds = sd_listen_fds(false);
> +    if (systemd_fds > 0 ||
> +        getenv("MANAGERPID") != NULL) {

Wouldn't it make more sense to only have the MANAGERPID check, and not call sd_listen_fds()? The fds aren't used anyway.

Also, a cosmetic note: the line break seems unnecessary, but if you want to have it, please indent the second line by one more level so that it's easier to see that it's not part of the if body.

pulseaudio.service.in:
> +Description=Pulseaudio daemon for the user session

s/Pulseaudio/PulseAudio/

> +Type=dbus
> +BusName=org.pulseaudio.Server

We traditionally haven't been bus-activatable. Also, PulseAudio can be built without D-Bus support. Bus activation probably makes sense, but I would prefer to not enable it at this point (socket activation already provides enough things to worry about).

> +ExecStart=@bindir@/pulseaudio --start --daemonize=no --disallow-exit
>  --exit-idle-time=-1

The --start and --disallow-exit options feel weird. If they are used, there should be a comment explaining why.

If pulseaudio is only started by systemd, --start makes no sense. If pulseaudio is only started by systemd, --disallow-exit may make some sense.

The user may at least try to start pulseaudio manually, and we need to think about how this should interact with systemd.

If systemd has started pulseaudio, trying to start pulseaudio manually will fail in the pid file check, which is good.

If systemd has not started pulseaudio, but it has reserved the socket, what happens? Can you try? I'd guess that the start-up fails during the loading of module-native-protocol-unix. Is that what should happen? I guess it's ok, but it would be better if the manually started pulseaudio would detect that systemd has the pulseaudio service enabled (but not started) and fail to start with an informative error message before trying to load any modules.

If there's a manually started pulseaudio running, and systemd tries to acquire the socket, I suppose that make the socket unit status "failed". I don't see any dependency from pulseaudio.service to pulseaudio.socket - does systemd generate the dependency implicitly, or should you add the dependency to pulseaudio.service? If the socket acquisition fails, I think systemd should not try to start pulseaudio, but if the dependency to the socket has not been specified, I suppose "systemctl --user start pulseaudio" will try to start pulseaudio, and since it fails in the pid file check, and Restart=on-failure has been specified, systemd will try to start pulseaudio again and again in a loop (I hope there's a limit how many times systemd tries to restart a failed service).

If there's a manually started pulseaudio running with non-standard configuration that allows systemd to acquire the socket, then when a client tries to connect to the socket (or "systemctl --user start pulseaudio" is run), systemd tries to start pulseaudio and it fails in the pid file check. Again, systemd will try to restart pulseaudio in a loop. Is there some way for pulseaudio to tell systemd in this situation that "don't try to restart me"?

Now, some words about --disallow-exit. I guess that if pulseaudio has been started by systemd, it makes sense to only support shutdown via "systemctl --user stop pulseaudio". The user can still send SIGTERM to the daemon, but in this case systemd will restart pulseaudio. So, --disallow-exit seems sensible to have in ExecStart.

module-protocol-stub.c:
> +    inherited_fds = sd_listen_fds(true);

sd_listen_fds() should be called only if this particular module-protocol-stub instance is a native protocol module, unix variant, and is loaded in the role of "the standard user instance". For the last condition you'll probably need a new module argument, or maybe interpret string "user" specially when parsing the "socket" module argument.

Alban, do you plan to still work on the patch?

(In reply to comment #7)
> Alban, do you plan to still work on the patch?

I have less time to work on the patch at the moment... So if you or someone else would like to take over, it would be better.

Thanks for the review btw.

Ok, thanks for the information. It appears that there's some desire to have this in Tizen, so maybe someone from Tizen will continue the work, but no promises at this point.

Ooops, I should have read this bug before hacking on the same things!

I posted a separate patch to the ML to do this too. Mine doesn't do the IP activation yet however (I was lazy!), but as with this patch it's relatively trivial to add.

I opted for not creating a new function to adopt the socket and just checked when creating a new socket server if we happen to have a passed FD that matches via the sd_is_socket_*() function and avoid the bind+listen step. That seems cleaner to me that a separate adopt function, but perhaps that's just taste.

Some things slightly different in my patch:

1. I still set filename for unix sockets such that the address is properly calculated and thus exported in e.g. module-x11-publish. I use a separate variable inside the socket server to track whether it's been activated and avoid the unlink when closing.

2. I added an install-data-hook to automatically ship a static symlink to start the socket. This means that it users do not need to do any manual configuration

I'm marking this as closed since Colin's patch was merged for the 6.0 release.