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,
-- 1.9.1

Frédéric Plourde

Principal Software engineer

 T :: (450) 415-0855
@:: frederic.plou...@collabora.co.uk

+++++++++++++

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 14-10-24 06:18 AM, Pekka Paalanen wrote:
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,
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
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

Reply via email to