Re: [PATCH 7/7] t/helper/simple-ipc: convert test-simple-ipc to use start_bg_command

[Taylor Blau <me@ttaylorr.com>]

On Wed, Sep 15, 2021 at 08:36:17PM +0000, Jeff Hostetler via GitGitGadget wrote:
> From: Jeff Hostetler <jeffhost@microsoft.com>
>
> 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 <jeffhost@microsoft.com>
> ---
>  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