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

[Andrea Cervesato via ltp <ltp@lists.linux.it>]

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

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp