[LTP] [PATCH v4] syscalls/mmap01: Rewrite the test using new LTP API
Avinesh Kumar
akumar@suse.de
Wed Mar 20 11:56:34 CET 2024
Hi Cyril, Petr,
Thank you for the review.
On Wednesday, March 6, 2024 3:55:53 PM CET Cyril Hrubis wrote:
> On Tue, Jan 30, 2024 at 01:23:57PM +0100, Avinesh Kumar wrote:
> > - use SAFE_MSYNC() macro
> > - fixed the test for iterations > 1
> > - enable test for all filesystems
> >
> > 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 index 99266b57f..e0b36915c
> > 100644
> > --- a/testcases/kernel/syscalls/mmap/mmap01.c
> > +++ b/testcases/kernel/syscalls/mmap/mmap01.c
> > @@ -1,194 +1,93 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> >
> > /*
> >
> > * Copyright (c) International Business Machines Corp., 2001
> >
> > - *
> > - * 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 + * 07/2001 Ported by Wayne Boyer
> > + * Copyright (c) 2023 SUSE LLC Avinesh Kumar <avinesh.kumar@suse.com>
> >
> > */
> >
> > -/*
> > - * Test Description:
> > - * Verify that, mmap() succeeds when used to map a file where size of
> > the
> > - * file is not a multiple of the page size, the memory area beyond the
> > end - * of the file to the end of the page is accessible. Also, verify
> > that - * this area is all zeroed and the modifications done to this area
> > are - * not written to the file.
> > - *
> > - * Expected Result:
> > - * mmap() should succeed returning the address of the mapped region.
> > - * The memory area beyond the end of file to the end of page should be
> > - * filled with zero.
> > - * The changes beyond the end of file should not get written to the
> > file.
> > +/*\
> > + * [Description]
> >
> > *
> >
> > - * HISTORY
> > - * 07/2001 Ported by Wayne Boyer
> > + * Verify that, mmap() succeeds when used to map a file where size of the
> > + * file is not a multiple of the page size, the memory area beyond the
> > end
> > + * of the file to the end of the page is accessible. Also, verify that
> > + * this area is all zeroed and the modifications done to this area are
> > + * not written to the file.
> >
> > */
> >
> > -#include <stdio.h>
> > -#include <stdlib.h>
> > -#include <sys/types.h>
> > -#include <errno.h>
> > -#include <unistd.h>
> > -#include <fcntl.h>
> > -#include <string.h>
> > -#include <signal.h>
> > -#include <stdint.h>
> > -#include <sys/stat.h>
> > -#include <sys/mman.h>
> > -#include <sys/shm.h>
> >
> > -#include "test.h"
> > -
> > -#define TEMPFILE "mmapfile"
> > +#include <stdlib.h>
> > +#include "tst_test.h"
> >
> > -char *TCID = "mmap01";
> > -int TST_TOTAL = 1;
> > +#define MNT_POINT "mntpoint"
> > +#define TEMPFILE MNT_POINT"/mmapfile"
> >
> > -static char *addr;
> > -static char *dummy;
> > +static int fd;
> >
> > static size_t page_sz;
> > static size_t file_sz;
> >
> > -static int fildes;
> > -static char cmd_buffer[BUFSIZ];
> > -
> > -static void setup(void);
> > -static void cleanup(void);
> > +static char *addr;
> > +static char *dummy;
> > +static struct stat stat_buf;
> > +static const char write_buf[] = "HelloWorld!";
> >
> > -int main(int ac, char **av)
> > +static void setup(void)
> >
> > {
> >
> > - int lc;
> > -
> > - tst_parse_opts(ac, av, NULL, NULL);
> > -
> > - setup();
> > -
> > - for (lc = 0; TEST_LOOPING(lc); lc++) {
> > -
> > - tst_count = 0;
> > -
> > - /*
> > - * Call mmap to map the temporary file beyond EOF
> > - * with write access.
> > - */
> > - errno = 0;
> > - addr = mmap(NULL, page_sz, PROT_READ | PROT_WRITE,
> > - MAP_FILE | MAP_SHARED, fildes, 0);
> > -
> > - /* Check for the return value of mmap() */
> > - if (addr == MAP_FAILED) {
> > - tst_resm(TFAIL | TERRNO, "mmap of %s failed", TEMPFILE);
> > - continue;
> > - }
> > -
> > - /*
> > - * Check if mapped memory area beyond EOF are
> > - * zeros and changes beyond EOF are not written
> > - * to file.
> > - */
> > - if (memcmp(&addr[file_sz], dummy, page_sz - file_sz)) {
> > - tst_brkm(TFAIL, cleanup,
> > - "mapped memory area contains invalid "
> > - "data");
> > - }
> > -
> > - /*
> > - * Initialize memory beyond file size
> > - */
> > - addr[file_sz] = 'X';
> > - addr[file_sz + 1] = 'Y';
> > - addr[file_sz + 2] = 'Z';
> > -
> > - /*
> > - * Synchronize the mapped memory region
> > - * with the file.
> > - */
> > - if (msync(addr, page_sz, MS_SYNC) != 0) {
> > - tst_brkm(TFAIL | TERRNO, cleanup,
> > - "failed to synchronize mapped file");
> > - }
> > -
> > - /*
> > - * Now, Search for the pattern 'XYZ' in the
> > - * temporary file. The pattern should not be
> > - * found and the return value should be 1.
> > - */
> > - if (system(cmd_buffer) != 0) {
> > - tst_resm(TPASS,
> > - "Functionality of mmap() successful");
> > - } else {
> > - tst_resm(TFAIL,
> > - "Specified pattern found in file");
> > - }
> > -
> > - /* Clean up things in case we are looping */
> > - /* Unmap the mapped memory */
> > - if (munmap(addr, page_sz) != 0) {
> > - tst_brkm(TFAIL | TERRNO, NULL, "munmap failed");
> > - }
> > - }
> > + 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);
> >
> > - /* Get the path of temporary file to be created */
> > - if (getcwd(Path_name, sizeof(Path_name)) == NULL) {
> > - tst_brkm(TFAIL | TERRNO, cleanup,
> > - "getcwd failed to get current working directory");
> > + if (memcmp(&addr[file_sz], dummy, page_sz - file_sz) != 0) {
> > + tst_res(TFAIL, "mapped memory area contains invalid data");
> > + goto unmap;
> >
> > }
> >
> > - /* Creat a temporary file used for mapping */
> > - if ((fildes = open(TEMPFILE, O_RDWR | O_CREAT, 0666)) < 0) {
> > - tst_brkm(TFAIL, cleanup, "opening %s failed", TEMPFILE);
> > - }
> > + addr[file_sz] = 'X';
> > + addr[file_sz + 1] = 'Y';
> > + addr[file_sz + 2] = 'Z';
> >
> > - /* Write some data into temporary file */
> > - if (write(fildes, write_buf, strlen(write_buf)) !=
> > (long)strlen(write_buf)) { - tst_brkm(TFAIL, cleanup, "writing to %s",
> > TEMPFILE);
> > - }
> > + SAFE_MSYNC(addr, page_sz, MS_SYNC);
> >
> > - /* Get the size of temporary file */
> > - if (stat(TEMPFILE, &stat_buf) < 0) {
> > - tst_brkm(TFAIL | TERRNO, cleanup, "stat of %s failed",
> > - TEMPFILE);
> > - }
> > - file_sz = stat_buf.st_size;
> > + SAFE_FILE_SCANF(TEMPFILE, "%s", buf);
>
> Hmm, why do we SAFE_LSEEK() the fd if we are not using it for reading?
I guess I can remove the SAFE_LSEEK() in setup(), as we want to read the
complete file contents without knowing it's size, hence SAFE_FILE_SCANF().
Please correct me if this is not the right approach.
>
> This could be just simple SAFE_READ() instead.
>
> > - page_sz = getpagesize();
> > + if (strcmp(write_buf, buf))
> > + tst_res(TFAIL, "File data has changed");
> > + else
> > + tst_res(TPASS, "mmap() functionality successful");
>
> ^
> "Data after file end were not written out"
>
> It's kind of pointless to print message that just means "success".
>
> > - /* Allocate and initialize dummy string of system page size bytes */
> > - if ((dummy = calloc(page_sz, sizeof(char))) == NULL) {
> > - tst_brkm(TFAIL, cleanup, "calloc failed (dummy)");
> > - }
> > + SAFE_LSEEK(fd, 0, SEEK_SET);
> > + memset(&addr[file_sz], 0, 3);
>
> I was wondering why this is needed, seems like for tmpfs we will read
> back the data after the end of the file on a subsequent runs of the
> test, i.e. with -i 2.
>
> I wonder if that is expected or not, it's a bit strange that we can
> expand the file size that way.
>
> And it seems to happen for FUSE as well, that actually does sound like a
> bug.
Thanks for pointing this out, I was overlooking this issue. I verified that we
read back the data written past eof in further iteration of the test only in
tmpfs and fuse.ntfs. How would you suggest to confirm if this is indeed a bug
with these filesystems.
Regards,
Avinesh
More information about the ltp
mailing list