[pulseaudio-tickets] [Bug 78701] Implement systemd socket activation in pulseaudio

[bugzilla-daemon at freedesktop.org]

https://bugs.freedesktop.org/show_bug.cgi?id=78701

--- Comment #6 from Tanu Kaskinen <tanuk at iki.fi> ---
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 at . 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 at 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.

-- 
You are receiving this mail because:
You are the QA Contact for the bug.
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/pulseaudio-bugs/attachments/20140717/2bcd4da5/attachment.html>