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

Petri Latvala
2019-04-01T10:24:41Z

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>