Re: [PATCH 7/7] t/helper/simple-ipc: convert test-simple-ipc to use start_bg_command
From: Taylor Blau <hidden>
Date: 2021-09-16 05:06:59
Subsystem:
the rest · Maintainer:
Linus Torvalds
On Wed, Sep 15, 2021 at 08:36:17PM +0000, Jeff Hostetler via GitGitGadget wrote:
quoted hunk ↗ jump to hunk
From: Jeff Hostetler <redacted> Convert test helper to use `start_bg_command()` when spawning a server daemon in the background rather than blocks of platform-specific code. Signed-off-by: Jeff Hostetler <redacted> --- t/helper/test-simple-ipc.c | 193 ++++++++----------------------------- 1 file changed, 40 insertions(+), 153 deletions(-)diff --git a/t/helper/test-simple-ipc.c b/t/helper/test-simple-ipc.c index 91345180750..59a950f3b00 100644 --- a/t/helper/test-simple-ipc.c +++ b/t/helper/test-simple-ipc.c@@ -9,6 +9,7 @@ #include "parse-options.h" #include "thread-utils.h" #include "strvec.h" +#include "run-command.h" #ifndef SUPPORTS_SIMPLE_IPC int cmd__simple_ipc(int argc, const char **argv)@@ -274,178 +275,64 @@ static int daemon__run_server(void) return ret; } -#ifndef GIT_WINDOWS_NATIVE -/* - * This is adapted from `daemonize()`. Use `fork()` to directly create and - * run the daemon in a child process. - */ -static int spawn_server(pid_t *pid) -{ - struct ipc_server_opts opts = { - .nr_threads = cl_args.nr_threads, - }; +static start_bg_wait_cb bg_wait_cb;
This whole patch is delightful to read, as the new implementation is so much cleaner as a result of the earlier work in this series. Am I correct in assuming that this is to encourage a compiler error if bg_wait_cb does not satisfy the type of start_bg_wait_cb? If so, then I think we are already getting that by trying to pass bg_wait_cb to start_bg_command(). E.g., applying this (intentionally broken) diff on top:
--- 8< ---
diff --git a/t/helper/test-simple-ipc.c b/t/helper/test-simple-ipc.c
index 59a950f3b0..3aed787206 100644
--- a/t/helper/test-simple-ipc.c
+++ b/t/helper/test-simple-ipc.c@@ -275,9 +275,7 @@ static int daemon__run_server(void) return ret; } -static start_bg_wait_cb bg_wait_cb; - -static int bg_wait_cb(void *cb_data, const struct child_process *cp) +static int bg_wait_cb(const void *cb_data, const struct child_process *cp) { int s = ipc_get_active_state(cl_args.path); --- >8 ---
and then compiling still warns of a mismatched type when calling start_bg_command().
- *pid = fork();
-
- switch (*pid) {
- case 0:
- if (setsid() == -1)
- error_errno(_("setsid failed"));
- close(0);
- close(1);
- close(2);
- sanitize_stdfds();
+static int bg_wait_cb(void *cb_data, const struct child_process *cp)
+{
+ int s = ipc_get_active_state(cl_args.path);
- return ipc_server_run(cl_args.path, &opts, test_app_cb,
- (void*)&my_app_data);
+ switch (s) {
+ case IPC_STATE__LISTENING:
+ /* child is "ready" */
+ return 0;
- case -1:
- return error_errno(_("could not spawn daemon in the background"));
+ case IPC_STATE__NOT_LISTENING:
+ case IPC_STATE__PATH_NOT_FOUND:
+ /* give child more time */
+ return 1;
default:I'm always a little hesitant to have default cases when switch over enum types, since it suppresses the warning when there's a new value of that type. But we already have a similar default in client__probe_server().
- else if (pid_seen == pid_child) {
- /*
- * The new child daemon process shutdown while
- * it was starting up, so it is not listening
- * on the socket.
- *
- * Try to ping the socket in the odd chance
- * that another daemon started (or was already
- * running) while our child was starting.
- *
- * Again, we don't care who services the socket.
- */
- s = ipc_get_active_state(cl_args.path);
- if (s == IPC_STATE__LISTENING)
- return 0;
+ default:Ditto. Thanks, Taylor