On Fri, 24 Oct 2014 10:28:57 +0100 Daniel Stone <[email protected]> wrote:
> Hi, > > On 22 October 2014 14:53, Pekka Paalanen <[email protected]> > 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 _______________________________________________ wayland-devel mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/wayland-devel
