[PATCH v7 1/4] lib/igt_sysfs: Add engine list helpers

[Kamil Konieczny]

Hi Jonathan,
On 2024-11-18 at 18:05:08 +0000, Jonathan Cavitt wrote:
> Create two new helper functions, igt_sysfs_get_engine_list and
> igt_sysfs_free_engine_list, that create and destroy lists of open
> engines, respectively.  The list created by igt_sysfs_get_engine_list
> can be used to iterate over the set of engines in sysfs/engines and must
> be freed by igt_sysfs_free_engine_list after use.
> 
> Signed-off-by: Jonathan Cavitt <jonathan.cavitt at intel.com>

Please check all patches in series with checkpatch.pl and correct
findings.

> ---
>  lib/igt_sysfs.c | 54 +++++++++++++++++++++++++++++++++++++++++++++++++
>  lib/igt_sysfs.h |  3 +++
>  2 files changed, 57 insertions(+)
> 
> diff --git a/lib/igt_sysfs.c b/lib/igt_sysfs.c
> index 00d5822fd3..3d2dd364cf 100644
> --- a/lib/igt_sysfs.c
> +++ b/lib/igt_sysfs.c
> @@ -1307,6 +1307,60 @@ static uint16_t xe_get_engine_class(char *name)
>  	return class;
>  }
>  
> +/**
> + * igt_sysfs_get_engine_list:
> + * @engines: fd of the directory engine
> + * @size: pointer to store the size of the returned char pointer
> + *
> + * Iterates over sysfs/engines and returns an array of
> + * opened engines.  The user will be in charge of closing
> + * the opened engines.
> + *
> + * The number of opened engines will be saved to size.
> + */
> +int *igt_sysfs_get_engine_list(int engines, int *size)
> +{
> +	struct dirent *de;
> +	DIR *dir;
> +	int* ret = calloc(1, sizeof(int));

	int *ret = ...

Assert for NULL here: igt_assert(ret)

Why not 16? So you should avoid realloc?
This would require to keep its size but imho would be better.
Up to you.

> +
> +	lseek(engines, 0, SEEK_SET);
> +
> +	dir = fdopendir(engines);
> +	if (!dir)
> +		close(engines);

Should be:

	if (!dir)
		return NULL;

Btw why do you close engines here?

> +
> +	*size = 0;

Move this to begin of function. Btw do we need size? You could assume
a guard at end with -1 so one param less.


> +	while ((de = readdir(dir))) {
> +		if (*de->d_name == '.')
> +			continue;
> +		ret[*size] = openat(engines, de->d_name, O_RDONLY);
> +		if (ret[*size] < 0) {

Why ignore? imho this is something unexpected?

Overall looks good, thx for fixing this.

Regards,
Kamil

> +			ret[*size] = 0;
> +			continue;
> +		}
> +		*size += 1;
> +		reallocarray(ret, *size + 1, sizeof(int));
> +	}
> +	return ret;
> +}
> +
> +/**
> + * igt_sysfs_free_engine_list:
> + * @list: list of opened engines
> + * @size: number of engines in list
> + *
> + * Helper for cleaning up after igt_sysfs_get_engine_list.
> + * Closes all engines in list before freeing the list.
> + */
> +void igt_sysfs_free_engine_list(int *list, int size)
> +{
> +	int i;
> +	for (i = 0; i < size; i++)
> +		close(list[i]);
> +	free(list);
> +}
> +
>  /**
>   * igt_sysfs_engines:
>   * @xe: fd of the device
> diff --git a/lib/igt_sysfs.h b/lib/igt_sysfs.h
> index 54a4087918..ba0a74755c 100644
> --- a/lib/igt_sysfs.h
> +++ b/lib/igt_sysfs.h
> @@ -168,6 +168,9 @@ typedef struct igt_sysfs_rw_attr {
>  
>  void igt_sysfs_rw_attr_verify(igt_sysfs_rw_attr_t *rw);
>  
> +int *igt_sysfs_get_engine_list(int engines, int *size);
> +void igt_sysfs_free_engine_list(int *list, int size);
> +
>  void igt_sysfs_engines(int xe, int engines, int gt, bool all, const char **property,
>  		       void (*test)(int, int, const char **, uint16_t, int));
>  
> -- 
> 2.43.0
>