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

[Kamil Konieczny]

Hi Cavitt,,
On 2024-12-03 at 22:24:46 +0000, Cavitt, Jonathan wrote:
> -----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.
> 

Right, if you want in future use realloc it should be just 'int',
not 'const int'.

> > 
> > > +	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.
> 

You are right.

> > 
> > > +
> > > +	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
> 

And here you are also right, I forgot about that last
guard added after 'while.

Regards,
Kamil

> > 
> > > +	}
> > > +
> > > +	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
> > > 
> >