[PATCH weston] compositor: systemd notifications support

[Giulio Camuffo]

2015-09-24 16:20 GMT+03:00 Pekka Paalanen <ppaalanen at gmail.com>:
> On Thu, 24 Sep 2015 15:53:53 +0300
> Giulio Camuffo <giuliocamuffo at gmail.com> wrote:
>
>> 2015-09-24 14:06 GMT+03:00 Egor Starkov <egor.starkov at ge.com>:
>> > Add systemd status and watchdog notification support.
>> > Feature is not compiled by default and can be enabled by
>> > "--enable-systemd-notify" configuration flag. Watchdog
>> > timeout equals to half of timeout defined by "WATCHDOG_USEC"
>> > environment variable, which is set by "WatchdogSec="
>> > setting in service file.
>>
>> Hi,
>>
>> I don't quite like that we're calling a systemd specific thing in the
>> main(), can't this be called by the logind launcher? Alternatively i
>> guess we could abstract this a way with a new weston_launcher_start()
>> or something like it.
>
> Hi,
>
> the launcher optionally uses logind, not systemd. Logind can exist
> without systemd, so conflating this patch with that doesn't seem
> appropriate either.
>
> We can't call it at the backend init time, because the listening
> sockets are created only afterwards. I suppose we could do it from the
> idle handler? But then we need to set up the idle handler somewhere.
>
> Or would it help if we named it weston_service_start()?
>
>> Besides currently if the notify support is built it will be called
>> even when running nested in X or another wayland compositor, right?
>> Using the launcher would avoid that.
>
> Yes, it will be called, and it's probably unneeded for the nested
> backends. It will also be called when starting weston manually instead
> from a service file, but sd_notify() should handle that just fine.
>
> There is an interesting scenario there. If host Weston is started from
> systemd service, I don't think it removes the systemd variables from
> the environment. If you then start a nested Weston in the same process
> hierarchy, it would think it's running on systemd.
>
> Hmm, should this be turned into a plugin instead?
>
> Making it a plugin seems like the most flexible choice for a user. And
> for packaging.

+1, making it a plugin is a nice idea.

>
>> >
>> > Signed-off-by: Egor Starkov <egor.starkov at ge.com>
>> > Reviewed-by: Pekka Paalanen <pekka.paalanen at collabora.co.uk>
>> > ---
>> >  Makefile.am          |  10 ++++-
>> >  configure.ac         |  13 ++++++
>> >  src/main.c           |   3 ++
>> >  src/systemd-notify.c | 120 +++++++++++++++++++++++++++++++++++++++++++++++++++
>> >  src/systemd-notify.h |  47 ++++++++++++++++++++
>> >  5 files changed, 192 insertions(+), 1 deletion(-)
>> >  create mode 100644 src/systemd-notify.c
>> >  create mode 100644 src/systemd-notify.h
>
>> > diff --git a/src/main.c b/src/main.c
>> > index a98570e..fd32751 100644
>> > --- a/src/main.c
>> > +++ b/src/main.c
>> > @@ -46,6 +46,7 @@
>> >  #include "../shared/helpers.h"
>> >  #include "git-version.h"
>> >  #include "version.h"
>> > +#include "systemd-notify.h"
>> >
>> >  static struct wl_list child_process_list;
>> >  static struct weston_compositor *segv_compositor;
>> > @@ -815,6 +816,8 @@ int main(int argc, char *argv[])
>> >
>> >         weston_compositor_wake(ec);
>> >
>> > +       weston_systemd_service_start(ec);
>> > +
>> >         wl_display_run(display);
>> >
>> >         /* Allow for setting return exit code after
>
>
> Thanks,
> pq