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

[Friedrich, Eugen (ADITG/SW1)]

> -----Original Message-----
> From: Pekka Paalanen [mailto:ppaalanen at gmail.com]
> Sent: Dienstag, 5. April 2016 13:08
> To: Friedrich, Eugen (ADITG/SW1)
> Cc: wayland mailing list; Natsume, Wataru (ADITJ/SWG); Ucan, Emre
> (ADITG/SW1)
> Subject: Re: [PATCH weston] systemd: take over sockets created by systemd
> 
> 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.htm
> l
> >
> > 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?

[EF] Hi Pekka,

Sure, I will incorporate the proposed changes from you and Bill Spitzak and send a v2 for this patch
> 
> 
> Thanks,
> pq