[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <b2e62143-fa68-4cd1-bf6c-67f0ad49c670@linuxfoundation.org>
Date: Mon, 2 Jun 2025 16:21:57 -0600
From: Shuah Khan <skhan@...uxfoundation.org>
To: "Dmitry V. Levin" <ldv@...ace.io>
Cc: "Maciej W. Rozycki" <macro@...am.me.uk>, Shuah Khan <shuah@...nel.org>,
Oleg Nesterov <oleg@...hat.com>, strace-devel@...ts.strace.io,
linux-kselftest@...r.kernel.org, linux-mips@...r.kernel.org,
linux-kernel@...r.kernel.org, Shuah Khan <skhan@...uxfoundation.org>
Subject: Re: [PATCH v2] selftests/ptrace/get_syscall_info: fix for MIPS n32
On 6/2/25 05:59, Dmitry V. Levin wrote:
> On Sat, Mar 29, 2025 at 02:02:28PM +0000, Maciej W. Rozycki wrote:
>> On Sat, 29 Mar 2025, Dmitry V. Levin wrote:
>>
>>>>> +#if defined(_MIPS_SIM) && _MIPS_SIM == _MIPS_SIM_NABI32
>>>>> +/*
>>>>> + * MIPS N32 is the only architecture where __kernel_ulong_t
>>>>> + * does not match the bitness of syscall arguments.
>>>>> + */
>>>>> +typedef unsigned long long kernel_ulong_t;
>>>>> +#else
>>>>> +typedef __kernel_ulong_t kernel_ulong_t;
>>>>> +#endif
>>>>> +
>>>>
>>>> What's the reason for adding these typedefs? checkpatch should
>>>> have warned you about adding new typedefs.
>>>>
>>>> Also this introduces kernel_ulong_t in user-space test code.
>>>> Something to avoid.
>>>
>>> There has to be a new type for this test, and the natural way to do this
>>> is to use typedef. The alternative would be to #define kernel_ulong_t
>>> which is ugly. By the way, there are quite a few typedefs in selftests,
>>> and there seems to be given no rationale why adding new types in selftests
>>> is a bad idea.
>>
>> FWIW I agree, and I fail to see a reason why this would be a problem in a
>> standalone test program where the typedef does not propagate anywhere.
>>
>> The only potential issue I can identify would be a namespace clash, so
>> perhaps the new type could have a name prefix specific to the test, but it
>> doesn't appear to me a widespread practice across our selftests and then
>> `kernel_' ought to be pretty safe against ISO C or POSIX, so perhaps let's
>> leave the things alone?
>
> Another similar test I authored (selftests/ptrace/set_syscall_info) has
> been merged, so there are two similar tests in the tree now, but only
> one of them is permitted to use this approach, creating inconsistency.
>
> Taking all of the above into consideration, please approve this fix.
>
>
Acked-by: Shuah Khan <skhan@...uxfoundation.org>
thanks,
-- Shuah
Powered by blists - more mailing lists