Re: [PATCH v4 12/12] t0052: add simple-ipc tests and t/helper/test-simple-ipc tool

> On Wed, Feb 17, 2021 at 09:48:48PM +0000, Jeff Hostetler via GitGitGadget wrote:
> 
>> Create t/helper/test-simple-ipc test tool to exercise the "simple-ipc"
>> functions.
> 
> BTW, one oddity I noticed in this (because of my -Wunused-parameters
> branch):
> 
>> +#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(const char *path,
>> +			const struct ipc_server_opts *opts,
>> +			pid_t *pid)
>> +{
>> +	*pid = fork();
>> +
>> +	switch (*pid) {
>> +	case 0:
>> +		if (setsid() == -1)
>> +			error_errno(_("setsid failed"));
>> +		close(0);
>> +		close(1);
>> +		close(2);
>> +		sanitize_stdfds();
>> +
>> +		return ipc_server_run(path, opts, test_app_cb, (void*)&my_app_data);
>> +
>> +	case -1:
>> +		return error_errno(_("could not spawn daemon in the background"));
>> +
>> +	default:
>> +		return 0;
>> +	}
>> +}
> 
> In the non-Windows version, we spawn a server using the "path" parameter
> we got from the caller.
> 
> But in the Windows version:
> 
>> +#else
>> +/*
>> + * Conceptually like `daemonize()` but different because Windows does not
>> + * have `fork(2)`.  Spawn a normal Windows child process but without the
>> + * limitations of `start_command()` and `finish_command()`.
>> + */
>> +static int spawn_server(const char *path,
>> +			const struct ipc_server_opts *opts,
>> +			pid_t *pid)
>> +{
>> +	char test_tool_exe[MAX_PATH];
>> +	struct strvec args = STRVEC_INIT;
>> +	int in, out;
>> +
>> +	GetModuleFileNameA(NULL, test_tool_exe, MAX_PATH);
>> +
>> +	in = open("/dev/null", O_RDONLY);
>> +	out = open("/dev/null", O_WRONLY);
>> +
>> +	strvec_push(&args, test_tool_exe);
>> +	strvec_push(&args, "simple-ipc");
>> +	strvec_push(&args, "run-daemon");
>> +	strvec_pushf(&args, "--threads=%d", opts->nr_threads);
>> +
>> +	*pid = mingw_spawnvpe(args.v[0], args.v, NULL, NULL, in, out, out);
>> +	close(in);
>> +	close(out);
>> +
>> +	strvec_clear(&args);
>> +
>> +	if (*pid < 0)
>> +		return error(_("could not spawn daemon in the background"));
>> +
>> +	return 0;
>> +}
>> +#endif
> 
> We ignore the "path" parameter entirely. Should we be passing it along
> as an option to the child process? I think it doesn't really matter at
> this point because both the parent and child processes will use the
> hard-coded string "ipc-test", but it seems like something the test
> script might want to be able to specify.
> 
> -Peff
>