Created attachment 251324 [details] [review]
wayland: don't use fork() and SIGCHLD to spawn processes

Created attachment 251325 [details] [review]
wayland: move XWayland support code to its own file

Given that xwayland code is already split in meta-xwayland, it
makes sense to have there the implementation of the private
xserver protocol too.

Review of attachment 251324 [details] [review]:

Wow, this was some ugly code you're deleting.

::: src/wayland/meta-xwayland.c
@@ +206,3 @@
+  fd = open (path, O_WRONLY | O_APPEND | O_CREAT | O_TRUNC, 0600);
+  if (fd < 0)
+    return;

Ewww.  On systemd systems, we should be logging to the journal.  I guess this isn't a new issue with the X server though...gdm is still putting the system's X log in a file.

How about we just don't touch stdout/stderr, and leave them connected to whatever mutter's output was?  That way on current GNOME+systemd we will use the journal.

(Later on GNOME+user-systemd we can make a user unit for XWayland and have it be socket activated, in its own cgroup etc.)

@@ +308,3 @@
+  args[8] = "-nolisten";
+  args[9] = "all";
+  args[10] = NULL;

For this type of code I like the style of doing:

args[i++] = ...
g_assert (i == G_N_ELEMENTS(args));

For added safety, as well as making it easy to insert/remove an argument later without changing every line.

@@ +314,3 @@
+                     args,
+                     env,
+                     G_SPAWN_LEAVE_DESCRIPTORS_OPEN |

Is this necessary?  Can't we just rely on unsetting the socket's cloexec flag?

@@ -267,3 @@
-                     /* FIXME: does it make sense to log to the filesystem by
-                      * default? */
-                     "-logfile", "/tmp/xwayland.log",

This old code was a security issue...

Review of attachment 251325 [details] [review]:

If this is just a mechanical change, looks fine to me.

(In reply to comment #3)
> Review of attachment 251324 [details] [review]:
> 
> Wow, this was some ugly code you're deleting.
> 
> ::: src/wayland/meta-xwayland.c
> @@ +206,3 @@
> +  fd = open (path, O_WRONLY | O_APPEND | O_CREAT | O_TRUNC, 0600);
> +  if (fd < 0)
> +    return;
> 
> Ewww.  On systemd systems, we should be logging to the journal.  I guess this
> isn't a new issue with the X server though...gdm is still putting the system's
> X log in a file.
> 
> How about we just don't touch stdout/stderr, and leave them connected to
> whatever mutter's output was?  That way on current GNOME+systemd we will use
> the journal.

The problem with not touching stdout/stderr is that mutter output is a tty, which doesn't like background stuff writing to it. OTOH, if I keep X in the foreground, pressing Ctrl-C in a debugger kills X too.
(In a later patch I also change mutter's stdout/stderr to be a log file, but still).
In general, X doesn't log much on stderr anyway, most of the interesting stuff is in the logfile, so we could maybe send it to /dev/null. I think I added it just for initialization errors.

> (Later on GNOME+user-systemd we can make a user unit for XWayland and have it
> be socket activated, in its own cgroup etc.)

I don't think we can do socket activation: xwayland must be launched by mutter to access the private protocol.

(In reply to comment #5)
 
> The problem with not touching stdout/stderr is that mutter output is a tty,

Not in the normal case, only when using "gnome-shell --replace" or the like from a terminal.

> which doesn't like background stuff writing to it.

I don't understand - what doesn't like background stuff?  What's the problem?

> OTOH, if I keep X in the
> foreground, pressing Ctrl-C in a debugger kills X too.

Hmmm, I don't think that's normal; I thought gdb trapped SIGINT specially and didn't pass it on to the children processes by default.  Maybe it has something to do with the fact that X installs a signal handler for SIGINT.

> (In a later patch I also change mutter's stdout/stderr to be a log file, but
> still).
> In general, X doesn't log much on stderr anyway, most of the interesting stuff
> is in the logfile, so we could maybe send it to /dev/null. I think I added it
> just for initialization errors.

Yeah, then let's just make the logfile /dev/null by default, maybe have an environment variable to override?

> > (Later on GNOME+user-systemd we can make a user unit for XWayland and have it
> > be socket activated, in its own cgroup etc.)
> 
> I don't think we can do socket activation: xwayland must be launched by mutter
> to access the private protocol.

Ok.

(Side note: I'm traveling today and tomorrow, so if this patch is blocking other things, you don't need to block on my review)

(In reply to comment #6)
> (In reply to comment #5)
> 
> > The problem with not touching stdout/stderr is that mutter output is a tty,
> 
> Not in the normal case, only when using "gnome-shell --replace" or the like
> from a terminal.

And that will be the normal case for running wayland in 3.10 (unless we have gdm launching by then, but it seems unlikely now)

> > which doesn't like background stuff writing to it.
> 
> I don't understand - what doesn't like background stuff?  What's the problem?
> 
> > OTOH, if I keep X in the
> > foreground, pressing Ctrl-C in a debugger kills X too.
> 
> Hmmm, I don't think that's normal; I thought gdb trapped SIGINT specially and
> didn't pass it on to the children processes by default.  Maybe it has something
> to do with the fact that X installs a signal handler for SIGINT.

No, the thing is, if you press Ctrl-C it goes to all processes in the foreground process group, but gdb only sees the one sent to the attached process.
(OTOH, if you press ctrl-c while gdb is attached to the process from a different terminal, it's gdb emulating the SIGINT and everything is fine)

(In reply to comment #8)

> No, the thing is, if you press Ctrl-C it goes to all processes in the
> foreground process group, but gdb only sees the one sent to the attached
> process.
> (OTOH, if you press ctrl-c while gdb is attached to the process from a
> different terminal, it's gdb emulating the SIGINT and everything is fine)

Ok right, we went through this with gnome-shell and applications too.  I'm too lazy to dig up the bugs...but I think we concluded there's just not a clean fix.

What we'll really have to figure out in the user systemd world is how to have jhbuild easily override the system gnome-shell.  I guess we'd have to tell systemd to look in $JHBUILD_ROOT; it will be the same problems with jhbuild+dbus.

Review of attachment 251324 [details] [review]:

Anyways, you're adding much better code here, it's not worth trying to spend a lot more work to achieve perfection; we can iteratively improve this stuff next cycle.

Attachment 251324 [details] pushed as 3803fd9 - wayland: don't use fork() and SIGCHLD to spawn processes
Attachment 251325 [details] pushed as 18a21b6 - wayland: move XWayland support code to its own file