Re: [PATCH i-g-t v2 1/2] runner/executor: Build test command line before fork

[Gustavo Sousa <gustavo.sousa@intel.com>]

Kamil Konieczny <kamil.konieczny@linux.intel.com> writes:

> Create a separate test command line building in main runner
> process before forking, so it could be later processed for other
> purposes like reporting.
>
> v2: free cmd line before closing fd, rename label (Gustavo)
>
> Signed-off-by: Kamil Konieczny <kamil.konieczny@linux.intel.com>

Reviewed-by: Gustavo Sousa <gustavo.sousa@intel.com>

> ---
>  runner/executor.c | 68 ++++++++++++++++++++++++++++++-----------------
>  1 file changed, 43 insertions(+), 25 deletions(-)
>
> diff --git a/runner/executor.c b/runner/executor.c
> index 8103c2a52..5bd95b929 100644
> --- a/runner/executor.c
> +++ b/runner/executor.c
> @@ -1618,28 +1618,29 @@ static int monitor_output(pid_t child,
>  	return killed;
>  }
>  
> -static void __attribute__((noreturn))
> -execute_test_process(int outfd, int errfd, int socketfd,
> -		     struct settings *settings,
> -		     struct job_list_entry *entry)
> +static void free_test_command(struct igt_vec *arg_vec)
>  {
> -	struct igt_vec arg_vec;
> -	char *arg;
> -	size_t rootlen;
> +	for (size_t i = 0; i < igt_vec_length(arg_vec); i++)
> +		free(*((char **)igt_vec_elem(arg_vec, i)));
>  
> -	dup2(outfd, STDOUT_FILENO);
> -	dup2(errfd, STDERR_FILENO);
> +	igt_vec_fini(arg_vec);
> +}
>  
> -	setpgid(0, 0);
> +static void build_test_command(struct settings *settings,
> +			       struct job_list_entry *entry,
> +			       struct igt_vec *arg_vec)
> +{
> +	size_t rootlen;
> +	char *arg;
>  
> -	igt_vec_init(&arg_vec, sizeof(char *));
> +	igt_vec_init(arg_vec, sizeof(char *));
>  
>  	rootlen = strlen(settings->test_root);
>  	arg = malloc(rootlen + strlen(entry->binary) + 2);
>  	strcpy(arg, settings->test_root);
>  	arg[rootlen] = '/';
>  	strcpy(arg + rootlen + 1, entry->binary);
> -	igt_vec_push(&arg_vec, &arg);
> +	igt_vec_push(arg_vec, &arg);
>  
>  	if (entry->subtest_count) {
>  		size_t argsize;
> @@ -1647,7 +1648,7 @@ execute_test_process(int outfd, int errfd, int socketfd,
>  		size_t i;
>  
>  		arg = strdup("--run-subtest");
> -		igt_vec_push(&arg_vec, &arg);
> +		igt_vec_push(arg_vec, &arg);
>  
>  		if ((dynbegin = strchr(entry->subtests[0], '@')) != NULL)
>  			argsize = dynbegin - entry->subtests[0];
> @@ -1670,35 +1671,49 @@ execute_test_process(int outfd, int errfd, int socketfd,
>  			argsize += sublen + 1;
>  		}
>  
> -		igt_vec_push(&arg_vec, &arg);
> +		igt_vec_push(arg_vec, &arg);
>  
>  		if (dynbegin) {
>  			arg = strdup("--dynamic-subtest");
> -			igt_vec_push(&arg_vec, &arg);
> +			igt_vec_push(arg_vec, &arg);
>  			arg = strdup(dynbegin + 1);
> -			igt_vec_push(&arg_vec, &arg);
> +			igt_vec_push(arg_vec, &arg);
>  		}
>  	}
>  
>  	for (size_t i = 0; i < igt_vec_length(&settings->hook_strs); i++) {
>  		arg = strdup("--hook");
> -		igt_vec_push(&arg_vec, &arg);
> +		igt_vec_push(arg_vec, &arg);
>  		arg = strdup(*((char **)igt_vec_elem(&settings->hook_strs, i)));
> -		igt_vec_push(&arg_vec, &arg);
> +		igt_vec_push(arg_vec, &arg);
>  	}
>  
>  	arg = NULL;
> -	igt_vec_push(&arg_vec, &arg);
> +	igt_vec_push(arg_vec, &arg);
> +}
> +
> +static void __attribute__((noreturn))
> +execute_test_process(int outfd, int errfd, int socketfd,
> +		     struct settings *settings,
> +		     struct job_list_entry *entry,
> +		     struct igt_vec *arg_vec)
> +{
> +	char *arg;
> +
> +	dup2(outfd, STDOUT_FILENO);
> +	dup2(errfd, STDERR_FILENO);
> +
> +	setpgid(0, 0);
>  
>  	if (socketfd >= 0) {
>  		struct runnerpacket *packet;
>  
> -		packet = runnerpacket_exec(arg_vec.elems);
> +		packet = runnerpacket_exec(arg_vec->elems);
>  		write(socketfd, packet, packet->size);
>  	}
>  
> -	arg = *((char **)igt_vec_elem(&arg_vec, 0));
> -	execv(arg, arg_vec.elems);
> +	arg = *((char **)igt_vec_elem(arg_vec, 0));
> +	execv(arg, arg_vec->elems);
>  	fprintf(stderr, "Cannot execute %s\n", arg);
>  	exit(IGT_EXIT_INVALID);
>  }
> @@ -1776,6 +1791,7 @@ static int execute_next_entry(struct execute_state *state,
>  			      bool *abort_already_written)
>  {
>  	int idirfd;
> +	struct igt_vec arg_vec;
>  	int outputs[_F_LAST];
>  	int kmsgfd;
>  	int outpipe[2] = { -1, -1 };
> @@ -1829,6 +1845,7 @@ static int execute_next_entry(struct execute_state *state,
>  		lseek(kmsgfd, 0, SEEK_END);
>  	}
>  
> +	build_test_command(settings, entry, &arg_vec);
>  
>  	if (settings->log_level >= LOG_LEVEL_NORMAL) {
>  		char buf[100];
> @@ -1860,7 +1877,7 @@ static int execute_next_entry(struct execute_state *state,
>  	if (child < 0) {
>  		errf("Failed to fork: %m\n");
>  		result = -1;
> -		goto out_kmsgfd;
> +		goto out_fork_failed;
>  	} else if (child == 0) {
>  		char envstring[16];
>  
> @@ -1880,7 +1897,7 @@ static int execute_next_entry(struct execute_state *state,
>  		setenv("IGT_SENTINEL_ON_STDERR", "1", 1);
>  		setenv("IGT_RUNNER_ATTACHMENTS_DIR", attdirname, 1);
>  
> -		execute_test_process(outfd, errfd, socketfd, settings, entry);
> +		execute_test_process(outfd, errfd, socketfd, settings, entry, &arg_vec);
>  		/* unreachable */
>  	}
>  
> @@ -1897,7 +1914,8 @@ static int execute_next_entry(struct execute_state *state,
>  				outputs, time_spent, settings,
>  				abortreason, abort_already_written);
>  
> -out_kmsgfd:
> +out_fork_failed:
> +	free_test_command(&arg_vec);
>  	close(kmsgfd);
>  out_pipe:
>  	close(outpipe[0]);
> -- 
> 2.55.0