[PATCH weston] systemd: take over a socket created by systemd in case of

[Friedrich, Eugen (ADITG/SW1)]

> -----Original Message-----
> From: Pekka Paalanen [mailto:ppaalanen at gmail.com]
> Sent: Friday, April 01, 2016 2:15 PM
> To: Friedrich, Eugen (ADITG/SW1)
> Cc: wayland mailing list; Natsume, Wataru (ADITJ/SWG); Ucan, Emre
> (ADITG/SW1)
> Subject: Re: [PATCH weston] systemd: take over a socket created by
> systemd in case of
> 
> On Wed, 30 Mar 2016 09:31:04 +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 socket was provided by
> > systemd and adds this as an additional socket to wayland display.
> > 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>
> > ---
> > src/systemd-notify.c |   12 ++++++++++++
> > 1 file changed, 12 insertions(+)
> >
> 
> Hi Eugen,
> 
> sorry, but it seems something has butchered the patch. All tabs have
> converted to spaces, there is an extra space in front of every context line,
> and there is some HTML garbage too.

[EF] should my automatic outlook settings ,will take care next time
> 
> The subject line seems cut short, would be fine to just remove the "in case
> of" too.
[EF] agree
> 
> The feature itself seems fine to me.
> 
> > diff --git a/src/systemd-notify.c b/src/systemd-notify.c index
> > e61db0f..a921241 100644
> > --- a/src/systemd-notify.c
> > +++ b/src/systemd-notify.c
> > @@ -79,6 +79,7 @@ module_init(struct weston_compositor *compositor,
> >                struct wl_event_loop *loop;
> >                long watchdog_time_conv;
> >                struct systemd_notifier *notifier;
> > +             int fd;
> >                 notifier = zalloc(sizeof *notifier);
> >                if (notifier == NULL)
> > @@ -89,6 +90,17 @@ module_init(struct weston_compositor *compositor,
> >                wl_signal_add(&compositor->destroy_signal,
> >
> > &notifier->compositor_destroy_listener);
> > +             /*take additional display socket if provided by systemd*/
> > +             if (1 == sd_listen_fds(0)) {
> 
> Shouldn't the argument be non-zero to clean up the environment, as
> Weston will launch child processes?
[EF] make sense, fs descriptors for child processes are not relevant, we should unset it
> 
> How about looping through all file descriptors received, instead of requiring
> exactly one to be ever defined?
[EF] did not saw the use-case before but basically it is a good idea, will add this
> 
> It would be good to use sd_is_socket(AF_UNIX, SOCK_STREAM,
> yes-listening) to check for a correct socket type.
[EF] good point, will add this
> 
> > +                             fd = SD_LISTEN_FDS_START + 0;
> > +                             weston_log("info:add socket for weston
> > + created by systemd\n");
> > +
> > +                             if (wl_display_add_socket_fd(compositor->wl_display, fd))
> {
> > +                                             weston_log("wl_display_add_socket_fd failed\n");
> > +                                             return -1;
> > +                             }
> > +             }
> > +
> >                sd_notify(0, "READY=1");
> >                 /* 'WATCHDOG_USEC' is environment variable that is set
> > --
> > 1.7.9.5
> >
> 
> I also seem to miss a bit of documentation. The systemd integration features
> were not really documented before either, but at least the ./configure --help
> talks only about notifications. Some words somewhere that this plugin
> supports systemd socket activation would be nice to add.
[EF] agree, will add
> 
> 
> Thanks,
> pq
[EF] basically you can just ignore this patch , I will create new one which will rework the findings
Best regards

Eugen Friedrich
Software Group I (ADITG/SW1)

Tel. +49 5121 49 6921