[LTP] [PATCH v2 6/6] syscalls/set_thread_area01: Refactor into new API
Andrea Cervesato
andrea.cervesato@suse.com
Fri Mar 28 10:50:37 CET 2025
Hi!
This test is weird. It stays inside set_thread_area folder, but it's
testing get_thread_area(2) as well. We should probably create a new
testing folder for get_thread_area(2), splitting test in two, doing the
exact same thing: verifying that syscalls can be executed correctly and
raises the correct errors.
More comments below.
On 3/27/25 15:28, Ricardo B. Marliere via ltp wrote:
> From: Ricardo B. Marlière <rbm@suse.com>
>
> Signed-off-by: Ricardo B. Marlière <rbm@suse.com>
> ---
> .../syscalls/set_thread_area/set_thread_area.h | 31 ------
> .../syscalls/set_thread_area/set_thread_area01.c | 120 ++++++---------------
> 2 files changed, 35 insertions(+), 116 deletions(-)
>
> diff --git a/testcases/kernel/syscalls/set_thread_area/set_thread_area.h b/testcases/kernel/syscalls/set_thread_area/set_thread_area.h
> deleted file mode 100644
> index 2bd2469d549ecf8c5c589a0a9485f886d043f7ed..0000000000000000000000000000000000000000
> --- a/testcases/kernel/syscalls/set_thread_area/set_thread_area.h
> +++ /dev/null
> @@ -1,31 +0,0 @@
> -#include <stdio.h>
> -#include <errno.h>
> -
> -/* Harness Specific Include Files. */
> -#include "test.h"
> -#include "lapi/syscalls.h"
> -#include "config.h"
> -
> -#if defined HAVE_ASM_LDT_H
> -#include <linux/unistd.h>
> -#include <asm/ldt.h>
> -
> -#if defined HAVE_STRUCT_USER_DESC
> -typedef struct user_desc thread_area_s;
> -#elif defined HAVE_STRUCT_MODIFY_LDT_LDT_S
> -typedef struct modify_ldt_ldt_s thread_area_s;
> -#else
> -typedef struct user_desc {
> - unsigned int entry_number;
> - unsigned long int base_addr;
> - unsigned int limit;
> - unsigned int seg_32bit:1;
> - unsigned int contents:2;
> - unsigned int read_exec_only:1;
> - unsigned int limit_in_pages:1;
> - unsigned int seg_not_present:1;
> - unsigned int useable:1;
> - unsigned int empty:25;
> -} thread_area_s;
> -#endif /* HAVE_STRUCT_USER_DESC */
> -#endif /* HAVE_ASM_LDT_H */
> diff --git a/testcases/kernel/syscalls/set_thread_area/set_thread_area01.c b/testcases/kernel/syscalls/set_thread_area/set_thread_area01.c
> index 30626d5e90ebf8e20624c370cc4474cb51a6b102..bbc219756b414617222435416c6ecd63a06c5b41 100644
> --- a/testcases/kernel/syscalls/set_thread_area/set_thread_area01.c
> +++ b/testcases/kernel/syscalls/set_thread_area/set_thread_area01.c
> @@ -1,60 +1,37 @@
> -/*************************************************************************
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> * Copyright (c) Crackerjack Project., 2007
> * Copyright (c) Manas Kumar Nayak <maknayak@in.ibm.com>
> * Copyright (c) Cyril Hrubis <chrubis@suse.cz> 2011
> - *
> - * This program is free software; you can redistribute it and/or modify
> - * it under the terms of the GNU General Public License as published by
> - * the Free Software Foundation; either version 2 of the License, or
> - * (at your option) any later version.
> - *
> - * This program is distributed in the hope that it will be useful,
> - * but WITHOUT ANY WARRANTY; without even the implied warranty of
> - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See
> - * the GNU General Public License for more details.
> - *
> - * You should have received a copy of the GNU General Public License
> - * along with this program; if not, write to the Free Software
> - * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
> - *
> - ************************************************************************/
> + * Copyright (c) 2025 SUSE LLC Ricardo B. Marlière <rbm@suse.com>
> + */
>
> -#include "set_thread_area.h"
> +/*\
> + * The test will first call set_thread_area to a struct user_desc pointer with
> + * entry_number = -1, which will be set to a free entry_number upon exiting.
> + * Therefore, a subsequent call to get_thread_area using the same pointer will
> + * be valid. The same process is done but using -2 instead, which returns EINVAL
> + * to both calls. Finally, it's done one more time but to an invalid pointer and
> + * therefore an EFAULT is returned.
This is a really specific technical explanation which could be easily
generated by AI or a person who's reading the test code :-)
What we need in the tests description is something that describe the
logic behind the test, abstracting from the technical implementations,
which could stay in [Algorithm] part.
In short, we should be able to read the description at first glance and
to understand the reason why a test has been made for.
> + */
>
> -char *TCID = "set_thread_area_01";
> -int TST_TOTAL = 6;
> +#include "tst_test.h"
>
> -#if defined(HAVE_ASM_LDT_H) && defined(HAVE_STRUCT_USER_DESC)
> +#ifdef __i386__
> +#include "lapi/ldt.h"
>
> -static void cleanup(void)
> -{
> -}
> +#define VALUE_AND_STRING(val) val, #val
We never use syscall_name in the whole test, so we can remove this macro
which is complicating the code.
>
> -static void setup(void)
> -{
> - TEST_PAUSE;
> -}
> +static struct user_desc u_info1 = {.entry_number = -1 };
> +static struct user_desc u_info2 = {.entry_number = -2 };
>
> -struct test {
> +static struct tst_case {
> int syscall;
This is not needed if we split the test.
> const char *const syscall_name;
This is never used.
> - thread_area_s *u_info;
> + struct user_desc *u_info;
> int exp_ret;
> int exp_errno;
> -};
> -
> -/*
> - * The set_thread_area uses a free entry_number if entry number is set to -1
> - * and upon the syscall exit the entry number is set to entry which was used.
> - * So when we call get_thread_area on u_info1, the entry number is initalized
> - * corectly by the previous set_thread_area.
> - */
> -static struct user_desc u_info1 = {.entry_number = -1 };
> -static struct user_desc u_info2 = {.entry_number = -2 };
> -
> -#define VALUE_AND_STRING(val) val, #val
> -
> -static struct test tests[] = {
> +} tst_cases[] = {
> {VALUE_AND_STRING(__NR_set_thread_area), &u_info1, 0, 0},
> {VALUE_AND_STRING(__NR_get_thread_area), &u_info1, 0, 0},
> {VALUE_AND_STRING(__NR_set_thread_area), &u_info2, -1, EINVAL},
> @@ -63,49 +40,22 @@ static struct test tests[] = {
> {VALUE_AND_STRING(__NR_get_thread_area), (void *)-9, -1, EFAULT},
We have 2 more errno to test according to man pages. We should probably
add them.
> };
>
> -int main(int argc, char *argv[])
> -{
> - int lc;
> - unsigned i;
> -
> - tst_parse_opts(argc, argv, NULL, NULL);
> -
> - setup();
>
> - for (lc = 0; TEST_LOOPING(lc); lc++) {
> - for (i = 0; i < sizeof(tests) / sizeof(struct test); i++) {
> - TEST(tst_syscall(tests[i].syscall, tests[i].u_info));
> -
> - if (TEST_RETURN != tests[i].exp_ret) {
> - tst_resm(TFAIL, "%s returned %li expected %i",
> - tests[i].syscall_name,
> - TEST_RETURN, tests[i].exp_ret);
> - continue;
> - }
> +void run(unsigned int i)
> +{
> + struct tst_case *tc = &tst_cases[i];
>
> - if (TEST_ERRNO != tests[i].exp_errno) {
> - tst_resm(TFAIL,
> - "%s failed with %i (%s) expected %i (%s)",
> - tests[i].syscall_name, TEST_ERRNO,
> - strerror(TEST_ERRNO),
> - tests[i].exp_errno,
> - strerror(tests[i].exp_errno));
> - continue;
> - }
> + if (tc->exp_errno)
> + TST_EXP_FAIL(tst_syscall(tc->syscall, tc->u_info), tc->exp_errno);
> + else
> + TST_EXP_PASS(tst_syscall(tc->syscall, tc->u_info));
> +}
>
> - tst_resm(TPASS, "%s returned %li errno %i (%s)",
> - tests[i].syscall_name, TEST_RETURN,
> - TEST_ERRNO, strerror(TEST_ERRNO));
> - }
> - }
> +static struct tst_test test = {
> + .test = run,
> + .tcnt = ARRAY_SIZE(tst_cases),
> +};
>
> - cleanup();
> - tst_exit();
> -}
> #else
> -int main(void)
> -{
> - tst_brkm(TCONF, NULL,
> - "set_thread_area isn't available for this architecture");
> -}
> -#endif
> +TST_TEST_TCONF("Test supported only on i386");
> +#endif /* __i386__ */
>
I guess we can skip this test from the modify_ldt testing suite and
create a new patch-set only for these 2 syscalls: get_thread_area(2) and
set_thread_area(2).
Kind Regards,
Andrea Cervesato
More information about the ltp
mailing list