> -----Original Message----- > From: Pekka Paalanen [mailto:[email protected]] > 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)" <[email protected]> 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 <[email protected]> > > 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, > > ¬ifier->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 _______________________________________________ wayland-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/wayland-devel
