[PATCH Weston 1/1] desktop-shell: implement autolaunch

[Pekka Paalanen]

On Fri, 24 Oct 2014 10:28:57 +0100
Daniel Stone <daniel at fooishbar.org> wrote:

> Hi,
> 
> On 22 October 2014 14:53, Pekka Paalanen <pekka.paalanen at collabora.co.uk>
> wrote:
> 
> > +       pid = fork();
> > +       if (pid < 0) {
> > +               fprintf(stderr, "fork failed: %m\n");
> > +               goto out;
> > +       }
> > +
> > +       if (pid)
> > +               goto out;
> > +
> > +       argvpp = argv.data;
> > +       if (execve(argvpp[0], argvpp, envp.data) < 0) {
> > +               fprintf(stderr, "execl '%s' failed: %m\n", argvpp[0]);
> > +               exit(1);
> > +       }
> >
> 
> Hmm. Can we please use weston_client_start here instead of open-coding it?

weston_client_start() does not support passing in environment
explicitly. It also automatically sets up WAYLAND_SOCKET environment
variable and creates a connection (wl_client) before the program even
starts.

I do not think it makes sense to use weston_client_start() here,
because whatever we launch here, might not be a single-shot Wayland
client. Especially in the script case mentioned below, it would
fail all restart attempts in the script as WAYLAND_SOCKET would be set
to a disconnected/invalid fd.

> And, while you're at it - as this was written for kiosk mode, it spawns a
> shell script which just restarts the video player in a loop. Can we please
> add an autostart param to weston_client_run/start (as a new enum with
> WESTON_CLIENT_RESTART, not bool) and to the config (as a bool) which lifts
> most of
> desktop_shell_client_destroy/check_desktop_shell_crash_too_early/respawn_desktop_shell_process?

I'm not sure that's feasible. Watching the process exit is not used to
trigger respawn for weston-desktop-shell, but losing the wl_client is.
These two events happen in arbitrary order, and we recently fixed a
related bug by exactly not respawning based on process exit. We want
weston-desktop-shell to respawn if the compositor disconnects it, even
if the actual process gets left behind due to malfunction.

Restart for autolaunch OTOH would be based on process exit rather than
losing the connection.

IOW, weston_client_start() and the existing respawn mechanism are
intended for special purpose known clients, while the panel
launchers and the autolaunch feature are meant for running arbitrary
programs (that might not even be clients themselves/directly or at all).

I'm not sure if adding restart to autolaunch is in scope... there are
many aspects to configure for restart (delays, when to give up)
so I'd rather maybe keep with the script. Or systemd user session.
After all, the autolaunch is just a quick'n'useful hack when no session
manager (systemd or other) is available.


Thanks,
pq

> Aside from that, and splitting refactor / autorestart code move / autorun
> feature into separate patches, this looks good to me.
> 
> Cheers,
> Daniel