Re: [LTP] [PATCH v4] syscalls/mmap01: Rewrite the test using new LTP API

[Petr Vorel <pvorel@suse.cz>]

HI Avinesh,

> - use SAFE_MSYNC() macro
> - fixed the test for iterations > 1
> - enable test for all filesystems
+1

> Signed-off-by: Avinesh Kumar <akumar@suse.de>
> ---

> Changes v3->v4:
> * Changed the logic to verify that mapped file has not been changed.
> * Enabled all filesystems.


>  testcases/kernel/syscalls/mmap/mmap01.c | 223 +++++++-----------------
>  1 file changed, 61 insertions(+), 162 deletions(-)

> diff --git a/testcases/kernel/syscalls/mmap/mmap01.c b/testcases/kernel/syscalls/mmap/mmap01.c
...
> +static char *addr;
> +static char *dummy;
> +static struct stat stat_buf;
nit: struct stat stat_buf is used just in the setup, it could defined just
there. It can be changed before merge.

> +static const char write_buf[] = "HelloWorld!";

> -int main(int ac, char **av)
> +static void setup(void)
>  {
...
> +	fd = SAFE_OPEN(TEMPFILE, O_RDWR | O_CREAT, 0666);

> -	cleanup();
> -	tst_exit();
> -}
> +	SAFE_WRITE(SAFE_WRITE_ALL, fd, write_buf, strlen(write_buf));
> +	SAFE_LSEEK(fd, 0, SEEK_SET);
> +	SAFE_STAT(TEMPFILE, &stat_buf);

> -static void setup(void)
> -{
> -	struct stat stat_buf;
> -	char Path_name[PATH_MAX];
> -	char write_buf[] = "hello world\n";
> +	file_sz = stat_buf.st_size;
> +	page_sz = getpagesize();

> -	tst_sig(FORK, DEF_HANDLER, cleanup);
> +	dummy = SAFE_MALLOC(page_sz);
> +	memset(dummy, 0, page_sz);
> +}

> -	TEST_PAUSE;
> +static void run(void)
> +{
> +	char buf[20];

> -	tst_tmpdir();
> +	addr = SAFE_MMAP(NULL, page_sz, PROT_READ | PROT_WRITE, MAP_FILE | MAP_SHARED, fd, 0);
...
> +	addr[file_sz] = 'X';
> +	addr[file_sz + 1] = 'Y';
> +	addr[file_sz + 2] = 'Z';
...
> +	SAFE_MSYNC(addr, page_sz, MS_SYNC);
...
> +	SAFE_FILE_SCANF(TEMPFILE, "%s", buf);

> -	page_sz = getpagesize();
> +	if (strcmp(write_buf, buf))
> +		tst_res(TFAIL, "File data has changed");
> +	else
> +		tst_res(TPASS, "mmap() functionality successful");
nit: I would prefer something more descriptive (what mmap() actually did, e.g.
"mapped file has not been changed"), but I guess it's ok to keep it as is.

The rest LGTM.

Reviewed-by: Petr Vorel <pvorel@suse.cz>

Kind regards,
Petr

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