Re: [LTP] [PATCH 1/3] mmap001: Convert to new API

Andrea Cervesato via ltp
2025-02-10T09:05:24Z

Hi!

A few comments below about the code.

On 1/29/25 18:19, Ricardo B. Marliere wrote:
> Hi Andrea,
>
> thanks for reviewing!
>
> On Wed Jan 15, 2025 at 10:14 AM -03, Andrea Cervesato wrote:
>> Hi!
>>
>> On 1/14/25 23:26, Ricardo B. Marliere via ltp wrote:
>>> From: Ricardo B. Marliere <rbm@suse.com>
>>>
>>> Signed-off-by: Ricardo B. Marliere <rbm@suse.com>
>>> ---
>>> testcases/kernel/syscalls/mmap/mmap001.c | 206 ++++++++-----------------------
>>> 1 file changed, 49 insertions(+), 157 deletions(-)
>>>
>>> diff --git a/testcases/kernel/syscalls/mmap/mmap001.c b/testcases/kernel/syscalls/mmap/mmap001.c
>>> index dabb7d1e4998b1097e179abe23555926f5841117..bc9b4155e8b53f942ef694fdf3187c0e544a97cd 100644
>>> --- a/testcases/kernel/syscalls/mmap/mmap001.c
>>> +++ b/testcases/kernel/syscalls/mmap/mmap001.c
>>> @@ -1,183 +1,75 @@
>>> +// SPDX-License-Identifier: GPL-2.0-or-later
>>> /*
>>> * Copyright (C) 2000 Juan Quintela <quintela@fi.udc.es>
>>> * Aaron Laffin <alaffin@sgi.com>
>>> + * Copyright (c) 2025 Linux Test Project
>>> + */
>>> +
>>> +/*\
>>> + * [Description]
>>> *
>>> - * 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., 59 Temple Place - Suite 330, Boston, MA 02111-1307, USA.
>>> - *
>>> - * mmap001.c - Tests mmapping a big file and writing it once
>>> + * Tests mmapping a big file and writing it once
>> Description is a bit short and it needs a bit more information. For
>> example, what we expect to see and what could happen during test (SEGV
>> for example).
> Ack.
>
>>> - if (TEST_RETURN == -1) {
>>> - tst_resm(TFAIL | TTERRNO,
>>> - "munmapping %s failed", filename);
>>> - } else {
>>> - tst_resm(TPASS, "munmapping %s successful", filename);
>>> - }
>>> + /*
>>> + * Seems that if the map area was bad, we'd get SEGV,
>>> + * hence we can indicate a PASS.
>>> + */
>> If this is true, we need to find a way to test that specific scenario.
>> This can e done by forking a process where test is running and to check
>> if SIGSEGV killed it after calling SAFE_WAITPID()
> Good idea, I kept the original comment and TPASS string but it could
> surely be expanded. What do you think of the following? I'll be sending
> as v2 when the series receive some more reviewing.
>
> diff --git a/testcases/kernel/syscalls/mmap/mmap001.c b/testcases/kernel/syscalls/mmap/mmap001.c
> index bc9b4155e8b5..10ce7a48e7c2 100644
> --- a/testcases/kernel/syscalls/mmap/mmap001.c
> +++ b/testcases/kernel/syscalls/mmap/mmap001.c
> @@ -8,7 +8,8 @@
> /*\
> * [Description]
> *
> - * Tests mmapping a big file and writing it once
> + * This test will map a big file to memory and write to it once,
> + * making sure nothing bad happened in between such as a SIGSEGV.
> */
>
> #include "tst_test.h"
> @@ -25,6 +26,8 @@ static void setup(void)
>
> static void run(void)
> {
> + pid_t pid;
> + int status;
> char *array;
> unsigned int i;
> unsigned int pages, memsize;
> @@ -40,20 +43,23 @@ static void run(void)
> SAFE_LSEEK(fd, memsize, SEEK_SET);
> SAFE_WRITE(SAFE_WRITE_ALL, fd, "\0", 1);
>
> - array = SAFE_MMAP(NULL, memsize, PROT_WRITE, MAP_SHARED, fd, 0);
> -
> - tst_res(TINFO, "touching mmaped memory");
> - for (i = 0; i < memsize; i++)
> - array[i] = (char)i;
> -
> - /*
> - * Seems that if the map area was bad, we'd get SEGV,
> - * hence we can indicate a PASS.
> - */
> - tst_res(TPASS, "we're still here, mmaped area must be good");
> -
> - SAFE_MSYNC(array, memsize, MS_SYNC);
> - SAFE_MUNMAP(array, memsize);
> + pid = SAFE_FORK();
> + if (pid == 0) {
> + array = SAFE_MMAP(NULL, memsize, PROT_WRITE, MAP_SHARED, fd, 0);
> + tst_res(TINFO, "touching mmapped memory");
> + for (i = 0; i < memsize; i++)
> + array[i] = (char)i;
> + SAFE_MSYNC(array, memsize, MS_SYNC);
> + SAFE_MUNMAP(array, memsize);
> + exit(0);
> + } else {
No need for else here. Remember to call "make check" on the folder to
verify code correctness. "make check-mmap001" in this case.
Also, I was thinking that we don't have a mechanism to know if file is
updated or not, so please take a look at my patch on mmap001 (mmap21).
I'm gonna send a v2 so you can take a look at the final idea.
> + SAFE_WAITPID(pid, &status, 0);
> + if (WIFSIGNALED(status) && WEXITSTATUS(status) == SIGSEGV)
> + tst_res(TFAIL, "test was killed by SIGSEGV");
> + else
> + tst_res(TPASS,
> + "memory was mapped and written to successfully");
> + }
> }
>
> static void cleanup(void)
> @@ -66,6 +72,7 @@ static struct tst_test test = {
> .setup = setup,
> .test_all = run,
> .cleanup = cleanup,
> + .forks_child = 1,
> .options =
> (struct tst_option[]){
> { "m:", &m_copt,
>
>
>
>> Kind regards,
>> Andrea
> Thank you,
> - Ricardo.
>
>
Andrea