| Message ID | 20220422070141.39397-4-xiangxia.m.yue@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | use standard sysctl macro | expand |
On Fri, 22 Apr 2022 15:01:41 +0800 xiangxia.m.yue@gmail.com wrote: > static int __init test_sysctl_init(void) > { > + test_data.match_int[0] = *(int *)SYSCTL_ZERO, > + test_data.match_int[1] = *(int *)SYSCTL_ONE, > + test_data.match_int[2] = *(int *)SYSCTL_TWO, > + test_data.match_int[3] = *(int *)SYSCTL_THREE, > + test_data.match_int[4] = *(int *)SYSCTL_FOUR, > + test_data.match_int[5] = *(int *)SYSCTL_ONE_HUNDRED, > + test_data.match_int[6] = *(int *)SYSCTL_TWO_HUNDRED, > + test_data.match_int[7] = *(int *)SYSCTL_ONE_THOUSAND, > + test_data.match_int[8] = *(int *)SYSCTL_THREE_THOUSAND, > + test_data.match_int[9] = *(int *)SYSCTL_INT_MAX, > + test_data.match_int[10] = *(int *)SYSCTL_MAXOLDUID, > + test_data.match_int[11] = *(int *)SYSCTL_NEG_ONE, > + local VALUES=(0 1 2 3 4 100 200 1000 3000 $INT_MAX 65535 -1) How does this test work? Am I reading it right that it checks if this bash array is in sync with the kernel code? I'd be better if we were checking the values of the constants against literals / defines.
On Tue, Apr 26, 2022 at 3:58 AM Jakub Kicinski <kuba@kernel.org> wrote: > > On Fri, 22 Apr 2022 15:01:41 +0800 xiangxia.m.yue@gmail.com wrote: > > static int __init test_sysctl_init(void) > > { > > + test_data.match_int[0] = *(int *)SYSCTL_ZERO, > > + test_data.match_int[1] = *(int *)SYSCTL_ONE, > > + test_data.match_int[2] = *(int *)SYSCTL_TWO, > > + test_data.match_int[3] = *(int *)SYSCTL_THREE, > > + test_data.match_int[4] = *(int *)SYSCTL_FOUR, > > + test_data.match_int[5] = *(int *)SYSCTL_ONE_HUNDRED, > > + test_data.match_int[6] = *(int *)SYSCTL_TWO_HUNDRED, > > + test_data.match_int[7] = *(int *)SYSCTL_ONE_THOUSAND, > > + test_data.match_int[8] = *(int *)SYSCTL_THREE_THOUSAND, > > + test_data.match_int[9] = *(int *)SYSCTL_INT_MAX, > > + test_data.match_int[10] = *(int *)SYSCTL_MAXOLDUID, > > + test_data.match_int[11] = *(int *)SYSCTL_NEG_ONE, > > > + local VALUES=(0 1 2 3 4 100 200 1000 3000 $INT_MAX 65535 -1) > > How does this test work? Am I reading it right that it checks if this > bash array is in sync with the kernel code? This patch tries to avoid SYSCTL_XXX not mapping the values we hoped, when introducing the new SYSCTL_YYY. for example: SYSCTL_TWO, we hope it is 2, so we check it in userspace. > I'd be better if we were checking the values of the constants against > literals / defines. Hi Jakub, I think this patch checks the values of the constants against defines. But I should make the codes more readable static int __init test_sysctl_init(void) { + int i; + + struct { + int defined; + int wanted; + } match_int[] = { + {.defined = *(int *)SYSCTL_ZERO, .wanted = 0}, + {.defined = *(int *)SYSCTL_ONE, .wanted = 1}, + {.defined = *(int *)SYSCTL_TWO, .wanted = 2}, + {.defined = *(int *)SYSCTL_THREE, .wanted = 3}, + {.defined = *(int *)SYSCTL_FOUR, .wanted = 4}, + {.defined = *(int *)SYSCTL_ONE_HUNDRED, .wanted = 100}, + {.defined = *(int *)SYSCTL_TWO_HUNDRED, .wanted = 200}, + {.defined = *(int *)SYSCTL_ONE_THOUSAND, .wanted = 1000}, + {.defined = *(int *)SYSCTL_THREE_THOUSAND, .wanted = 3000}, + {.defined = *(int *)SYSCTL_INT_MAX, .wanted = INT_MAX}, + {.defined = *(int *)SYSCTL_MAXOLDUID, .wanted = 65535}, + {.defined = *(int *)SYSCTL_NEG_ONE, .wanted = -1}, + }; + + for (i = 0; i < ARRAY_SIZE(match_int); i++) + if (match_int[i].defined != match_int[i].wanted) + match_int_ok = 0; + -- Best regards, Tonghao
On Sun, 1 May 2022 11:31:47 +0800 Tonghao Zhang wrote: > static int __init test_sysctl_init(void) > { > + int i; > + nit: No empty line needed here. > + struct { > + int defined; > + int wanted; > + } match_int[] = { > + {.defined = *(int *)SYSCTL_ZERO, .wanted = 0}, > + {.defined = *(int *)SYSCTL_ONE, .wanted = 1}, > + {.defined = *(int *)SYSCTL_TWO, .wanted = 2}, > + {.defined = *(int *)SYSCTL_THREE, .wanted = 3}, > + {.defined = *(int *)SYSCTL_FOUR, .wanted = 4}, > + {.defined = *(int *)SYSCTL_ONE_HUNDRED, .wanted = 100}, > + {.defined = *(int *)SYSCTL_TWO_HUNDRED, .wanted = 200}, > + {.defined = *(int *)SYSCTL_ONE_THOUSAND, .wanted = 1000}, > + {.defined = *(int *)SYSCTL_THREE_THOUSAND, .wanted = 3000}, > + {.defined = *(int *)SYSCTL_INT_MAX, .wanted = INT_MAX}, > + {.defined = *(int *)SYSCTL_MAXOLDUID, .wanted = 65535}, > + {.defined = *(int *)SYSCTL_NEG_ONE, .wanted = -1}, > + }; > + > + for (i = 0; i < ARRAY_SIZE(match_int); i++) > + if (match_int[i].defined != match_int[i].wanted) > + match_int_ok = 0; That's better, thank you!
diff --git a/lib/test_sysctl.c b/lib/test_sysctl.c index a5a3d6c27e1f..43b8d502f4c7 100644 --- a/lib/test_sysctl.c +++ b/lib/test_sysctl.c @@ -43,6 +43,7 @@ struct test_sysctl_data { int int_0001; int int_0002; int int_0003[4]; + int match_int[12]; int boot_int; @@ -95,6 +96,13 @@ static struct ctl_table test_table[] = { .mode = 0644, .proc_handler = proc_dointvec, }, + { + .procname = "match_int", + .data = &test_data.match_int, + .maxlen = sizeof(test_data.match_int), + .mode = 0444, + .proc_handler = proc_dointvec, + }, { .procname = "boot_int", .data = &test_data.boot_int, @@ -132,6 +140,19 @@ static struct ctl_table_header *test_sysctl_header; static int __init test_sysctl_init(void) { + test_data.match_int[0] = *(int *)SYSCTL_ZERO, + test_data.match_int[1] = *(int *)SYSCTL_ONE, + test_data.match_int[2] = *(int *)SYSCTL_TWO, + test_data.match_int[3] = *(int *)SYSCTL_THREE, + test_data.match_int[4] = *(int *)SYSCTL_FOUR, + test_data.match_int[5] = *(int *)SYSCTL_ONE_HUNDRED, + test_data.match_int[6] = *(int *)SYSCTL_TWO_HUNDRED, + test_data.match_int[7] = *(int *)SYSCTL_ONE_THOUSAND, + test_data.match_int[8] = *(int *)SYSCTL_THREE_THOUSAND, + test_data.match_int[9] = *(int *)SYSCTL_INT_MAX, + test_data.match_int[10] = *(int *)SYSCTL_MAXOLDUID, + test_data.match_int[11] = *(int *)SYSCTL_NEG_ONE, + test_data.bitmap_0001 = kzalloc(SYSCTL_TEST_BITMAP_SIZE/8, GFP_KERNEL); if (!test_data.bitmap_0001) return -ENOMEM; diff --git a/tools/testing/selftests/sysctl/sysctl.sh b/tools/testing/selftests/sysctl/sysctl.sh index 19515dcb7d04..cd74f4749748 100755 --- a/tools/testing/selftests/sysctl/sysctl.sh +++ b/tools/testing/selftests/sysctl/sysctl.sh @@ -40,6 +40,7 @@ ALL_TESTS="$ALL_TESTS 0004:1:1:uint_0001" ALL_TESTS="$ALL_TESTS 0005:3:1:int_0003" ALL_TESTS="$ALL_TESTS 0006:50:1:bitmap_0001" ALL_TESTS="$ALL_TESTS 0007:1:1:boot_int" +ALL_TESTS="$ALL_TESTS 0008:1:1:match_int" function allow_user_defaults() { @@ -785,6 +786,47 @@ sysctl_test_0007() return $ksft_skip } +sysctl_test_0008() +{ + TARGET="${SYSCTL}/match_int" + if [ ! -f $TARGET ]; then + echo "Skipping test for $TARGET as it is not present ..." + return $ksft_skip + fi + + echo -n "Testing if $TARGET is matched with kernel ..." + ORIG_VALUES=$(cat "${TARGET}") + + # SYSCTL_ZERO 0 + # SYSCTL_ONE 1 + # SYSCTL_TWO 2 + # SYSCTL_THREE 3 + # SYSCTL_FOUR 4 + # SYSCTL_ONE_HUNDRED 100 + # SYSCTL_TWO_HUNDRED 200 + # SYSCTL_ONE_THOUSAND 1000 + # SYSCTL_THREE_THOUSAND 3000 + # SYSCTL_INT_MAX INT_MAX + # SYSCTL_MAXOLDUID 65535 + # SYSCTL_NEG_ONE -1 + local VALUES=(0 1 2 3 4 100 200 1000 3000 $INT_MAX 65535 -1) + idx=0 + + for ori in $ORIG_VALUES; do + val=${VALUES[$idx]} + if [ $ori -ne $val ]; then + echo "Expected $val, got $ori, TEST FAILED" + rc=1 + test_rc + fi + + let idx=$idx+1 + done + + echo "ok" + return 0 +} + list_tests() { echo "Test ID list:" @@ -800,6 +842,7 @@ list_tests() echo "0005 x $(get_test_count 0005) - tests proc_douintvec() array" echo "0006 x $(get_test_count 0006) - tests proc_do_large_bitmap()" echo "0007 x $(get_test_count 0007) - tests setting sysctl from kernel boot param" + echo "0008 x $(get_test_count 0008) - tests sysctl macro values match" } usage()