Re: [igt-dev] [PATCH i-g-t] runner/executor: refactor error handling

[Petri Latvala <petri.latvala@intel.com>]

On Wed, Mar 20, 2019 at 09:33:55AM +0000, Ser, Simon wrote:
> * Refactor to use goto error handling
> * Make execute_test_process noreturn to remove uninitialized variable
>   warning
> * Check fork() return value
> 
> Signed-off-by: Simon Ser <simon.ser@intel.com>
> ---
>  runner/executor.c | 63 ++++++++++++++++++++++++-----------------------
>  1 file changed, 32 insertions(+), 31 deletions(-)
> 
> diff --git a/runner/executor.c b/runner/executor.c
> index d535c276..6bf7ce1c 100644
> --- a/runner/executor.c
> +++ b/runner/executor.c
> @@ -753,9 +753,10 @@ static int monitor_output(pid_t child,
>  	return killed;
>  }
>  
> -static void execute_test_process(int outfd, int errfd,
> -				 struct settings *settings,
> -				 struct job_list_entry *entry)
> +static void __attribute__((noreturn))
> +execute_test_process(int outfd, int errfd,
> +		     struct settings *settings,
> +		     struct job_list_entry *entry)
>  {
>  	char *argv[4] = {};
>  	size_t rootlen;
> @@ -884,9 +885,9 @@ static int execute_next_entry(struct execute_state
> *state,
>  	}
>  
>  	if (!open_output_files(dirfd, outputs, true)) {
> -		close(dirfd);
>  		fprintf(stderr, "Error opening output files\n");
> -		return -1;
> +		result = -1;
> +		goto out_dirfd;
>  	}
>  
>  	if (settings->sync) {
> @@ -895,14 +896,9 @@ static int execute_next_entry(struct execute_state
> *state,
>  	}
>  
>  	if (pipe(outpipe) || pipe(errpipe)) {
> -		close_outputs(outputs);
> -		close(dirfd);
> -		close(outpipe[0]);
> -		close(outpipe[1]);
> -		close(errpipe[0]);
> -		close(errpipe[1]);
>  		fprintf(stderr, "Error creating pipes: %s\n",
> strerror(errno));
> -		return -1;
> +		result = -1;
> +		goto out_pipe;
>  	}
>  
>  	if ((kmsgfd = open("/dev/kmsg", O_RDONLY | O_CLOEXEC)) < 0) {
> @@ -923,14 +919,8 @@ static int execute_next_entry(struct execute_state
> *state,
>  	if (sigfd < 0) {
>  		/* TODO: Handle better */
>  		fprintf(stderr, "Cannot monitor child process with
> signalfd\n");
> -		close(outpipe[0]);
> -		close(errpipe[0]);
> -		close(outpipe[1]);
> -		close(errpipe[1]);
> -		close(kmsgfd);
> -		close_outputs(outputs);
> -		close(dirfd);
> -		return -1;
> +		result = -1;
> +		goto out_kmsgfd;
>  	}
>  
>  	if (settings->log_level >= LOG_LEVEL_NORMAL) {
> @@ -951,19 +941,15 @@ static int execute_next_entry(struct
> execute_state *state,
>  	 * Flush outputs before forking so our (buffered) output won't
>  	 * end up in the test outputs.
>  	 */
> -
>  	fflush(stdout);
>  	fflush(stderr);
>  
> -	if ((child = fork())) {
> -		int outfd = outpipe[0];
> -		int errfd = errpipe[0];
> -		close(outpipe[1]);
> -		close(errpipe[1]);
> -
> -		result = monitor_output(child, outfd, errfd, kmsgfd,
> sigfd,
> -					outputs, time_spent, settings);
> -	} else {
> +	child = fork();
> +	if (child < 0) {
> +		fprintf(stderr, "Failed to fork()");
> +		result = -1;
> +		goto out_kmsgfd;
> +	} else if (child == 0) {
>  		int outfd = outpipe[1];
>  		int errfd = errpipe[1];
>  		close(outpipe[0]);
> @@ -974,13 +960,28 @@ static int execute_next_entry(struct
> execute_state *state,
>  		setenv("IGT_SENTINEL_ON_STDERR", "1", 1);
>  
>  		execute_test_process(outfd, errfd, settings, entry);
> +		/* unreachable */
>  	}
>  
> -	/* TODO: Refactor this whole function to use onion teardown */
> +	int outfd = outpipe[0];
> +	int errfd = errpipe[0];

Try to keep variable declarations at the beginning of the block.


Otherwise LGTM,

Reviewed-by: Petri Latvala <petri.latvala@intel.com>
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev