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

[Kamil Konieczny]

Hi,
On 2025-04-01 at 12:59:54 +0000, Piatkowski, Dominik Karol wrote:
> Hi Sobin,
> 
> > -----Original Message-----
> > From: Thomas, Sobin <sobin.thomas at intel.com>
> > Sent: Tuesday, April 1, 2025 1:45 PM
> > To: igt-dev at lists.freedesktop.org
> > Cc: Piatkowski, Dominik Karol <dominik.karol.piatkowski at intel.com>
> > Subject: [PATCH v3 i-g-t 1/1] tests/intel/xe_sysfs_scheduler: Add invalid string
> > test for engine sysfs properties
> > 
> > From: Sobin Thomas <sobin.thomas at intel.com>
> > 
> > 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.
> > 
> > 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 | 28 ++++++++++++++++++++++++++++
> >  1 file changed, 28 insertions(+)
> > 
> > diff --git a/tests/intel/xe_sysfs_scheduler.c b/tests/intel/xe_sysfs_scheduler.c
> > index 4fc764f82..eb95d0d6e 100644
> > --- a/tests/intel/xe_sysfs_scheduler.c
> > +++ b/tests/intel/xe_sysfs_scheduler.c
> > @@ -10,6 +10,11 @@
> >   * Sub-category: SysMan tests
> >   * Functionality: scheduler control interface
> >   *
> > + * SUBTEST: %s-invalid-string
> > + * Description: Test to check if %s arg[1] schedule parameter checks for
> > + *		min max values.
> > + * Test category: Negative string test
> > + *
> >   * SUBTEST: %s-invalid
> >   * Description: Test to check if %s arg[1] schedule parameter rejects any
> >   *		unrepresentable intervals.
> > @@ -121,6 +126,28 @@ 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)
> 
> One tab disappeared from this indent, it was present in v2, please fix.
> 
> > +{
> > +	unsigned int saved, set;
> > +	char invalid_input[] = "999abc";
> > +
> > +	for (int i = 0; i < 3; i++) {
> > +		igt_assert(igt_sysfs_scanf(engine, property[i], "%u", &saved)
> > == 1);
> > +		igt_info("Initial %s: %u\n", property[i], saved);
> > +		if (igt_sysfs_printf(engine, property[i], "%s", invalid_input) >
> > 0) {
> > +			igt_critical("invalid value %s can be set to property
> > %s\n",
> > +					invalid_input, property[i]);
> 
> Please replace the last indenting tab with 5 spaces for perfect alignment.
> 
> > +		}
> 
> We don't use braces for single statements - please remove them from this if.
> 
> > +
> > +		igt_assert(igt_sysfs_scanf(engine, property[i], "%u", &set) ==
> > 0);

Here also it is better to use _eq macro:

	igt_assert_eq(igt_sysfs_scanf(engine, property[i], "%u", &set), 0);

> > +
> > +		//Check if the values are unchanged.
> 
> I believe we prefer /* C-style comments */
> +cc Kamil

Yes, please use C-style comments. Also for white space errors please
use checkpatch.pl script from Linux kernel source, for some oprtions
for its usage look into CONTRIBUTING.md

> 
> With mentioned changes,
> LGTM,
> Reviewed-by: Dominik Karol Piątkowski <dominik.karol.piatkowski at intel.com>

After fixing above nits please add Dominik's r-b and resend.

Regards,
Kamil

> 
> > +		igt_assert_eq(set, saved);
> > +	}
> > +}
> > +
> > +
> >  #define MAX_GTS 8
> >  igt_main
> >  {
> > @@ -130,6 +157,7 @@ igt_main
> >  	} tests[] = {
> >  		{ "invalid", test_invalid },
> >  		{ "min-max", test_min_max },
> > +		{ "invalid-string", test_invalid_string },
> >  		{ }
> >  	};
> > 
> > --
> > 2.34.1
>