* RE: [PATCH i-g-t 1/1] tests/intel/xe_sysfs_scheduler: Add invalid string test for engine sysfs properties
2025-04-08 4:18 ` [PATCH i-g-t 1/1] " Sobin Thomas
@ 2025-04-08 9:32 ` Piatkowski, Dominik Karol
0 siblings, 0 replies; 3+ messages in thread
From: Piatkowski, Dominik Karol @ 2025-04-08 9:32 UTC (permalink / raw)
To: Thomas, Sobin, igt-dev@lists.freedesktop.org; +Cc: Konieczny, Kamil
Hi Sobin,
> -----Original Message-----
> From: Thomas, Sobin <sobin.thomas@intel.com>
> Sent: Tuesday, April 8, 2025 6:19 AM
> To: igt-dev@lists.freedesktop.org
> Cc: Piatkowski, Dominik Karol <dominik.karol.piatkowski@intel.com>;
> Konieczny, Kamil <kamil.konieczny@intel.com>
> Subject: [PATCH i-g-t 1/1] tests/intel/xe_sysfs_scheduler: Add invalid string
> test for engine sysfs properties
Please remember to add version number to patch subject.
>
> 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.
>
> 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@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..285689778 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 function is supposed to generate random
> + * string with more bias on alphabets than numeric this function
> + * provides random string based on the output of rand() and can
> + * sometime provide same class of characters if the output of
> + * rand() is same.
> + */
Nitpick: this documentation could be improved. I propose:
/**
* generate_random_string:
* @str: pointer to string buffer
* @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_digts)
digit_count > max_digits will never happen, but >= comparison will still work.
There is a typo as well, it should be `max_digits` instead of `max_digts`.
This causes build to fail.
> + 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';
> +}
> +
> 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 *)calloc(STR_LENGTH, sizeof(char));
You can use malloc instead of calloc here, as we will fully overwrite the buffer
anyway.
Also, one very important note - you're allocating buffer of size STR_LENGTH,
but the generate_random_string function modifies random_str[STR_LENGTH] and
that is a buffer overflow. You need to allocate STR_LENGTH + 1 to accommodate
the null terminator. This needs to be fixed.
> + 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);
Please align this line.
With issues fixed,
Reviewed-by: Dominik Karol Piątkowski <dominik.karol.piatkowski@intel.com>
> +
> + 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
^ permalink raw reply [flat|nested] 3+ messages in thread