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

[Cavitt, Jonathan]

-----Original Message-----
From: Kamil Konieczny <kamil.konieczny at linux.intel.com> 
Sent: Tuesday, December 3, 2024 10:31 AM
To: Cavitt, Jonathan <jonathan.cavitt at intel.com>
Cc: igt-dev at lists.freedesktop.org; Gupta, saurabhg <saurabhg.gupta at intel.com>; Zuo, Alex <alex.zuo at intel.com>; Belgaumkar, Vinay <vinay.belgaumkar at intel.com>
Subject: Re: [PATCH v8 1/4] lib/igt_sysfs: Add engine list helpers
> 
> Hi Jonathan,
> On 2024-12-02 at 18:08:13 +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>
> > ---
> >  lib/igt_sysfs.c | 60 +++++++++++++++++++++++++++++++++++++++++++++++++
> >  lib/igt_sysfs.h |  3 +++
> >  2 files changed, 63 insertions(+)
> > 
> > diff --git a/lib/igt_sysfs.c b/lib/igt_sysfs.c
> > index 00d5822fd3..efb071bfe9 100644
> > --- a/lib/igt_sysfs.c
> > +++ b/lib/igt_sysfs.c
> > @@ -1307,6 +1307,66 @@ static uint16_t xe_get_engine_class(char *name)
> >  	return class;
> >  }
> >  
> > +/**
> > + * igt_sysfs_get_engine_list:
> > + * @engines: fd of the directory engine
> > + *
> > + * Iterates over sysfs/engines and returns an array of
> > + * opened engines.  The user will be in charge of closing
> > + * the opened engines.
> > + *
> > + * The returned array will always be terminated by a -1.
> > + */
> > +int *igt_sysfs_get_engine_list(int engines)
> > +{
> > +	struct dirent *de;
> > +	DIR *dir;
> > +#define ARRAY_MAX	16
> 
> Use cont int here, like:
> 	cont int max_engines = 16;

I'll do it, but this seems very much like we're intentionally pushing the
definition of what a "magic value" is.

> 
> > +	int *ret = calloc(ARRAY_MAX, sizeof(int));
> > +	int size = 0;
> > +
> > +	igt_assert(ret);
> > +
> > +	lseek(engines, 0, SEEK_SET);
> > +
> > +	dir = fdopendir(engines);
> > +	if (!dir)
> > +		close(engines);
> 
> Add 'else' here.

The manual page for readdir states that if the function fails, NULL is returned and
errno is set.  Passing readdir a NULL pointer is one such failure case, returning
NULL and causing the while loop to exit immediately before it starts.  In this case,
the returned array will only contain the terminator value -1.  This will essentially
report a size of 0 and cause the test case this is used in to result in failure.

In short, we don't need a contingency plan for if the directory failed to open
because readdir already handles the "failed to open" case in a way that can
benefit from.

> 
> > +
> > +	while ((de = readdir(dir))) {
> > +		if (*de->d_name == '.')
> > +			continue;
> > +		ret[size] = openat(engines, de->d_name, O_RDONLY);
> > +		if (ret[size] < 0) {
> > +			ret[size] = 0;
> > +			continue;
> > +		}
> > +		size += 1;
> > +		igt_assert_lt(size, ARRAY_MAX);
> 
> Move this assert before assignment 'ret[size] = ...'

If size == ARRAY_MAX after incrementing size, then assigning the terminator
value -1 to position "size" in the array (see below) will overrun the array.
I can move the assert to before the assignment, but because we need the
position size+1 to be available for the terminator value after the while loop,
we'd need to assert that size < ARRAY_MAX - 1 at that position to account
for the later incrementation.

I think that implementation of the assert would make the purpose of the
assert less clear, though.  Also, we might hit some false positives doing it
that way; For example, if the engines directory has 20 elements, and only
the first 15 are valid, then we'd technically have enough room to store the
engines and the terminator since we'd not have to store the last 5 invalid
elements.  However, we'd hit the assert anyways because we wouldn't
check those elements beforehand.

The alternative would be to move the assert back to before the assignment
without updating the check value, but then we'd need to check the size
again outside of the loop before we add the terminator value.  Not that
there's anything inherently wrong with that approach, it just seems a bit
inefficient compared to the current implementation.

I'll take the second approach.
-Jonathan Cavitt

> 
> > +	}
> > +
> > +	ret[size] = -1;
> 
> Add newline here.
> 
> 
> > +	return ret;
> > +}
> > +#undef ARRAY_MAX
> 
> Remove this.
> 
> Rest looks good.
> 
> Regards,
> Kamil
> 
> 
> > +
> > +/**
> > + * 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 i = 0;
> > +
> > +	while (list[i] != -1)
> > +		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..86345f3d1b 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);
> > +void igt_sysfs_free_engine_list(int *list);
> > +
> >  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
> > 
>