[PATCH v5 i-g-t 1/1] tests/intel/xe_sysfs_scheduler: Add invalid string test for engine sysfs properties

[Piatkowski, Dominik Karol]

Hi Kamil, hi Sobin,

> -----Original Message-----
> From: Kamil Konieczny <kamil.konieczny at linux.intel.com>
> Sent: Wednesday, April 9, 2025 6:14 PM
> To: Thomas, Sobin <sobin.thomas at intel.com>
> Cc: igt-dev at lists.freedesktop.org; Piatkowski, Dominik Karol
> <dominik.karol.piatkowski at intel.com>; Konieczny, Kamil
> <kamil.konieczny at intel.com>
> Subject: Re: [PATCH v5 i-g-t 1/1] tests/intel/xe_sysfs_scheduler: Add invalid
> string test for engine sysfs properties
> 
> Hi Sobin,
> On 2025-04-09 at 02:22:17 +0000, Sobin Thomas wrote:
> > This test validates that invalid string inputs are correctly rejected
> > by engine sysfs write. It ensures that the property values remain
> > unchanged when invalid inputs are provided.
> >
> > v5: Fixed documentation and fixed error.
> >
> > v4: Added subtests to test large (4k) random strings (kamil).
> > Replaced igt macro.
> >
> > v3: Fixed the test logic. Fixed the tabs
> >
> > v2: Added error check for return values for igt_sysfs_scanf and
> > igt_sysfs_printf. Removed the changes for fault injection in
> > tests/intel/xe_fault_injection.c
> >
> > v1: Initial changes for checking error string.
> >
> > Signed-off-by: Sobin Thomas <sobin.thomas at intel.com>
> > ---
> >  tests/intel/xe_sysfs_scheduler.c | 98
> ++++++++++++++++++++++++++++++++
> >  1 file changed, 98 insertions(+)
> >
> > diff --git a/tests/intel/xe_sysfs_scheduler.c
> b/tests/intel/xe_sysfs_scheduler.c
> > index 4fc764f82..c4386ffce 100644
> > --- a/tests/intel/xe_sysfs_scheduler.c
> > +++ b/tests/intel/xe_sysfs_scheduler.c
> > @@ -15,6 +15,16 @@
> >   *		unrepresentable intervals.
> >   * Test category: negative test
> >   *
> > + * SUBTEST: %s-invalid-string
> > + * Description: Test to check if %s arg[1] schedule parameter checks for
> > + *		invalid string values.
> > + * Test category: negative test
> > + *
> > + * SUBTEST: %s-invalid-large-string
> > + * Description: Test to check if %s arg[1] schedule parameter checks for
> > + *		large invalid strings (4k).
> > + * Test category: negative test
> > + *
> >   * SUBTEST: %s-min-max
> >   * Description: Test to check if %s arg[1] schedule parameter checks for
> >   *		min max values.
> > @@ -39,6 +49,46 @@
> >  #include "xe_drm.h"
> >  #include "xe/xe_query.h"
> >
> > +#define STR_LENGTH 4096
> > +
> > +/**
> > + * generate_random_string:
> > + * @str: pointer to string buffer that will be having random string generated.
> > + * @length: length of string to generate, excluding null termination
> > + *
> > + * Generates random string that will always contain non-numerical
> characters.
> > + */
> > +static void generate_random_string(char *str, size_t length)
> > +{
> > +	int type = 0;
> > +	int digit_count = 0;
> > +	int max_digits = length / 4;
> > +
> > +	for (size_t i = 0; i < length; i++) {
> > +		if (digit_count >= max_digits)
> > +			type = rand() % 2;
> > +		else
> > +			type = rand() % 3;
> > +
> > +		switch (type) {
> > +		case 0:
> > +			str[i] = 'A' + (rand() % 26);
> > +			break;
> > +		case 1:
> > +			str[i] = 'a' + (rand() % 26);
> > +			break;
> > +		case 2:
> > +			str[i] = '0' + (rand() % 10);
> > +			digit_count++;
> > +			break;
> > +		default:
> > +			str[i] = '_';
> > +			break;
> > +		}
> > +	}
> > +	str[length] = '\0';
> 
> imho this could be dangerous as in all libs len for strings
> means 0...len-1
> 
> Why not just
> 
> 	str[length - 1] = '\0';

Behavior of this function is documented, but true, it's non-standard.
Keep my r-b in v6 with this change.

Thanks,
Dominik Karol

> 
> Regards,
> Kamil
> 
> > +}
> > +
> >  static void test_invalid(int xe, int engine, const char **property,
> >  			 uint16_t class, int gt)
> >  {
> > @@ -121,6 +171,52 @@ static void test_min_max(int xe, int engine, const
> char **property,
> >  	igt_assert_eq(set, default_max);
> >  }
> >
> > +static void test_invalid_string(int xe, int engine, const char **property,
> > +				 uint16_t class, int gt)
> > +{
> > +	unsigned int saved, set;
> > +	static const char invalid_input[] = "999abc";
> > +
> > +	for (int i = 0; i < 3; i++) {
> > +		igt_assert_eq(igt_sysfs_scanf(engine, property[i], "%u",
> &saved), 1);
> > +		igt_info("Initial %s: %u\n", property[i], saved);
> > +		/* Assert if the invalid write is returning negative error */
> > +		igt_assert_lt(igt_sysfs_printf(engine, property[i], "%s",
> > +			      invalid_input), 0);
> > +
> > +		igt_assert_eq(igt_sysfs_scanf(engine, property[i], "%u",
> &set), 1);
> > +		/* Check if the values are unchanged. */
> > +		igt_assert_eq(set, saved);
> > +	}
> > +}
> > +
> > +static void test_invalid_large_string(int xe, int engine, const char
> **property,
> > +				       uint16_t class, int gt)
> > +{
> > +	unsigned int saved, set;
> > +	char *random_str;
> > +
> > +	random_str = (char *)malloc(sizeof(char) * (STR_LENGTH + 1));
> > +	igt_assert_f(random_str, "Memory allocation failed\n");
> > +
> > +	generate_random_string(random_str, STR_LENGTH);
> > +	igt_debug("Generated random string: %.10s...\n", random_str);
> > +
> > +	for (int i = 0; i < 3; i++) {
> > +		igt_assert_eq(igt_sysfs_scanf(engine, property[i], "%u",
> &saved), 1);
> > +		igt_info("Initial %s: %u\n", property[i], saved);
> > +
> > +		/* Assert if the invalid write is returning negative error */
> > +		igt_assert_lt(igt_sysfs_printf(engine, property[i], "%s",
> > +			      random_str), 0);
> > +
> > +		igt_assert_eq(igt_sysfs_scanf(engine, property[i], "%u",
> &set), 1);
> > +		/* Check if the values are unchanged. */
> > +		igt_assert_eq(set, saved);
> > +	}
> > +	free(random_str);
> > +}
> > +
> >  #define MAX_GTS 8
> >  igt_main
> >  {
> > @@ -129,6 +225,8 @@ igt_main
> >  		void (*fn)(int, int, const char **, uint16_t, int);
> >  	} tests[] = {
> >  		{ "invalid", test_invalid },
> > +		{ "invalid-string", test_invalid_string },
> > +		{ "invalid-large-string", test_invalid_large_string },
> >  		{ "min-max", test_min_max },
> >  		{ }
> >  	};
> > --
> > 2.34.1
> >