[PATCH 3/5] runner/settings: Add function to set IMGDIR environment variable

[Kamil Konieczny]

Hi Naladala,
On 2024-11-19 at 13:22:57 +0530, Naladala Ramanaidu wrote:
> Introduced set_imgdir() function in settings.c to set the IMGDIR

imho better name is IGT_DATA or IGT_IMAGES, also you do not
need to write about which file you changed, it is already in patch
itself.

Swati, do you have suggestions here for this env var?

> environment variable. The function uses absolute_path() to convert
> the relative path "../share/igt-gpu-tools" to an absolute path.
> Added the declaration of set_imgdir() in settings.h with a brief
> description.

Drop this "C to description", it is in patch.

> 
> Signed-off-by: Naladala Ramanaidu <ramanaidu.naladala at intel.com>
> ---
>  runner/runner.c   |  2 ++
>  runner/settings.c | 13 +++++++++++++
>  runner/settings.h |  8 ++++++++
>  3 files changed, 23 insertions(+)
> 
> diff --git a/runner/runner.c b/runner/runner.c
> index 4855ad641..a52834197 100644
> --- a/runner/runner.c
> +++ b/runner/runner.c
> @@ -49,6 +49,8 @@ int main(int argc, char **argv)
>  		exitcode = 1;
>  	}
>  
> +	unsetenv("IMGDIR");

imho you should have a flag and do not unset this
if user sets this before runner starts, e.g. honor
user environment.

> +

Remove this empty line.

>  	printf("Done.\n");

Why this print is here?

While at this, could you add newline here?

>  	return exitcode;
>  }
> diff --git a/runner/settings.c b/runner/settings.c
> index dd4b08dd7..d42f3a68a 100644
> --- a/runner/settings.c
> +++ b/runner/settings.c
> @@ -587,6 +587,15 @@ char *absolute_path(const char *path)
>  	return result;
>  }
>  
> +int set_imgdir(void)
> +{
> +	const char *path = "../share/igt-gpu-tools";
> +	char *abpath;
> +
> +	abpath = absolute_path(path);
> +	return setenv("IMGDIR", abpath, 1);

Repeated string here shows we need a global const char for it.

> +}
> +
>  static char *bin_path(char *fname)
>  {
>  	char *path, *p;
> @@ -863,6 +872,10 @@ bool parse_options(int argc, char **argv,
>  		settings->test_root = absolute_path(env_test_root);
>  	}
>  
> +	if (set_imgdir() != 0) {
> +		usage(stderr, "img dir not set");
> +	}
> +
>  	if (!settings->test_root) {
>  		usage(stderr, "Test root not set");
>  		goto error;
> diff --git a/runner/settings.h b/runner/settings.h
> index 6246d0c3d..a7abe19fd 100644
> --- a/runner/settings.h
> +++ b/runner/settings.h
> @@ -150,4 +150,12 @@ 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);
>  
> +/**
> + * set_imgdir:
> + *
> + * The function will set the environment variable.
> + *
> + */

Description is needed in C source, not in header.

Regards,
Kamil

> +int set_imgdir(void);
> +
>  #endif
> -- 
> 2.43.0
>