[LTP] [PATCH v3] syscalls/set_thread_area01: Refactor into new API
Andrea Cervesato
andrea.cervesato@suse.com
Fri Apr 4 14:03:19 CEST 2025
Hi Ricardo,
On 4/3/25 18:40, Ricardo B. Marli��re via ltp wrote:
> On Thu Apr 3, 2025 at 12:59 PM -03, Ricardo B. Marlière wrote:
>> From: Ricardo B. Marlière <rbm@suse.com>
>>
>> Signed-off-by: Ricardo B. Marlière <rbm@suse.com>
>> ---
>> Changes in v3:
>> - Moved syscall wrappers to include/lapi/ldt.h
>> - Split the test into a new set_thread_area02.c file
>> - Used guarded buffers
>> - Used .supported_archs instead of #ifdef __i386__
>> - Used .test_variants and .tcnt+.test in set_thread_area02.c
>> - Tweaked test decriptions
>> - Link to v2: https://lore.kernel.org/r/20250403-conversions-set_thread_area-v2-1-a177e5c2b28d@suse.com
>>
>> Changes in v2:
>> - Used tst_test.test_all instead of .test
>> - Removed VALUE_AND_STRING macro
>> - Added local wrappers to the syscalls
>> - Fixed the test description
>> - Added a comment quote from the set_thread_area manual to add context
>> - Link to v1: https://lore.kernel.org/all/20250327-conversions-modify_ldt-v2-6-2907d4d3f6c0@suse.com
>> ---
>> include/lapi/ldt.h | 11 ++
>> .../kernel/syscalls/set_thread_area/.gitignore | 3 +-
>> .../syscalls/set_thread_area/set_thread_area.h | 31 -----
>> .../syscalls/set_thread_area/set_thread_area01.c | 140 +++++++--------------
>> .../syscalls/set_thread_area/set_thread_area02.c | 62 +++++++++
>> 5 files changed, 120 insertions(+), 127 deletions(-)
>>
>> diff --git a/include/lapi/ldt.h b/include/lapi/ldt.h
>> index 068a577cf113ba47a45d29920c6adb6851f4fc52..f9e70dd3de53ab5ad4fb9cff6552f39b49c7a3ac 100644
>> --- a/include/lapi/ldt.h
>> +++ b/include/lapi/ldt.h
>> @@ -33,4 +33,15 @@ static inline int safe_modify_ldt(const char *file, const int lineno, int func,
>> #define SAFE_MODIFY_LDT(func, ptr, bytecount) \
>> safe_modify_ldt(__FILE__, __LINE__, (func), (ptr), (bytecount))
>>
>> +static inline int set_thread_area(const struct user_desc *entry)
>> +{
>> + return tst_syscall(__NR_set_thread_area, entry);
>> +}
>> +
>> +static inline int get_thread_area(const struct user_desc *entry)
>> +{
>> + return tst_syscall(__NR_get_thread_area, entry);
>> +}
>> +
>> +
nit: there's one more space :-)
>> #endif /* LAPI_LDT_H__ */
>> diff --git a/testcases/kernel/syscalls/set_thread_area/.gitignore b/testcases/kernel/syscalls/set_thread_area/.gitignore
>> index 547eb86e3c4ee86e2343067867c3bb027e0ee85a..b80ce741812c33fd9af47ecba8561ef385212e97 100644
>> --- a/testcases/kernel/syscalls/set_thread_area/.gitignore
>> +++ b/testcases/kernel/syscalls/set_thread_area/.gitignore
>> @@ -1 +1,2 @@
>> -/set_thread_area01
>> +set_thread_area01
>> +set_thread_area02
We are missing the reference in runtest/syscalls file for set_thread_area02
>> 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..4e190db9afb93a56a32593d2908ef3a389ccf830 100644
>> --- a/testcases/kernel/syscalls/set_thread_area/set_thread_area01.c
>> +++ b/testcases/kernel/syscalls/set_thread_area/set_thread_area01.c
>> @@ -1,111 +1,61 @@
>> -/*************************************************************************
>> +// 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
>> + * Copyright (c) 2025 SUSE LLC Ricardo B. Marlière <rbm@suse.com>
>> + */
>> +
>> +/*\
>> + * Basic test of i386 thread-local storage for set_thread_area and
>> + * get_thread_area syscalls. It verifies a simple write and read of an entry
>> + * works.
>> *
>> - * 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
>> + * [Algorithm]
>> *
>> - ************************************************************************/
>> + * - 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.
>> + * - Call get_thread_area to read the new entry.
>> + * - Use the new entry_number in another pointer and call get_thread_area.
>> + * - Make sure they have the same data.
>> + */
>>
>> -#include "set_thread_area.h"
>> +#include "tst_test.h"
>>
>> -char *TCID = "set_thread_area_01";
>> -int TST_TOTAL = 6;
>> +#include "lapi/ldt.h"
>>
>> -#if defined(HAVE_ASM_LDT_H) && defined(HAVE_STRUCT_USER_DESC)
>> +static struct user_desc *entry1;
>> +static struct user_desc *entry2;
>>
>> -static void cleanup(void)
>> +void run(void)
In general, all functions defined inside the test should be defined static.
>> {
>> + TST_EXP_PASS_SILENT(set_thread_area(entry1));
>> + TST_EXP_PASS_SILENT(get_thread_area(entry1));
> Perhaps I should also add the SAFE_ macros and make use of them here?
>
>> +
>> + entry2->entry_number = entry1->entry_number;
>> + TST_EXP_PASS_SILENT(get_thread_area(entry2));
>> +
>> + TST_EXP_PASS(memcmp(entry1, entry2, sizeof(struct user_desc)));
>> }
>>
>> -static void setup(void)
>> +void setup(void)
>> {
>> - TEST_PAUSE;
>> + /* When set_thread_area() is passed an entry_number of -1, it searches
>> + * for a free TLS entry. If set_thread_area() finds a free TLS entry,
>> + * the value of u_info->entry_number is set upon return to show which
>> + * entry was changed.
>> + */
>> + entry1->entry_number = -1;
>> }
>>
>> -struct test {
>> - int syscall;
>> - const char *const syscall_name;
>> - thread_area_s *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[] = {
>> - {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},
>> - {VALUE_AND_STRING(__NR_get_thread_area), &u_info2, -1, EINVAL},
>> - {VALUE_AND_STRING(__NR_set_thread_area), (void *)-9, -1, EFAULT},
>> - {VALUE_AND_STRING(__NR_get_thread_area), (void *)-9, -1, EFAULT},
>> +static struct tst_test test = {
>> + .setup = setup,
>> + .test_all = run,
>> + .supported_archs = (const char *const[]){ "x86", NULL },
>> + .bufs =
>> + (struct tst_buffers[]){
This is a strange formatting. I'm wondering why "make check" is not
catching it. But eventually, we should use the syntax:
.bufs = (struct tst_buffers[]) {
^ with space here
>> + { &entry1, .size = sizeof(struct user_desc) },
>> + { &entry2, .size = sizeof(struct user_desc) },
>> + {},
>> + },
>> };
The rest of the test looks good.
>> -
>> -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;
>> - }
>> -
>> - 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;
>> - }
>> -
>> - tst_resm(TPASS, "%s returned %li errno %i (%s)",
>> - tests[i].syscall_name, TEST_RETURN,
>> - TEST_ERRNO, strerror(TEST_ERRNO));
>> - }
>> - }
>> -
>> - cleanup();
>> - tst_exit();
>> -}
>> -#else
>> -int main(void)
>> -{
>> - tst_brkm(TCONF, NULL,
>> - "set_thread_area isn't available for this architecture");
>> -}
>> -#endif
>> diff --git a/testcases/kernel/syscalls/set_thread_area/set_thread_area02.c b/testcases/kernel/syscalls/set_thread_area/set_thread_area02.c
>> new file mode 100644
>> index 0000000000000000000000000000000000000000..e5ace82353bacc40bd462a5be2a598d456d31e3d
>> --- /dev/null
>> +++ b/testcases/kernel/syscalls/set_thread_area/set_thread_area02.c
>> @@ -0,0 +1,62 @@
>> +// 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
>> + * Copyright (c) 2025 SUSE LLC Ricardo B. Marlière <rbm@suse.com>
>> + */
>> +
>> +/*\
>> + * Tests set_thread_area and get_thread_area syscalls for their expected errors.
We need to add the dot list of the expected errors with description.
Remember that this description will be visible in the tests catalog and
it needs to be well formatted.
>> + */
>> +
>> +#include "tst_test.h"
>> +
>> +#include "lapi/ldt.h"
>> +
>> +static struct user_desc *entry;
>> +
>> +static struct tvariant {
>> + const char *desc;
>> +} tvariants[] = {
>> + { TST_TO_STR(SET_THREAD_AREA) },
>> + { TST_TO_STR(GET_THREAD_AREA) },
>> +};
This is not needed, we can use the right message in the setup (see below)
>> +
>> +static struct tcase {
>> + struct user_desc *entry;
>> + int exp_errno;
>> +} tcases[] = {
>> + { NULL, EINVAL },
>> + { (void *)-9, EFAULT },
>> +};
>> +void run(unsigned int i)
>> +{
>> + struct tcase tc = tcases[i];
>> +
>> + if (tst_variant)
>> + TST_EXP_FAIL(get_thread_area(tc.entry), tc.exp_errno);
>> + else
>> + TST_EXP_FAIL(set_thread_area(tc.entry), tc.exp_errno);
>> +}
>> +
We are also missing static definition for the functions inside the test.
>> +void setup(void)
>> +{
>> + /* This makes *entry invalid */
>> + entry->entry_number = -2;
>> + tcases[0].entry = entry;
>> + tst_res(TINFO, "Testing %s", tvariants[tst_variant].desc);
We can use the following shorter version:
tst_res(TINFO, "Testing %s", tst_variant ? "get_thread_area":
"set_thread_area");
But I have the feeling we can just skip this message and keep the
TST_EXP_FAIL() message coming from stdout, since it's self explanatory.
>> +}
>> +
>> +static struct tst_test test = {
>> + .setup = setup,
>> + .tcnt = ARRAY_SIZE(tcases),
>> + .test = run,
>> + .test_variants = ARRAY_SIZE(tvariants),
>> + .supported_archs = (const char *const[]){ "x86", NULL },
>> + .bufs =
>> + (struct tst_buffers[]){
Also here there's a strange fomartting.
>> + { &entry, .size = sizeof(struct user_desc) },
>> + {},
>> + },
>> +};
>>
>> ---
>> base-commit: 898cc14ad412fb521867b43fed5c4e067b76f809
>> change-id: 20250403-conversions-set_thread_area-07a90e0cd449
>> prerequisite-message-id: <20250402-conversions-modify_ldt-v6-0-2e4b0e27870e@suse.com>
>> prerequisite-patch-id: 490a3e6bc4004db5234224b6fd6d4bf5030b219d
>> prerequisite-patch-id: 962bb815444eb2de9756dd2659e097567b6d6fe8
>> prerequisite-patch-id: a8357fb870a8d7278e206b4fc65f1d9450fee802
>>
>> Best regards,
>
Kind regards,
Andrea Cervesato
More information about the ltp
mailing list