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

[Jeff Hostetler <git@jeffhostetler.com>]

On 3/2/21 4:44 AM, Jeff King wrote:
> 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
> 

Yeah, since it was a test helper I hesitated to add a command line
arg to pass it to the child process (when all callers were right here
and using the same default value).  However it would be good to do so
in case we want to write more complicated tests.

Jeff