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

["Sharma, Swati2" <swati2.sharma@intel.com>]

Hi Ramanaidu,

On 11-12-2024 02:44 pm, Kamil Konieczny wrote:
> 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?
Agree with Kamil, IGT_DATA might be better choice for environment variable
which is inline with other env var too like 
IGT_RUNNER_DISABLE_SOCKET_COMMUNICATION,
IGT_HOOK*, etc.
>> 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@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
>>