Ok, I split the patches and here's the
one doing the refactor, below.
Dan : the other patch (the one adding autolaunch per se) still needs a comment from you before I can update it. See below for the conversation, I'd like to have your comments about using/not using weston_client_start() and about adding an autostart param to it and to the config. thanks :) ***************** Some other desktop-shell bits will be (re)using most of panel_add_launcher code, so this patch extracts useful bits and pieces into a new function, parse_launcher_path. Signed-off-by: Frederic Plourde <frederic.plou...@collabora.co.uk> --- clients/desktop-shell.c | 38 ++++++++++++++++++++++++-------------- 1 file changed, 24 insertions(+), 14 deletions(-) diff --git a/clients/desktop-shell.c b/clients/desktop-shell.c index 961a9b2..c8e2810 100644 --- a/clients/desktop-shell.c +++ b/clients/desktop-shell.c @@ -597,72 +597,82 @@ load_icon_or_fallback(const char *icon) cairo_move_to(cr, 4, 16); cairo_line_to(cr, 16, 4); cairo_stroke(cr); cairo_destroy(cr); return surface; } static void -panel_add_launcher(struct panel *panel, const char *icon, const char *path) +parse_launcher_path(char *path, struct wl_array *envp_array, struct wl_array *argv_array) { - struct panel_launcher *launcher; char *start, *p, *eq, **ps; int i, j, k; - launcher = xzalloc(sizeof *launcher); - launcher->icon = load_icon_or_fallback(icon); - launcher->path = xstrdup(path); + struct wl_array *envp = envp_array; + struct wl_array *argv = argv_array; - wl_array_init(&launcher->envp); - wl_array_init(&launcher->argv); + wl_array_init(envp); + wl_array_init(argv); for (i = 0; environ[i]; i++) { - ps = wl_array_add(&launcher->envp, sizeof *ps); + ps = wl_array_add(envp, sizeof *ps); *ps = environ[i]; } j = 0; - start = launcher->path; + start = path; while (*start) { for (p = start, eq = NULL; *p && !isspace(*p); p++) if (*p == '=') eq = p; if (eq && j == 0) { - ps = launcher->envp.data; + ps = envp->data; for (k = 0; k < i; k++) if (strncmp(ps[k], start, eq - start) == 0) { ps[k] = start; break; } if (k == i) { - ps = wl_array_add(&launcher->envp, sizeof *ps); + ps = wl_array_add(envp, sizeof *ps); *ps = start; i++; } } else { - ps = wl_array_add(&launcher->argv, sizeof *ps); + ps = wl_array_add(argv, sizeof *ps); *ps = start; j++; } while (*p && isspace(*p)) *p++ = '\0'; start = p; } - ps = wl_array_add(&launcher->envp, sizeof *ps); + ps = wl_array_add(envp, sizeof *ps); *ps = NULL; - ps = wl_array_add(&launcher->argv, sizeof *ps); + ps = wl_array_add(argv, sizeof *ps); *ps = NULL; +} + +static void +panel_add_launcher(struct panel *panel, const char *icon, const char *path) +{ + struct panel_launcher *launcher; + + launcher = xzalloc(sizeof *launcher); + launcher->icon = load_icon_or_fallback(icon); + launcher->path = xstrdup(path); + + parse_launcher_path(launcher->path, &launcher->envp, &launcher->argv); launcher->panel = panel; wl_list_insert(panel->launcher_list.prev, &launcher->link); launcher->widget = widget_add_widget(panel->widget, launcher); widget_set_enter_handler(launcher->widget, panel_launcher_enter_handler); widget_set_leave_handler(launcher->widget, panel_launcher_leave_handler); widget_set_button_handler(launcher->widget, Principal Software engineer T :: (450) 415-0855 +++++++++++++ Open First ! We're hiring ! Our current opportunities can be found here: http://bit.ly/Collabora-Careers Visit Collabora on the Web at https://www.collabora.com On Fri, 24 Oct 2014 10:28:57 +0100 Daniel Stone <dan...@fooishbar.org> wrote:Hi, On 22 October 2014 14:53, Pekka Paalanen <pekka.paala...@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, pqAside from that, and splitting refactor / autorestart code move / autorun feature into separate patches, this looks good to me. Cheers, Daniel_______________________________________________ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel |
_______________________________________________ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel