Re: [igt-dev] [PATCH i-g-t 2/3] runner: Refactor metadata parsing

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

On Mon, Apr 01, 2019 at 09:46:56AM +0300, Arkadiusz Hiler wrote:
> To aid testing function parsing metadata.txt is split into outer helper
> that operates on dirfd and inner function that operates on FILE*.
> 
> This allows us to test the parsing using fmemopen(), limiting the amount
> of necessary boilerplate.
> 
> Signed-off-by: Arkadiusz Hiler <arkadiusz.hiler@intel.com>
> ---
>  runner/executor.c     |  2 +-
>  runner/resultgen.c    |  2 +-
>  runner/runner_tests.c |  2 +-
>  runner/settings.c     | 40 ++++++++++++++++++++++++++--------------
>  runner/settings.h     |  4 +++-
>  5 files changed, 32 insertions(+), 18 deletions(-)
> 
> diff --git a/runner/executor.c b/runner/executor.c
> index a40e2dd1..0e91b7ab 100644
> --- a/runner/executor.c
> +++ b/runner/executor.c
> @@ -1112,7 +1112,7 @@ bool initialize_execute_state_from_resume(int dirfd,
>  	memset(state, 0, sizeof(*state));
>  	state->resuming = true;
>  
> -	if (!read_settings(settings, dirfd) ||
> +	if (!read_settings_from_dir(settings, dirfd) ||
>  	    !read_job_list(list, dirfd)) {
>  		close(dirfd);
>  		return false;
> diff --git a/runner/resultgen.c b/runner/resultgen.c
> index 73fda64f..40d1cfcc 100644
> --- a/runner/resultgen.c
> +++ b/runner/resultgen.c
> @@ -1085,7 +1085,7 @@ struct json_object *generate_results_json(int dirfd)
>  	init_settings(&settings);
>  	init_job_list(&job_list);
>  
> -	if (!read_settings(&settings, dirfd)) {
> +	if (!read_settings_from_dir(&settings, dirfd)) {
>  		fprintf(stderr, "resultgen: Cannot parse settings\n");
>  		return NULL;
>  	}
> diff --git a/runner/runner_tests.c b/runner/runner_tests.c
> index e46568ac..06725536 100644
> --- a/runner/runner_tests.c
> +++ b/runner/runner_tests.c
> @@ -680,7 +680,7 @@ igt_main
>  				     "Opening %s/metadata.txt failed\n", dirname);
>  			close(fd);
>  
> -			igt_assert_f(read_settings(&cmp_settings, dirfd), "Reading settings failed\n");
> +			igt_assert_f(read_settings_from_dir(&cmp_settings, dirfd), "Reading settings failed\n");
>  			assert_settings_equal(&settings, &cmp_settings);
>  		}
>  
> diff --git a/runner/settings.c b/runner/settings.c
> index e64244e6..20b21378 100644
> --- a/runner/settings.c
> +++ b/runner/settings.c
> @@ -540,7 +540,7 @@ bool serialize_settings(struct settings *settings)
>  #undef SERIALIZE_LINE
>  }
>  
> -bool read_settings(struct settings *settings, int dirfd)
> +bool read_settings_from_file(struct settings *settings, FILE *f)
>  {
>  #define PARSE_LINE(s, name, val, field, write) \
>  	if (!strcmp(name, #field)) {	       \
> @@ -551,20 +551,8 @@ bool read_settings(struct settings *settings, int dirfd)
>  		continue;		       \
>  	}
>  
> -	int fd;
> -	FILE *f;
>  	char *name = NULL, *val = NULL;
>  
> -	free_settings(settings);
> -
> -	if ((fd = openat(dirfd, settings_filename, O_RDONLY)) < 0)
> -		return false;
> -
> -	f = fdopen(fd, "r");
> -	if (!f) {
> -		close(fd);
> -		return false;
> -	}
>  
>  	while (fscanf(f, "%ms : %ms", &name, &val) == 2) {
>  		int numval = atoi(val);
> @@ -592,9 +580,33 @@ bool read_settings(struct settings *settings, int dirfd)
>  
>  	free(name);
>  	free(val);
> -	fclose(f);
>  
>  	return true;
>  
>  #undef PARSE_LINE
>  }
> +
> +bool read_settings_from_dir(struct settings *settings, int dirfd)
> +{
> +	int fd;
> +	FILE *f;
> +
> +


Extra line here


> +	free_settings(settings);
> +
> +	if ((fd = openat(dirfd, settings_filename, O_RDONLY)) < 0)
> +		return false;
> +
> +	f = fdopen(fd, "r");
> +	if (!f) {
> +		close(fd);
> +		return false;
> +	}
> +
> +	if (!read_settings_from_file(settings, f))
> +		return false;

f leaked if read_settings_from_file fails



With those fixed,
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