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

Submitted by Pekka Paalanen on Oct. 22, 2014, 1:53 p.m.

Details

Message ID 1413986035-9282-1-git-send-email-pekka.paalanen@collabora.co.uk
State Rejected
Headers show

Not browsing as part of any series.

Commit Message

Pekka Paalanen Oct. 22, 2014, 1:53 p.m.
Process a new section 'autolaunch' from weston.ini, launching all
programs given there on desktop start-up.

[Frederic Plourde: cut redundancy between do_autolaunch and panel_add_launcher]
---
 clients/desktop-shell.c | 97 ++++++++++++++++++++++++++++++++++++++++++-------
 man/weston.ini.man      | 17 +++++++++
 weston.ini.in           |  3 ++
 3 files changed, 103 insertions(+), 14 deletions(-)

Patch hide | download patch | download mbox

diff --git a/clients/desktop-shell.c b/clients/desktop-shell.c
index 961a9b2..a964094 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,
@@ -1316,20 +1326,77 @@  panel_add_launchers(struct panel *panel, struct desktop *desktop)
 	}
 
 	if (count == 0) {
 		/* add default launcher */
 		panel_add_launcher(panel,
 				   DATADIR "/weston/terminal.png",
 				   BINDIR "/weston-terminal");
 	}
 }
 
+static void
+do_autolaunch(const char *path_arg)
+{
+	struct wl_array envp;
+	struct wl_array argv;
+	pid_t pid;
+	char **argvpp;
+	char *path;
+
+	path = xstrdup(path_arg);
+
+	parse_launcher_path(path, &envp, &argv);
+
+	/* panel_launcher_activate */
+
+	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);
+	}
+
+out:
+	wl_array_release(&argv);
+	wl_array_release(&envp);
+	free(path);
+}
+
+static void
+process_autolaunch(struct desktop *desktop)
+{
+	struct weston_config_section *s;
+	char *path;
+	const char *name;
+
+	s = NULL;
+	while (weston_config_next_section(desktop->config, &s, &name)) {
+		if (strcmp(name, "autolaunch") != 0)
+			continue;
+
+		weston_config_section_get_string(s, "path", &path, NULL);
+		if (!path)
+			continue;
+
+		do_autolaunch(path);
+		free(path);
+	}
+}
+
 int main(int argc, char *argv[])
 {
 	struct desktop desktop = { 0 };
 	struct output *output;
 	struct weston_config_section *s;
 
 	desktop.unlock_task.run = unlock_dialog_finish;
 	wl_list_init(&desktop.outputs);
 
 	desktop.config = weston_config_parse("weston.ini");
@@ -1349,20 +1416,22 @@  int main(int argc, char *argv[])
 	/* Create panel and background for outputs processed before the shell
 	 * global interface was processed */
 	wl_list_for_each(output, &desktop.outputs, link)
 		if (!output->panel)
 			output_init(output, &desktop);
 
 	grab_surface_create(&desktop);
 
 	signal(SIGCHLD, sigchild_handler);
 
+	process_autolaunch(&desktop);
+
 	display_run(desktop.display);
 
 	/* Cleanup */
 	grab_surface_destroy(&desktop);
 	desktop_destroy_outputs(&desktop);
 	if (desktop.unlock_dialog)
 		unlock_dialog_destroy(desktop.unlock_dialog);
 	desktop_shell_destroy(desktop.shell);
 	display_destroy(desktop.display);
 
diff --git a/man/weston.ini.man b/man/weston.ini.man
index c05a221..365141c 100644
--- a/man/weston.ini.man
+++ b/man/weston.ini.man
@@ -66,20 +66,21 @@  Comment lines are ignored:
 .RE
 .PP
 The section headers are:
 .PP
 .RS 4
 .nf
 .BR "core           " "The core modules"
 .BR "libinput       " "Input device configuration"
 .BR "shell          " "Desktop customization"
 .BR "launcher       " "Add launcher to the panel"
+.BR "autolaunch     " "Launch program automatically on startup"
 .BR "screensaver    " "Screensaver selection"
 .BR "output         " "Output configuration"
 .BR "input-method   " "Onscreen keyboard input"
 .BR "keyboard       " "Keyboard layouts"
 .BR "terminal       " "Terminal application options"
 .BR "xwayland       " "XWayland options"
 .BR "screen-share   " "Screen sharing options"
 .fi
 .RE
 .PP
@@ -272,20 +273,36 @@  sets the path to the program that is run by clicking on this launcher (string).
 It is possible to pass arguments and environment variables to the program. For
 example:
 .nf
 .in +4n
 
 path=GDK_BACKEND=wayland gnome-terminal --full-screen
 .in
 .fi
 .PP
 .RE
+.SH "AUTOLAUNCH SECTION"
+There can be multiple autolaunch sections for starting multiple programs.
+.TP 7
+.BI "path=" program
+sets the path (string) to the program that is run automatically when the
+desktop is starting.
+It is possible to pass arguments and environment variables to the program. For
+example:
+.nf
+.in +4n
+
+path=GDK_BACKEND=wayland gnome-terminal --full-screen
+.in
+.fi
+.PP
+.RE
 .SH "SCREENSAVER SECTION"
 The
 .B screensaver
 section is used to select and schedule a screensaver.
 The
 .B screensaver
 section is optional, as are all of the entries that may be specified in
 it.
 .TP 7
 .BI "path=" /usr/libexec/weston-screensaver
diff --git a/weston.ini.in b/weston.ini.in
index 4fca0bb..b0cb31f 100644
--- a/weston.ini.in
+++ b/weston.ini.in
@@ -30,20 +30,23 @@  icon=/usr/share/icons/gnome/24x24/apps/utilities-terminal.png
 path=@bindir@/weston-terminal
 
 [launcher]
 icon=/usr/share/icons/hicolor/24x24/apps/google-chrome.png
 path=/usr/bin/google-chrome
 
 [launcher]
 icon=/usr/share/icons/gnome/24x24/apps/arts.png
 path=@abs_top_builddir@/weston-flower
 
+#[autolaunch]
+#path=@bindir@/weston-terminal
+
 [screensaver]
 # Comment path to disable screensaver
 path=@libexecdir@/weston-screensaver
 duration=600
 
 [input-method]
 path=@libexecdir@/weston-keyboard
 
 #[output]
 #name=LVDS1

Comments

I'd prefer to see the refactor and the new feature in separate patches,
but this is pretty trivial.

I also have a slight preference for exit(EXIT_FAILURE), which is already
used somewhere else in that file - though there's also precedent for
exit(1), so you make the call.  :)

I'd not seen printf's %m until today - do we want to depend on a gnuism?
 I've seen at least some activity towards a freebsd port - I don't
believe %m is supported there?


That said, it runs nicely here and does what it says on the tin...

Reviewed-by: Derek Foreman <derekf@osg.samsung.com>

On 22/10/14 08:53 AM, Pekka Paalanen wrote:
> Process a new section 'autolaunch' from weston.ini, launching all
> programs given there on desktop start-up.
> 
> [Frederic Plourde: cut redundancy between do_autolaunch and panel_add_launcher]
> ---
>  clients/desktop-shell.c | 97 ++++++++++++++++++++++++++++++++++++++++++-------
>  man/weston.ini.man      | 17 +++++++++
>  weston.ini.in           |  3 ++
>  3 files changed, 103 insertions(+), 14 deletions(-)
> 
> diff --git a/clients/desktop-shell.c b/clients/desktop-shell.c
> index 961a9b2..a964094 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,
> @@ -1316,20 +1326,77 @@ panel_add_launchers(struct panel *panel, struct desktop *desktop)
>  	}
>  
>  	if (count == 0) {
>  		/* add default launcher */
>  		panel_add_launcher(panel,
>  				   DATADIR "/weston/terminal.png",
>  				   BINDIR "/weston-terminal");
>  	}
>  }
>  
> +static void
> +do_autolaunch(const char *path_arg)
> +{
> +	struct wl_array envp;
> +	struct wl_array argv;
> +	pid_t pid;
> +	char **argvpp;
> +	char *path;
> +
> +	path = xstrdup(path_arg);
> +
> +	parse_launcher_path(path, &envp, &argv);
> +
> +	/* panel_launcher_activate */
> +
> +	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);
> +	}
> +
> +out:
> +	wl_array_release(&argv);
> +	wl_array_release(&envp);
> +	free(path);
> +}
> +
> +static void
> +process_autolaunch(struct desktop *desktop)
> +{
> +	struct weston_config_section *s;
> +	char *path;
> +	const char *name;
> +
> +	s = NULL;
> +	while (weston_config_next_section(desktop->config, &s, &name)) {
> +		if (strcmp(name, "autolaunch") != 0)
> +			continue;
> +
> +		weston_config_section_get_string(s, "path", &path, NULL);
> +		if (!path)
> +			continue;
> +
> +		do_autolaunch(path);
> +		free(path);
> +	}
> +}
> +
>  int main(int argc, char *argv[])
>  {
>  	struct desktop desktop = { 0 };
>  	struct output *output;
>  	struct weston_config_section *s;
>  
>  	desktop.unlock_task.run = unlock_dialog_finish;
>  	wl_list_init(&desktop.outputs);
>  
>  	desktop.config = weston_config_parse("weston.ini");
> @@ -1349,20 +1416,22 @@ int main(int argc, char *argv[])
>  	/* Create panel and background for outputs processed before the shell
>  	 * global interface was processed */
>  	wl_list_for_each(output, &desktop.outputs, link)
>  		if (!output->panel)
>  			output_init(output, &desktop);
>  
>  	grab_surface_create(&desktop);
>  
>  	signal(SIGCHLD, sigchild_handler);
>  
> +	process_autolaunch(&desktop);
> +
>  	display_run(desktop.display);
>  
>  	/* Cleanup */
>  	grab_surface_destroy(&desktop);
>  	desktop_destroy_outputs(&desktop);
>  	if (desktop.unlock_dialog)
>  		unlock_dialog_destroy(desktop.unlock_dialog);
>  	desktop_shell_destroy(desktop.shell);
>  	display_destroy(desktop.display);
>  
> diff --git a/man/weston.ini.man b/man/weston.ini.man
> index c05a221..365141c 100644
> --- a/man/weston.ini.man
> +++ b/man/weston.ini.man
> @@ -66,20 +66,21 @@ Comment lines are ignored:
>  .RE
>  .PP
>  The section headers are:
>  .PP
>  .RS 4
>  .nf
>  .BR "core           " "The core modules"
>  .BR "libinput       " "Input device configuration"
>  .BR "shell          " "Desktop customization"
>  .BR "launcher       " "Add launcher to the panel"
> +.BR "autolaunch     " "Launch program automatically on startup"
>  .BR "screensaver    " "Screensaver selection"
>  .BR "output         " "Output configuration"
>  .BR "input-method   " "Onscreen keyboard input"
>  .BR "keyboard       " "Keyboard layouts"
>  .BR "terminal       " "Terminal application options"
>  .BR "xwayland       " "XWayland options"
>  .BR "screen-share   " "Screen sharing options"
>  .fi
>  .RE
>  .PP
> @@ -272,20 +273,36 @@ sets the path to the program that is run by clicking on this launcher (string).
>  It is possible to pass arguments and environment variables to the program. For
>  example:
>  .nf
>  .in +4n
>  
>  path=GDK_BACKEND=wayland gnome-terminal --full-screen
>  .in
>  .fi
>  .PP
>  .RE
> +.SH "AUTOLAUNCH SECTION"
> +There can be multiple autolaunch sections for starting multiple programs.
> +.TP 7
> +.BI "path=" program
> +sets the path (string) to the program that is run automatically when the
> +desktop is starting.
> +It is possible to pass arguments and environment variables to the program. For
> +example:
> +.nf
> +.in +4n
> +
> +path=GDK_BACKEND=wayland gnome-terminal --full-screen
> +.in
> +.fi
> +.PP
> +.RE
>  .SH "SCREENSAVER SECTION"
>  The
>  .B screensaver
>  section is used to select and schedule a screensaver.
>  The
>  .B screensaver
>  section is optional, as are all of the entries that may be specified in
>  it.
>  .TP 7
>  .BI "path=" /usr/libexec/weston-screensaver
> diff --git a/weston.ini.in b/weston.ini.in
> index 4fca0bb..b0cb31f 100644
> --- a/weston.ini.in
> +++ b/weston.ini.in
> @@ -30,20 +30,23 @@ icon=/usr/share/icons/gnome/24x24/apps/utilities-terminal.png
>  path=@bindir@/weston-terminal
>  
>  [launcher]
>  icon=/usr/share/icons/hicolor/24x24/apps/google-chrome.png
>  path=/usr/bin/google-chrome
>  
>  [launcher]
>  icon=/usr/share/icons/gnome/24x24/apps/arts.png
>  path=@abs_top_builddir@/weston-flower
>  
> +#[autolaunch]
> +#path=@bindir@/weston-terminal
> +
>  [screensaver]
>  # Comment path to disable screensaver
>  path=@libexecdir@/weston-screensaver
>  duration=600
>  
>  [input-method]
>  path=@libexecdir@/weston-keyboard
>  
>  #[output]
>  #name=LVDS1
>
The %m from glibc would indeed be a portability problem. However, it is already lightly used within wayland (11 occurrences) and heavily in weston (125 occurrences). I suggest you keep them for now, then clean them all up in one patch later - assuming the wayland community and prospective users consider portability a worthwhile effort.

Cheers, Karsten

Am 22.10.2014 um 17:57 schrieb Derek Foreman <derekf@osg.samsung.com>:

> I'd prefer to see the refactor and the new feature in separate patches,
> but this is pretty trivial.
> 
> I also have a slight preference for exit(EXIT_FAILURE), which is already
> used somewhere else in that file - though there's also precedent for
> exit(1), so you make the call.  :)
> 
> I'd not seen printf's %m until today - do we want to depend on a gnuism?
> I've seen at least some activity towards a freebsd port - I don't
> believe %m is supported there?
> 
> 
> That said, it runs nicely here and does what it says on the tin...
> 
> Reviewed-by: Derek Foreman <derekf@osg.samsung.com>
> 
> On 22/10/14 08:53 AM, Pekka Paalanen wrote:
>> Process a new section 'autolaunch' from weston.ini, launching all
>> programs given there on desktop start-up.
>> 
>> [Frederic Plourde: cut redundancy between do_autolaunch and panel_add_launcher]
>> ---
>> clients/desktop-shell.c | 97 ++++++++++++++++++++++++++++++++++++++++++-------
>> man/weston.ini.man      | 17 +++++++++
>> weston.ini.in           |  3 ++
>> 3 files changed, 103 insertions(+), 14 deletions(-)
>> 
>> diff --git a/clients/desktop-shell.c b/clients/desktop-shell.c
>> index 961a9b2..a964094 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,
>> @@ -1316,20 +1326,77 @@ panel_add_launchers(struct panel *panel, struct desktop *desktop)
>> 	}
>> 
>> 	if (count == 0) {
>> 		/* add default launcher */
>> 		panel_add_launcher(panel,
>> 				   DATADIR "/weston/terminal.png",
>> 				   BINDIR "/weston-terminal");
>> 	}
>> }
>> 
>> +static void
>> +do_autolaunch(const char *path_arg)
>> +{
>> +	struct wl_array envp;
>> +	struct wl_array argv;
>> +	pid_t pid;
>> +	char **argvpp;
>> +	char *path;
>> +
>> +	path = xstrdup(path_arg);
>> +
>> +	parse_launcher_path(path, &envp, &argv);
>> +
>> +	/* panel_launcher_activate */
>> +
>> +	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);
>> +	}
>> +
>> +out:
>> +	wl_array_release(&argv);
>> +	wl_array_release(&envp);
>> +	free(path);
>> +}
>> +
>> +static void
>> +process_autolaunch(struct desktop *desktop)
>> +{
>> +	struct weston_config_section *s;
>> +	char *path;
>> +	const char *name;
>> +
>> +	s = NULL;
>> +	while (weston_config_next_section(desktop->config, &s, &name)) {
>> +		if (strcmp(name, "autolaunch") != 0)
>> +			continue;
>> +
>> +		weston_config_section_get_string(s, "path", &path, NULL);
>> +		if (!path)
>> +			continue;
>> +
>> +		do_autolaunch(path);
>> +		free(path);
>> +	}
>> +}
>> +
>> int main(int argc, char *argv[])
>> {
>> 	struct desktop desktop = { 0 };
>> 	struct output *output;
>> 	struct weston_config_section *s;
>> 
>> 	desktop.unlock_task.run = unlock_dialog_finish;
>> 	wl_list_init(&desktop.outputs);
>> 
>> 	desktop.config = weston_config_parse("weston.ini");
>> @@ -1349,20 +1416,22 @@ int main(int argc, char *argv[])
>> 	/* Create panel and background for outputs processed before the shell
>> 	 * global interface was processed */
>> 	wl_list_for_each(output, &desktop.outputs, link)
>> 		if (!output->panel)
>> 			output_init(output, &desktop);
>> 
>> 	grab_surface_create(&desktop);
>> 
>> 	signal(SIGCHLD, sigchild_handler);
>> 
>> +	process_autolaunch(&desktop);
>> +
>> 	display_run(desktop.display);
>> 
>> 	/* Cleanup */
>> 	grab_surface_destroy(&desktop);
>> 	desktop_destroy_outputs(&desktop);
>> 	if (desktop.unlock_dialog)
>> 		unlock_dialog_destroy(desktop.unlock_dialog);
>> 	desktop_shell_destroy(desktop.shell);
>> 	display_destroy(desktop.display);
>> 
>> diff --git a/man/weston.ini.man b/man/weston.ini.man
>> index c05a221..365141c 100644
>> --- a/man/weston.ini.man
>> +++ b/man/weston.ini.man
>> @@ -66,20 +66,21 @@ Comment lines are ignored:
>> .RE
>> .PP
>> The section headers are:
>> .PP
>> .RS 4
>> .nf
>> .BR "core           " "The core modules"
>> .BR "libinput       " "Input device configuration"
>> .BR "shell          " "Desktop customization"
>> .BR "launcher       " "Add launcher to the panel"
>> +.BR "autolaunch     " "Launch program automatically on startup"
>> .BR "screensaver    " "Screensaver selection"
>> .BR "output         " "Output configuration"
>> .BR "input-method   " "Onscreen keyboard input"
>> .BR "keyboard       " "Keyboard layouts"
>> .BR "terminal       " "Terminal application options"
>> .BR "xwayland       " "XWayland options"
>> .BR "screen-share   " "Screen sharing options"
>> .fi
>> .RE
>> .PP
>> @@ -272,20 +273,36 @@ sets the path to the program that is run by clicking on this launcher (string).
>> It is possible to pass arguments and environment variables to the program. For
>> example:
>> .nf
>> .in +4n
>> 
>> path=GDK_BACKEND=wayland gnome-terminal --full-screen
>> .in
>> .fi
>> .PP
>> .RE
>> +.SH "AUTOLAUNCH SECTION"
>> +There can be multiple autolaunch sections for starting multiple programs.
>> +.TP 7
>> +.BI "path=" program
>> +sets the path (string) to the program that is run automatically when the
>> +desktop is starting.
>> +It is possible to pass arguments and environment variables to the program. For
>> +example:
>> +.nf
>> +.in +4n
>> +
>> +path=GDK_BACKEND=wayland gnome-terminal --full-screen
>> +.in
>> +.fi
>> +.PP
>> +.RE
>> .SH "SCREENSAVER SECTION"
>> The
>> .B screensaver
>> section is used to select and schedule a screensaver.
>> The
>> .B screensaver
>> section is optional, as are all of the entries that may be specified in
>> it.
>> .TP 7
>> .BI "path=" /usr/libexec/weston-screensaver
>> diff --git a/weston.ini.in b/weston.ini.in
>> index 4fca0bb..b0cb31f 100644
>> --- a/weston.ini.in
>> +++ b/weston.ini.in
>> @@ -30,20 +30,23 @@ icon=/usr/share/icons/gnome/24x24/apps/utilities-terminal.png
>> path=@bindir@/weston-terminal
>> 
>> [launcher]
>> icon=/usr/share/icons/hicolor/24x24/apps/google-chrome.png
>> path=/usr/bin/google-chrome
>> 
>> [launcher]
>> icon=/usr/share/icons/gnome/24x24/apps/arts.png
>> path=@abs_top_builddir@/weston-flower
>> 
>> +#[autolaunch]
>> +#path=@bindir@/weston-terminal
>> +
>> [screensaver]
>> # Comment path to disable screensaver
>> path=@libexecdir@/weston-screensaver
>> duration=600
>> 
>> [input-method]
>> path=@libexecdir@/weston-keyboard
>> 
>> #[output]
>> #name=LVDS1
>> 
> 
> _______________________________________________
> wayland-devel mailing list
> wayland-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/wayland-devel

On Thu, 23 Oct 2014 15:07:01 -0400
Frederic Plourde <frederic.plourde@collabora.co.uk> wrote:

> +1 for refactoring in a separate patch.
> by merging the change with panel_add_launcher in the same patch, it gets *really* hard to read because of the intertwining.

Yeah, please split it.

Btw. it was Fred who actually sent the patch, but made a slight mistake
in trying to preserve my authorship in the process, so it ended
up looking like I sent the patch. :-P

I did write the original feature[1], but Fred has been cleaning it up
since. Credit where credit is due. :-)

Fred, since you're going to split the patch, you should take the
authorship of the parse_launcher_path patch, since I never did that.
Refactor first, new feature second - less code to review.

> now, for exit(<int>) instead of using EXIT_FAILURE/SUCCESS, currently there is roughly 50 usages of exit(<int>) out of 100 total calls to exit() in Weston codebase.
> So we can't really say one if more trendy than the other.
> I'd suggest we leave that as is for now in this patch and open up another bug to change all of them separately someday.

Do make a difference between compositor vs. other programs' exit()
usage. There are only a few exit() calls in the compositor, and note
that the test suite also uses the Weston exit codes to relay test
success/skip/failure IIRC.

Fred has been writing a way to return an exit code from Weston also
with a normal shutdown, so that will interact here. I think a graceful
shutdown would be "better" than calling exit(), but that needs to be
verified. OTOH, calling exit() should not have any adverse effects like
leaving a VT unusable, but somehow I'm not quite convinced over all
possible cases (e.g. running weston directly as root for debugging on
DRM - or what about restoring KMS state?).

Oh, but the exit() call in this particular case is not Weston but the
forked process. So nevermind yet.

> On 14-10-22 11:57 AM, Derek Foreman wrote:
> I'd prefer to see the refactor and the new feature in separate patches,
> but this is pretty trivial.
> 
> I also have a slight preference for exit(EXIT_FAILURE), which is already
> used somewhere else in that file - though there's also precedent for
> exit(1), so you make the call.  :)
> 
> I'd not seen printf's %m until today - do we want to depend on a gnuism?
>  I've seen at least some activity towards a freebsd port - I don't
> believe %m is supported there?

Yeah, we already use that a lot, and I think we can continue adding
those, until someone comes and complains it doesn't work on
Android/BSD/Windows/whatever/...

Can do a mass change then.

> That said, it runs nicely here and does what it says on the tin...
> 
> Reviewed-by: Derek Foreman <derekf@osg.samsung.com>
> 
> On 22/10/14 08:53 AM, Pekka Paalanen wrote:
> Process a new section 'autolaunch' from weston.ini, launching all
> programs given there on desktop start-up.
> 
> [Frederic Plourde: cut redundancy between do_autolaunch and panel_add_launcher]
> ---
>  clients/desktop-shell.c | 97 ++++++++++++++++++++++++++++++++++++++++++-------
>  man/weston.ini.man      | 17 +++++++++
>  weston.ini.in           |  3 ++
>  3 files changed, 103 insertions(+), 14 deletions(-)


Thanks,
pq


[1] http://cgit.collabora.com/git/user/pq/weston.git/commit/?h=next&id=8a0e419fb33c3ab7d4036f25b0c33482e1059d45
Hi,

On 22 October 2014 14:53, Pekka Paalanen <pekka.paalanen@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?

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?

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

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

> Hi,
> 
> On 22 October 2014 14:53, Pekka Paalanen <pekka.paalanen@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
On Fri, Oct 24, 2014 at 01:18:51PM +0300, Pekka Paalanen wrote:
> > 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.

Hmm, the several special cases / hacks here maybe suggests some
generalization is needed?

Also, for X, there is typically a .xinitrc, .gnomerc, or similar that's
executed as a shell script during system launch.  Do we have plans for
adding something similar for weston?  Like a .westonrc?

If not, perhaps this feature gives us a place for hanging startup
scripts?

(Not saying that shell scripts are the best way to do autostart of
applications, but it has been a tradition...)

Bryce
On Fri, 24 Oct 2014 14:47:00 -0700
Bryce Harrington <bryce@osg.samsung.com> wrote:

> On Fri, Oct 24, 2014 at 01:18:51PM +0300, Pekka Paalanen wrote:
> > > 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.
> 
> Hmm, the several special cases / hacks here maybe suggests some
> generalization is needed?

The two cases I described are very different. weston_client_start() is
usually used to launch DE helper applications that need special
privileges, and therefore the Wayland connection is created
before-hand. It is part of our security mechanisms.

What would you refactor here?

> Also, for X, there is typically a .xinitrc, .gnomerc, or similar that's
> executed as a shell script during system launch.  Do we have plans for
> adding something similar for weston?  Like a .westonrc?

This patch adds exactly that. Well, a place to tell what to execute
automatically when the compositor becomes ready, anyway.

Btw. there is already also the "primary client" feature in Weston. An
application may launch Weston and be the only normal client to Weston
then. Weston will quit when this client disconnects. It is controlled
by WESTON_SERVER_SOCKET environment variable, which is set to an open
file descriptor number. Weston does not create a listening socket in
this case. Obviously that is a very different case than start-up
scripts.

> If not, perhaps this feature gives us a place for hanging startup
> scripts?

Yes. We already have lots of logic to find a weston.ini file, why would
we add more logic to similarly find a .westonrc? We can just say in
weston.ini what to execute.

I think that is also more clear, because the only file that Weston
needs to find on its own is weston.ini, and so it's a single point of
failure rather than finding two, if something goes wrong.

Note, that you can have any number of autolaunch entries. You don't
need a separate script if you just start a bunch of one-off apps.

> (Not saying that shell scripts are the best way to do autostart of
> applications, but it has been a tradition...)

Well, if you are building a DE, you write a plugin for it in any case,
and that plugin can then spawn and manage the helper apps if it needs
to.

This autolaunch on the other hand is very much a hack, which is useful
in the odd cases where writing a plugin is not appropriate. A demo setup
that automatically starts a video player at boot, for instance, which is
the very case behind this patch.

One might see this similar to script based init vs. systemd. I do
like systemd, but I don't think we should make Weston a generic process
manager. For the odd hacks, being able to run scripts and other random
stuff just via weston.ini is useful.


Thanks,
pq

Hi,

On 24 October 2014 11:18, Pekka Paalanen <pekka.paalanen@collabora.co.uk>
wrote:

> On Fri, 24 Oct 2014 10:28:57 +0100
> Daniel Stone <daniel@fooishbar.org> wrote:
> >
> > Hmm. Can we please use weston_client_start here instead of open-coding
> it?
>
> weston_client_start() does not support passing in environment
> explicitly.


Entirely fixable.


> 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.


Well sure, but the script case is indeed something which just restarts in a
loop - which would be 100% unnecessary if autostart did that for us.
Allowing multiple autostart entries would eliminate another reason to write
scripts, in that you could launch multiple clients. And if you really,
really, really need it: unset WAYLAND_SOCKET? Obviously that would preclude
use of autorestart.

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.
>

I can see where you're going with this, but again, this could be solved
with documentation: if you have a client which terminates the
WAYLAND_SOCKET before it's actually exited, either don't enable
autorestart, or handle it yourself.


> 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.
>

Sure, but having everyone just code the same while true script is
necessarily that great either. Having Weston more usable OOTB as a kiosk
mode is a pretty good thing, and having it be more robust doubly so.

I mean, I don't disagree with you, it's just that I don't see these issues
as showstoppers - if people want to write random weird scripts or clients
which don't fit in with passing sockets or autorestart, then don't use
those features?

Cheers,
Daniel


> Thanks,
> pq
>
> > Aside from that, and splitting refactor / autorestart code move / autorun
> > feature into separate patches, this looks good to me.
> >
> > Cheers,
> > Daniel
>
>
On Tue, 28 Oct 2014 16:46:24 +0000
Daniel Stone <daniel@fooishbar.org> wrote:

> Hi,
> 
> On 24 October 2014 11:18, Pekka Paalanen <pekka.paalanen@collabora.co.uk>
> wrote:
> 
> > On Fri, 24 Oct 2014 10:28:57 +0100
> > Daniel Stone <daniel@fooishbar.org> wrote:
> > >
> > > Hmm. Can we please use weston_client_start here instead of open-coding
> > it?
> >
> > weston_client_start() does not support passing in environment
> > explicitly.
> 
> 
> Entirely fixable.
> 
> 
> > 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.
> 
> 
> Well sure, but the script case is indeed something which just restarts in a
> loop - which would be 100% unnecessary if autostart did that for us.
> Allowing multiple autostart entries would eliminate another reason to write
> scripts, in that you could launch multiple clients. And if you really,
> really, really need it: unset WAYLAND_SOCKET? Obviously that would preclude
> use of autorestart.
> 
> 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.
> >
> 
> I can see where you're going with this, but again, this could be solved
> with documentation: if you have a client which terminates the
> WAYLAND_SOCKET before it's actually exited, either don't enable
> autorestart, or handle it yourself.
> 
> 
> > 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.
> >
> 
> Sure, but having everyone just code the same while true script is
> necessarily that great either. Having Weston more usable OOTB as a kiosk
> mode is a pretty good thing, and having it be more robust doubly so.
> 
> I mean, I don't disagree with you, it's just that I don't see these issues
> as showstoppers - if people want to write random weird scripts or clients
> which don't fit in with passing sockets or autorestart, then don't use
> those features?

I think we all forgot a tiny detail here.

weston_client_start() is a compositor internal function, while this
patch is modifying weston-desktop-shell client.

We would need to move the stuff into libshared, and make sure that
weston_client_start() can be implemented on top of the shared code.
Sharing the code that parses a command into a structure of environment,
executable and arguments is ok, but I think that is all there would be
shared. OTOH, the compositor side does not need to parse such commands
at all at the moment (but it could be a useful addition).

The compositor watches process exit with wl_event_loop's signalfd
handler, which is a libwayland-server feature.

In client side, there is no such thing as wl_client destruction. The
only common thing you can have is SIGCHLD to trigger respawn, which the
server side does not want. You'd have two different respawn triggers
even with shared respawn logic.

Then respawn needs a callback for respawning, so that the server code
can set up WAYLAND_SOCKET and then call back into libshared to
fork/exec the process, when libshared decides it needs to respawn
something, which in the server case needs the server to tell it to
respawn in the first place.

Is there anything worth sharing the code between the compositor and
weston-desktop-shell here?


Thanks,
pq
Hi,

On Wednesday, October 29, 2014, Pekka Paalanen <
pekka.paalanen@collabora.co.uk> wrote:

> On Tue, 28 Oct 2014 16:46:24 +0000
> Daniel Stone <daniel@fooishbar.org <javascript:;>> wrote:
> > On 24 October 2014 11:18, Pekka Paalanen <pekka.paalanen@collabora.co.uk
> <javascript:;>>
> > wrote:
> I think we all forgot a tiny detail here.
>
> weston_client_start() is a compositor internal function, while this
> patch is modifying weston-desktop-shell client.


Egads, I'd completely missed that, and assumed it was compositor rather
than shell.

Any reason to have it like that? Compositor seems more natural to me ...

-d
On Wed, 29 Oct 2014 20:01:33 +0000
Daniel Stone <daniel@fooishbar.org> wrote:

> Hi,
> 
> On Wednesday, October 29, 2014, Pekka Paalanen <
> pekka.paalanen@collabora.co.uk> wrote:
> 
> > On Tue, 28 Oct 2014 16:46:24 +0000
> > Daniel Stone <daniel@fooishbar.org <javascript:;>> wrote:
> > > On 24 October 2014 11:18, Pekka Paalanen <pekka.paalanen@collabora.co.uk
> > <javascript:;>>
> > > wrote:
> > I think we all forgot a tiny detail here.
> >
> > weston_client_start() is a compositor internal function, while this
> > patch is modifying weston-desktop-shell client.
> 
> 
> Egads, I'd completely missed that, and assumed it was compositor rather
> than shell.
> 
> Any reason to have it like that? Compositor seems more natural to me ...

The compositor could do the autolaunch, but we also have the panel
launch buttons, that need to be executed in weston-desktop-shell. And
autolaunch and the panel launchers are very similar, but not too similar
to the in-weston launching.

But I see what you might be getting at: if weston-desktop-shell does
autolaunch, and it respawns, it will autolaunch everything again. Not
good.

So we'd probably need autolaunch in the server side indeed. Then we
could put the command line parsing into env/cmd/args in libshared to
share that with weston-desktop-shell. Keep all the respawn logic inside
the compositor and maybe refactor that into some shared weston core
code.

Below I'll attach what I had written earlier but not sent, because
then I realized the server vs. weston-desktop-shell thing. If we do
autolaunch in the server, the below more or less applies again.

**

Okay, if Fred or someone actually has time to do all that refactoring
and writing, it's cool. It's just more work than simply adding
autolaunch, which was the original goal here.

So, we need to cater for two different cases:
a) start a client with a pre-made connection (set WAYLAND_SOCKET), and
b) start a new arbitrary process (do not set WAYLAND_SOCKET).

By refactoring, both cases should then have customizable environment,
and maybe optional respawn with conditions to give up if the process
exits too often.

Custom environment may come from weston.ini in case of manual
and auto-launchers, or hardcoded in plugins (currently not done).

Respawn needs the following parameters:
- is respawn triggered by wl_client destruction or SIGCHLD
- when to give up
(- delay to respawn?)

If I understood right, you suggested that respawn would be limited to
observing the wl_client, which practically means purely case a). That
should work, I think.

Manual launchers (the panel buttons) should never support respawn.

As a detail, case a) function will also need a hook that delivers the
new wl_client to the calling code when respawn happens. Otherwise e.g.
shell.c would not get the new authorized connection if
weston-desktop-shell crashes. We will also need a way to stop
respawning, e.g. shell.c uses that during shutdown when it deliberately
disconnects weston-desktop-shell.

When to give up?

The current logic for weston-desktop-shell is that if it crashes during
the first 30 seconds from session start, respawn will not be attempted,
and weston is shut down instead. Later, if weston-desktop-shell crashes
more than 5 times within 30 seconds, it gives up but does not exit
Weston, since other clients may still work.

**

Do we want to support autolaunching anything else than Wayland clients
directly? If not, then case b) can be dropped, which would be nice.

Can the weston-desktop-shell respawn/shutdown logic be supported by a
generic respawn code, or is it too complicated to be worth it? Maybe
e.g. weston-keyboard has a less complicated respawn logic that could
share with autolaunch.

Details... up to the person writing the code. We can then see how it
looks like. I would just want to avoid designing a big and complicated
launching and respawning framework just to cater the need of one
special case (weston-desktop-shell). If we can do with a simple design
otherwise but force weston-desktop-shell logic still be separate, that
is fine. Simplicity should be the driving goal in the code design. ;-)


Thanks,
pq
On 14-10-30 04:06 AM, Pekka Paalanen wrote:
> Below I'll attach what I had written earlier but not sent, because
> then I realized the server vs. weston-desktop-shell thing. If we do
> autolaunch in the server, the below more or less applies again.
>
> **
>
> Okay, if Fred or someone actually has time to do all that refactoring
> and writing, it's cool. It's just more work than simply adding
> autolaunch, which was the original goal here.

Yes, I definitely have time to tackle this as well.
We'll discuss it some more in a short while. thanks.

> So, we need to cater for two different cases:
> a) start a client with a pre-made connection (set WAYLAND_SOCKET), and
> b) start a new arbitrary process (do not set WAYLAND_SOCKET).
>
>
> Thanks,
> pq
> _______________________________________________
> wayland-devel mailing list
> wayland-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/wayland-devel