~sircmpwn/public-inbox

General catch-all for patches, questions, and discussions for sircmpwn's projects that don't have their own mailing list.

When posting patches to this list, please edit the [PATCH] line to include the specific project you're contributing to, e.g.

[PATCH scdoc v2] Add thing to stuff

Patches

8 2

[PATCH] rootston: Install SIGCHLD handler using wl_event_loop

Genki Sky
Details
Message ID
<f27735ca3983ef89ba2e5ba3f2d3fe25033e445d.1533347140.git.sky@genki.is>
Download raw message
Patch +17 -0
test: For xwayland={false,true,immediate}
  1. Open gnome-terminal
  2. Close gnome-terminal
  3. Look at $(pgrep gnome-terminal | xargs ps)
    Without this patch, will see <defunct> processes.
    With this patch, you won't.
---
 rootston/main.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/rootston/main.c b/rootston/main.c
index cc3ffd3e..b7ce8ffa 100644
--- a/rootston/main.c
+++ b/rootston/main.c
@@ -1,6 +1,8 @@
 #define _POSIX_C_SOURCE 200112L
 #include <assert.h>
+#include <signal.h>
 #include <stdlib.h>
+#include <sys/wait.h>
 #include <unistd.h>
 #include <wayland-server.h>
 #include <wlr/backend.h>
@@ -14,6 +16,15 @@

 struct roots_server server = { 0 };

+static int handle_sigchld(int signal_number, void *data) {
+	assert(signal_number == SIGCHLD);
+	int old_errno = errno;
+	while (waitpid(-1, NULL, WNOHANG) > 0)
+		;
+	errno = old_errno;
+	return 0;
+}
+
 int main(int argc, char **argv) {
 	wlr_log_init(WLR_DEBUG, NULL);
 	server.config = roots_config_create_from_args(argc, argv);
@@ -21,6 +32,12 @@ int main(int argc, char **argv) {
 	server.wl_event_loop = wl_display_get_event_loop(server.wl_display);
 	assert(server.config && server.wl_display && server.wl_event_loop);

+	if (wl_event_loop_add_signal(server.wl_event_loop,
+			SIGCHLD, handle_sigchld, NULL) == NULL) {
+		wlr_log(WLR_ERROR, "Unable to install SIGCHLD handler");
+		return 1;
+	}
+
 	server.backend = wlr_backend_autocreate(server.wl_display, NULL);
 	if (server.backend == NULL) {
 		wlr_log(WLR_ERROR, "could not start backend");
--
2.18.0
Details
Message ID
<20180804201730.GA18046@homura.localdomain>
In-Reply-To
<f27735ca3983ef89ba2e5ba3f2d3fe25033e445d.1533347140.git.sky@genki.is>
Download raw message
On 2018-08-03  9:46 PM, Genki Sky wrote:
> --- a/rootston/main.c
> +++ b/rootston/main.c
> +	while (waitpid(-1, NULL, WNOHANG) > 0)
> +		;
> +	errno = old_errno;
> +	return 0;
> +}

We prefer the following style:

while (x) {
	/* this space intentionally left blank */
}

LGTM otherwise.

[PATCH wlroots v2] rootston: Install SIGCHLD handler using wl_event_loop

Genki Sky
Details
Message ID
<5f1a5d0442aaed6bf01f954f8ead9480ca9c0ebe.1533417969.git.sky@genki.is>
In-Reply-To
<20180804201730.GA18046@homura.localdomain>
Download raw message
Patch +18 -0
test: For xwayland={false,true,immediate}
  1. Open gnome-terminal
  2. Close gnome-terminal
  3. Look at $(pgrep gnome-terminal | xargs ps)
    Without this patch, will see <defunct> processes.
    With this patch, you won't.
---

Changes from v1:

  Use preferred style for indicating blank body in while loop.

 rootston/main.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/rootston/main.c b/rootston/main.c
index cc3ffd3e..5d025c9c 100644
--- a/rootston/main.c
+++ b/rootston/main.c
@@ -1,6 +1,8 @@
 #define _POSIX_C_SOURCE 200112L
 #include <assert.h>
+#include <signal.h>
 #include <stdlib.h>
+#include <sys/wait.h>
 #include <unistd.h>
 #include <wayland-server.h>
 #include <wlr/backend.h>
@@ -14,6 +16,16 @@

 struct roots_server server = { 0 };

+static int handle_sigchld(int signal_number, void *data) {
+	assert(signal_number == SIGCHLD);
+	int old_errno = errno;
+	while (waitpid(-1, NULL, WNOHANG) > 0) {
+		/* this space intentionally left blank */
+	}
+	errno = old_errno;
+	return 0;
+}
+
 int main(int argc, char **argv) {
 	wlr_log_init(WLR_DEBUG, NULL);
 	server.config = roots_config_create_from_args(argc, argv);
@@ -21,6 +33,12 @@ int main(int argc, char **argv) {
 	server.wl_event_loop = wl_display_get_event_loop(server.wl_display);
 	assert(server.config && server.wl_display && server.wl_event_loop);

+	if (wl_event_loop_add_signal(server.wl_event_loop,
+			SIGCHLD, handle_sigchld, NULL) == NULL) {
+		wlr_log(WLR_ERROR, "Unable to install SIGCHLD handler");
+		return 1;
+	}
+
 	server.backend = wlr_backend_autocreate(server.wl_display, NULL);
 	if (server.backend == NULL) {
 		wlr_log(WLR_ERROR, "could not start backend");
--
2.18.0

Re: [PATCH wlroots v2] rootston: Install SIGCHLD handler using wl_event_loop

Details
Message ID
<20180804221651.GA11641@homura.localdomain>
In-Reply-To
<5f1a5d0442aaed6bf01f954f8ead9480ca9c0ebe.1533417969.git.sky@genki.is>
Download raw message
<emersion> the logic LGTM
<emersion> wait
<emersion> no
<emersion> what happens if you waitpid() in a blocking manner _and_ have
           a sigchld handler?
<emersion> i'd rather double-fork tbh
<emersion> session-direct and xwayland do blocking waitpid() calls
<emersion> also this should handle ECHILD
    <ongy> NOHANG
<emersion> >For all other conditions, it is unspecified whether child
           status will be available when a SIGCHLD signal is delivered.
<emersion> erm
    <ongy> ohh, in other situations
    <ongy> durr

[PATCH wlroots v3] rootston: Double fork for keyboard bindings

Genki Sky
Details
Message ID
<0a8a1646474fe2efdd08cba7b7c89975baafec6e.1533784509.git.sky@genki.is>
In-Reply-To
<20180804221651.GA11641@homura.localdomain>
Download raw message
Patch +32 -7
This avoids leaving around zombies, without having to setup SIGCHLD
handler (which interferes with other fork/waitpid calls).
---
 rootston/keyboard.c | 39 ++++++++++++++++++++++++++++++++-------
 1 file changed, 32 insertions(+), 7 deletions(-)

diff --git a/rootston/keyboard.c b/rootston/keyboard.c
index 40d4a7c7..0e1eac25 100644
--- a/rootston/keyboard.c
+++ b/rootston/keyboard.c
@@ -2,6 +2,7 @@
 #include <stdbool.h>
 #include <stdint.h>
 #include <stdlib.h>
+#include <sys/wait.h>
 #include <unistd.h>
 #include <wayland-server.h>
 #include <wlr/backend/multi.h>
@@ -84,6 +85,36 @@ static void pressed_keysyms_update(xkb_keysym_t *pressed_keysyms,
 	}
 }
 
+static void double_fork_shell_cmd(const char *shell_cmd) {
+	pid_t pid = fork();
+	if (pid < 0) {
+		wlr_log(WLR_ERROR, "cannot execute binding command: fork() failed");
+		return;
+	}
+
+	if (pid == 0) {
+		pid = fork();
+		if (pid == 0) {
+			execl("/bin/sh", "/bin/sh", "-c", shell_cmd, (void *)NULL);
+			_exit(EXIT_FAILURE);
+		} else {
+			_exit(pid == -1);
+		}
+	}
+
+	int status;
+	while (waitpid(pid, &status, 0) < 0) {
+		if (errno == EINTR)
+			continue;
+		wlr_log_errno(WLR_ERROR, "waitpid() on first child failed");
+		return;
+	}
+
+	if (!WIFEXITED(status) || !WEXITSTATUS(status)) {
+		wlr_log(WLR_ERROR, "first child failed to fork command");
+	}
+}
+
 static const char *exec_prefix = "exec ";
 
 static bool outputs_enabled = true;
@@ -113,13 +144,7 @@ static void keyboard_binding_execute(struct roots_keyboard *keyboard,
 		}
 	} else if (strncmp(exec_prefix, command, strlen(exec_prefix)) == 0) {
 		const char *shell_cmd = command + strlen(exec_prefix);
-		pid_t pid = fork();
-		if (pid < 0) {
-			wlr_log(WLR_ERROR, "cannot execute binding command: fork() failed");
-			return;
-		} else if (pid == 0) {
-			execl("/bin/sh", "/bin/sh", "-c", shell_cmd, (void *)NULL);
-		}
+		double_fork_shell_cmd(shell_cmd);
 	} else if (strcmp(command, "maximize") == 0) {
 		struct roots_view *focus = roots_seat_get_focus(seat);
 		if (focus != NULL) {
-- 
2.18.0

[PATCH wlroots v4] rootston: Double fork for keyboard bindings

Genki Sky
Details
Message ID
<1629acf3f4362b3159dbab8d39f22178874c46ec.1533875094.git.sky@genki.is>
In-Reply-To
<0a8a1646474fe2efdd08cba7b7c89975baafec6e.1533784509.git.sky@genki.is>
Download raw message
Patch +34 -7
This avoids leaving around zombies, without having to setup SIGCHLD
handler (which interferes with other fork/waitpid calls).
---
Notes:

  Changes from v3:
    - Fix WIFEXITED and WEXITSTATUS logic.

  Changes from v2:
    - Use double fork instead of SIGCHLD handler.

  Not totally confident about any edge cases w.r.t double forking, but
  this seems to do the right thing, from my testing.

 rootston/keyboard.c | 41 ++++++++++++++++++++++++++++++++++-------
 1 file changed, 34 insertions(+), 7 deletions(-)

diff --git a/rootston/keyboard.c b/rootston/keyboard.c
index 40d4a7c7..5479ff13 100644
--- a/rootston/keyboard.c
+++ b/rootston/keyboard.c
@@ -2,6 +2,7 @@
 #include <stdbool.h>
 #include <stdint.h>
 #include <stdlib.h>
+#include <sys/wait.h>
 #include <unistd.h>
 #include <wayland-server.h>
 #include <wlr/backend/multi.h>
@@ -84,6 +85,38 @@ static void pressed_keysyms_update(xkb_keysym_t *pressed_keysyms,
 	}
 }

+static void double_fork_shell_cmd(const char *shell_cmd) {
+	pid_t pid = fork();
+	if (pid < 0) {
+		wlr_log(WLR_ERROR, "cannot execute binding command: fork() failed");
+		return;
+	}
+
+	if (pid == 0) {
+		pid = fork();
+		if (pid == 0) {
+			execl("/bin/sh", "/bin/sh", "-c", shell_cmd, (void *)NULL);
+			_exit(EXIT_FAILURE);
+		} else {
+			_exit(pid == -1);
+		}
+	}
+
+	int status;
+	while (waitpid(pid, &status, 0) < 0) {
+		if (errno == EINTR)
+			continue;
+		wlr_log_errno(WLR_ERROR, "waitpid() on first child failed");
+		return;
+	}
+
+	if (WIFEXITED(status) && WEXITSTATUS(status) == 0) {
+		return;
+	}
+
+	wlr_log(WLR_ERROR, "first child failed to fork command");
+}
+
 static const char *exec_prefix = "exec ";

 static bool outputs_enabled = true;
@@ -113,13 +146,7 @@ static void keyboard_binding_execute(struct roots_keyboard *keyboard,
 		}
 	} else if (strncmp(exec_prefix, command, strlen(exec_prefix)) == 0) {
 		const char *shell_cmd = command + strlen(exec_prefix);
-		pid_t pid = fork();
-		if (pid < 0) {
-			wlr_log(WLR_ERROR, "cannot execute binding command: fork() failed");
-			return;
-		} else if (pid == 0) {
-			execl("/bin/sh", "/bin/sh", "-c", shell_cmd, (void *)NULL);
-		}
+		double_fork_shell_cmd(shell_cmd);
 	} else if (strcmp(command, "maximize") == 0) {
 		struct roots_view *focus = roots_seat_get_focus(seat);
 		if (focus != NULL) {
--
2.18.0

Re: [PATCH wlroots v4] rootston: Double fork for keyboard bindings

Details
Message ID
<20180810134553.GA826@miku>
In-Reply-To
<1629acf3f4362b3159dbab8d39f22178874c46ec.1533875094.git.sky@genki.is>
Download raw message
Looks pretty good, but there are some style issues.

On 2018-08-10 12:27 AM, Genki Sky wrote:
> diff --git a/rootston/keyboard.c b/rootston/keyboard.c
> index 40d4a7c7..5479ff13 100644
> --- a/rootston/keyboard.c
> +++ b/rootston/keyboard.c
> -%<-
> +	if (pid == 0) {
> +		pid = fork();
> +		if (pid == 0) {
> +			execl("/bin/sh", "/bin/sh", "-c", shell_cmd, (void *)NULL);

No need to cast a NULL pointer.

> +			_exit(EXIT_FAILURE);

Why are you using _exit over exit?

> +	int status;
> +	while (waitpid(pid, &status, 0) < 0) {
> +		if (errno == EINTR)
> +			continue;

Needs braces.

[PATCH wlroots v5] rootston: Double fork for keyboard bindings

Genki Sky
Details
Message ID
<72bc9ae60b5a966de5242e344a524b56e3709582.1533910839.git.sky@genki.is>
In-Reply-To
<20180810134553.GA826@miku>
Download raw message
Patch +35 -7
This avoids leaving around zombies, without having to setup SIGCHLD
handler (which interferes with other fork/waitpid calls).
---

Notes:

  Changes from v4:
    - Use braces for EINTR errno check.
    - Don't cast NULL to void* (this was blindly copied over from the
      original code).

  I use _exit() instead of exit() because apparently forked (but not
  exec'd) children should use _exit(). I looked at _exit(3p) and it
  says that unlike normal exit(), it does not flush stdio buffers
  (which it'd share with the parent), and it does not call any
  atexit() handlers registered by the parent. So probably wouldn't be
  any issue using exit, but it seems _exit is the "good practice".

 rootston/keyboard.c | 42 +++++++++++++++++++++++++++++++++++-------
 1 file changed, 35 insertions(+), 7 deletions(-)

diff --git a/rootston/keyboard.c b/rootston/keyboard.c
index 40d4a7c7..b5a8093b 100644
--- a/rootston/keyboard.c
+++ b/rootston/keyboard.c
@@ -2,6 +2,7 @@
 #include <stdbool.h>
 #include <stdint.h>
 #include <stdlib.h>
+#include <sys/wait.h>
 #include <unistd.h>
 #include <wayland-server.h>
 #include <wlr/backend/multi.h>
@@ -84,6 +85,39 @@ static void pressed_keysyms_update(xkb_keysym_t *pressed_keysyms,
 	}
 }

+static void double_fork_shell_cmd(const char *shell_cmd) {
+	pid_t pid = fork();
+	if (pid < 0) {
+		wlr_log(WLR_ERROR, "cannot execute binding command: fork() failed");
+		return;
+	}
+
+	if (pid == 0) {
+		pid = fork();
+		if (pid == 0) {
+			execl("/bin/sh", "/bin/sh", "-c", shell_cmd, NULL);
+			_exit(EXIT_FAILURE);
+		} else {
+			_exit(pid == -1);
+		}
+	}
+
+	int status;
+	while (waitpid(pid, &status, 0) < 0) {
+		if (errno == EINTR) {
+			continue;
+		}
+		wlr_log_errno(WLR_ERROR, "waitpid() on first child failed");
+		return;
+	}
+
+	if (WIFEXITED(status) && WEXITSTATUS(status) == 0) {
+		return;
+	}
+
+	wlr_log(WLR_ERROR, "first child failed to fork command");
+}
+
 static const char *exec_prefix = "exec ";

 static bool outputs_enabled = true;
@@ -113,13 +147,7 @@ static void keyboard_binding_execute(struct roots_keyboard *keyboard,
 		}
 	} else if (strncmp(exec_prefix, command, strlen(exec_prefix)) == 0) {
 		const char *shell_cmd = command + strlen(exec_prefix);
-		pid_t pid = fork();
-		if (pid < 0) {
-			wlr_log(WLR_ERROR, "cannot execute binding command: fork() failed");
-			return;
-		} else if (pid == 0) {
-			execl("/bin/sh", "/bin/sh", "-c", shell_cmd, (void *)NULL);
-		}
+		double_fork_shell_cmd(shell_cmd);
 	} else if (strcmp(command, "maximize") == 0) {
 		struct roots_view *focus = roots_seat_get_focus(seat);
 		if (focus != NULL) {
--
2.18.0

Re: [PATCH wlroots v5] rootston: Double fork for keyboard bindings

Details
Message ID
<20180810150023.GB826@miku>
In-Reply-To
<72bc9ae60b5a966de5242e344a524b56e3709582.1533910839.git.sky@genki.is>
Download raw message
Thanks!

To git.sr.ht:~sircmpwn/wlroots
   b902c2bd..11d44097  master -> master