[PATCH weston] systemd: take over sockets created by systemd

[Pekka Paalanen]

On Sun, 3 Apr 2016 20:18:02 +0000
"Friedrich, Eugen (ADITG/SW1)" <efriedrich at de.adit-jv.com> wrote:

> systemd provides a feature of socket-based activation, details in [1]
> This commit adds an implementation to check if sockets were provided by
> systemd and adds this as an additional socket to wayland display.
> before adding sockets are checked for the correctness:
> only AF_UNIX of type SOCK_STREAM are accepted
> 
> This is usefull for early rendering use-cases where weston and
> early-rendering-application can be started parallel.
> 
> [1] https://www.freedesktop.org/software/systemd/man/systemd.socket.html
> 
> Signed-off-by: Eugen Friedrich <efriedrich at de.adit-jv.com>

Hi Eugen,

the feature additions look good. Now it's only style comments from here
on. It compiles fine after an unrelated fix I am sending separately. It
also loads fine into Weston, but I did not test the socket activation
actually worked.

> ---
>  configure.ac         |  5 ++++-
>  src/systemd-notify.c | 27 +++++++++++++++++++++++++++
>  2 files changed, 31 insertions(+), 1 deletion(-)
>  mode change 100644 => 100755 src/systemd-notify.c
> 
> diff --git a/configure.ac b/configure.ac
> index 9e8115a..447cf6b 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -634,7 +634,10 @@ AC_ARG_ENABLE(systemd_notify,
>                AS_HELP_STRING([--enable-systemd-notify],
>                               [Enables systemd notifications to
>                                notify systemd about weston state
> -                              and update watchdog.]),,
> +                              and update watchdog.
> +                              Also sockets provided by systemd
> +                              in case of socket-base activation
> +                              are added to wayland display]),,
>                enable_systemd_notify=no)
>  AM_CONDITIONAL(SYSTEMD_NOTIFY_SUPPORT, test x$enable_systemd_notify = xyes)
>  if test "x$enable_systemd_notify" = "xyes"; then
> diff --git a/src/systemd-notify.c b/src/systemd-notify.c
> old mode 100644
> new mode 100755
> index e61db0f..0be1f6f
> --- a/src/systemd-notify.c
> +++ b/src/systemd-notify.c
> @@ -28,6 +28,7 @@
>  #include <errno.h>
>  #include <stdlib.h>
>  #include <systemd/sd-daemon.h>
> +#include <sys/socket.h>
>  #include <wayland-server.h>
>  #include "shared/helpers.h"
>  #include "shared/zalloc.h"
> @@ -79,6 +80,8 @@ module_init(struct weston_compositor *compositor,
>  	struct wl_event_loop *loop;
>  	long watchdog_time_conv;
>  	struct systemd_notifier *notifier;
> +	int fd;
> +	int systemd_socket_fds = 0;

Unnecessary initialization.

>  
>  	notifier = zalloc(sizeof *notifier);
>  	if (notifier == NULL)
> @@ -89,6 +92,30 @@ module_init(struct weston_compositor *compositor,
>  	wl_signal_add(&compositor->destroy_signal,
>  		      &notifier->compositor_destroy_listener);
>  

The whole following hunk could be put into a separate function to make
it stand out as its own block. It would clean up the variable scopes
and help reduce the indentation levels.

> +	/*take additional display sockets if provided by systemd*/

/* I would prefer spaces like this. */

> +	systemd_socket_fds = sd_listen_fds(1);
> +
> +	if (systemd_socket_fds > 0) {

In a separate function, this could be written as
	if (nr_fds <= 0)
		return;

Too bad at least my version of sd_listen_fds() manual does not say when
it could return an error instead of zero and whether we'd want to fail
Weston's launch in that case.

> +		int current_fd = 0;
> +
> +		for (;current_fd < systemd_socket_fds; current_fd++) {

Space after ;.

> +			fd = SD_LISTEN_FDS_START + current_fd;
> +
> +			if (sd_is_socket(fd, AF_UNIX, SOCK_STREAM,1) > 0) {
> +				if (wl_display_add_socket_fd(compositor->wl_display, fd)) {
> +					weston_log("wl_display_add_socket_fd failed\n");
> +					return -1;
> +				}
> +			} else {
> +				weston_log("invalid socket provided from systemd\n");
> +				return -1;
> +			}

Perhaps a bit easier to read would be:

	if (sd_is_socket(...) <= 0) {
		weston_log(complain...);
		return -1;
	}

	if (wl_display_add_socket_fd(...) < 0) {
		complain...
		return
	}

> +		}
> +
> +		weston_log("info: add %d socket(s) provided by systemd\n",
> +				current_fd);
> +	}
> +
>  	sd_notify(0, "READY=1");
>  
>  	/* 'WATCHDOG_USEC' is environment variable that is set

Would you like to revise one more time, please?


Thanks,
pq
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 811 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/wayland-devel/attachments/20160405/755f7322/attachment.sig>