[PATCH i-g-t v2 3/5] runner/settings: Add function to set IGT_RUNNER_DATA environment variable

[Kamil Konieczny]

Hi Naladala,
On 2025-01-08 at 01:36:12 +0530, Naladala Ramanaidu wrote:
> Introduced set_runner_datadir() to set the IGT_RUNNER_DATA environment
> variable. This function uses absolute_path() to convert the relative
> path "../share/igt-gpu-tools" to an absolute path. 

> Update the
> runner/runner code to call set_runner_datadir() and handle errors
> appropriately.

Please do not translate code into words, is it there in a patch itself.
imho here the most important part is description why we need it and
what problem you try to resolve.

> 
> v2: Fix review comments. (Kamil)
> 
> Signed-off-by: Naladala Ramanaidu <ramanaidu.naladala at intel.com>
> ---
>  runner/runner.c   | 8 ++++++++
>  runner/settings.c | 9 +++++++++
>  runner/settings.h | 1 +
>  3 files changed, 18 insertions(+)
> 
> diff --git a/runner/runner.c b/runner/runner.c
> index 4855ad641..950eb5662 100644
> --- a/runner/runner.c
> +++ b/runner/runner.c
> @@ -12,8 +12,13 @@ int main(int argc, char **argv)
>  	struct job_list job_list;
>  	struct execute_state state;
>  	int exitcode = 0;
> +	int data_flag = 0;
>  
>  	init_settings(&settings);
> +	data_flag = set_runner_datadir();
> +	if (data_flag)
> +		fprintf(stderr, "Data dir path not set\n");

Why an error? You could check if './data' path is present
but this imho should be done in init_settings(), not here.

> +
>  	init_job_list(&job_list);
>  
>  	if (!parse_options(argc, argv, &settings)) {
> @@ -49,6 +54,9 @@ int main(int argc, char **argv)
>  		exitcode = 1;
>  	}
>  
> +	if (!data_flag)
> +		unsetenv("IGT_RUNNER_DATA");
> +
>  	printf("Done.\n");
>  	return exitcode;
>  }
> diff --git a/runner/settings.c b/runner/settings.c
> index bea0c3059..c8220be32 100644
> --- a/runner/settings.c
> +++ b/runner/settings.c
> @@ -589,6 +589,15 @@ char *absolute_path(const char *path)
>  	return result;
>  }
>  
> +int set_runner_datadir(void)

The only user of this function is in runner.c
so why do you need it here?

> +{
> +	const char *datapath = "../share/igt-gpu-tools";
> +	char *abpath;
> +
> +	abpath = absolute_path(datapath);
> +	return setenv("IGT_RUNNER_DATA", abpath, 1);

Why are you setting this var here? It could be set by a user
so why are you overwriting it in that case?

imho you do not need this function but maybe I am missing something?

Regards,
Kamil

> +}
> +
>  static char *bin_path(char *fname)
>  {
>  	char *path, *p;
> diff --git a/runner/settings.h b/runner/settings.h
> index 7e6cd11e2..5926817b6 100644
> --- a/runner/settings.h
> +++ b/runner/settings.h
> @@ -150,5 +150,6 @@ bool serialize_settings(struct settings *settings);
>  
>  bool read_settings_from_file(struct settings *settings, FILE* f);
>  bool read_settings_from_dir(struct settings *settings, int dirfd);
> +int set_runner_datadir(void);
>  
>  #endif
> -- 
> 2.43.0
>