| Message ID | 20241212-convert_mmap01-v6-1-d186b68c3f09@suse.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [v6] mmap01: Convert to new API | expand |
Hi Ricardo, generally LGTM, with minor comments below (only relevant is SAFE_MSYNC()). Reviewed-by: Petr Vorel <pvorel@suse.cz> ... > +++ b/testcases/kernel/syscalls/mmap/mmap01.c > @@ -1,194 +1,145 @@ > -/* > - * 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. > +/*\ > + * [Description] > * > - * 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. > + * 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. nit: Slightly hard to read, but I'm not able to suggest any improvement. > * > - * HISTORY > - * 07/2001 Ported by Wayne Boyer > + * 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. nit: FYI this new line has no effect for docparse formatting (html/pdf formatting). If you want to have separate paragraph, it needs to be extra blank space. I would just continue the sentence in previous paragaraph. > + * The changes beyond the end of file should not get written to the file. > */ > +#include "tst_test.h" > + > +#define TEMPFILE "mmapfile" > +#define STRING "hello world\n" ... static void check_file(void) { ... for (i = 0; i < buf_len; i++) if (buf[i] == 'X' || buf[i] == 'Y' || buf[i] == 'Z') break; if (i == buf_len) tst_res(TPASS, "Functionality of mmap() successful"); very nit: IMHO that mmap() works is detectable from TPASS. I like when TPASS describe what was the evaluation, maybe something like "pattern found in the file" ? else tst_res(TFAIL, "Specified pattern found in file"); SAFE_CLOSE(fildes); } > - page_sz = getpagesize(); > +static void run(void) > +{ > + pid_t pid; > + > + set_file(); > + > + addr = SAFE_MMAP(NULL, page_sz, PROT_READ | PROT_WRITE, > + MAP_FILE | MAP_SHARED, fildes, 0); > + > + /* > + * 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_brk(TFAIL, "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_res(TFAIL | TERRNO, "failed to synchronize mapped file"); > + return; I would use here SAFE_MSYNC(). > + } > - /* 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)"); > + /* > + * Now, search for the pattern 'XYZ' in the temporary file. > + * The pattern should not be found and the return value should be 1. > + */ > + pid = SAFE_FORK(); > + if (!pid) { nit: pid is not needed, how about use SAFE_FORK() directly? if (!SAFE_FORK()) { > + check_file(); > + SAFE_MUNMAP(addr, page_sz); > + exit(0); > } ... > +static void setup(void) > +{ > + page_sz = getpagesize(); > + > + /* Allocate and initialize dummy string of system page size bytes */ > + dummy = SAFE_CALLOC(page_sz, sizeof(char)); nit: sizeof(char) is always guaranteed to be 1, I would use: dummy = SAFE_CALLOC(page_sz, 1); The rest LGTM. Reviewed-by: Petr Vorel <pvorel@suse.cz> Kind regards, Petr
Hi Petr! On 16 Dec 24 14:01, Petr Vorel wrote: > Hi Ricardo, > > generally LGTM, with minor comments below (only relevant is SAFE_MSYNC()). > > Reviewed-by: Petr Vorel <pvorel@suse.cz> > > ... > > +++ b/testcases/kernel/syscalls/mmap/mmap01.c > > @@ -1,194 +1,145 @@ > > > -/* > > - * 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. > > +/*\ > > + * [Description] > > * > > - * 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. > > + * 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. > nit: Slightly hard to read, but I'm not able to suggest any improvement. > > * > > - * HISTORY > > - * 07/2001 Ported by Wayne Boyer > > + * 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. > nit: FYI this new line has no effect for docparse formatting (html/pdf formatting). > If you want to have separate paragraph, it needs to be extra blank space. > I would just continue the sentence in previous paragaraph. > > + * The changes beyond the end of file should not get written to the file. > > */ > > > +#include "tst_test.h" > > + > > +#define TEMPFILE "mmapfile" > > +#define STRING "hello world\n" > > ... > > static void check_file(void) > { > ... > for (i = 0; i < buf_len; i++) > if (buf[i] == 'X' || buf[i] == 'Y' || buf[i] == 'Z') > break; > > if (i == buf_len) > tst_res(TPASS, "Functionality of mmap() successful"); > very nit: IMHO that mmap() works is detectable from TPASS. I like when TPASS > describe what was the evaluation, maybe something like "pattern found in the file" ? > else > tst_res(TFAIL, "Specified pattern found in file"); > SAFE_CLOSE(fildes); > } > > > - page_sz = getpagesize(); > > +static void run(void) > > +{ > > + pid_t pid; > > + > > + set_file(); > > + > > + addr = SAFE_MMAP(NULL, page_sz, PROT_READ | PROT_WRITE, > > + MAP_FILE | MAP_SHARED, fildes, 0); > > + > > + /* > > + * 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_brk(TFAIL, "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_res(TFAIL | TERRNO, "failed to synchronize mapped file"); > > + return; > I would use here SAFE_MSYNC(). Indeed! From now on I'll always check for SAFE_* :) Thanks for the review, I'll push a new revision addressing your points. - Ricardo. > > + } > > > - /* 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)"); > > + /* > > + * Now, search for the pattern 'XYZ' in the temporary file. > > + * The pattern should not be found and the return value should be 1. > > + */ > > + pid = SAFE_FORK(); > > + if (!pid) { > nit: pid is not needed, how about use SAFE_FORK() directly? > if (!SAFE_FORK()) { > > > + check_file(); > > + SAFE_MUNMAP(addr, page_sz); > > + exit(0); > > } > ... > > > +static void setup(void) > > +{ > > + page_sz = getpagesize(); > > + > > + /* Allocate and initialize dummy string of system page size bytes */ > > + dummy = SAFE_CALLOC(page_sz, sizeof(char)); > nit: sizeof(char) is always guaranteed to be 1, I would use: > > dummy = SAFE_CALLOC(page_sz, 1); > > The rest LGTM. > > Reviewed-by: Petr Vorel <pvorel@suse.cz> > > Kind regards, > Petr
diff --git a/testcases/kernel/syscalls/mmap/mmap01.c b/testcases/kernel/syscalls/mmap/mmap01.c index 99266b57ffc16e6c3a0e31a2c87d0f8106f429e5..0ada71ee5d066be44fa52d8c21d8e21685d9dc44 100644 --- a/testcases/kernel/syscalls/mmap/mmap01.c +++ b/testcases/kernel/syscalls/mmap/mmap01.c @@ -1,194 +1,145 @@ +// SPDX-License-Identifier: GPL-2.0-or-later /* * Copyright (c) International Business Machines Corp., 2001 + * Copyright (c) 2024 Ricardo B. Marliere <rbm@suse.com> * - * 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 + * HISTORY + * 07/2001 Ported by Wayne Boyer */ -/* - * 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. +/*\ + * [Description] * - * 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. + * 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. * - * HISTORY - * 07/2001 Ported by Wayne Boyer + * 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. */ -#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" - -char *TCID = "mmap01"; -int TST_TOTAL = 1; + +#include "tst_test.h" + +#define TEMPFILE "mmapfile" +#define STRING "hello world\n" static char *addr; static char *dummy; 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); - -int main(int ac, char **av) +static void check_file(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"); - } - } + int i, fildes, buf_len = sizeof(STRING) + 3; + char buf[buf_len]; - cleanup(); - tst_exit(); -} + fildes = SAFE_OPEN(TEMPFILE, O_RDONLY); + SAFE_READ(0, fildes, buf, sizeof(buf)); -static void setup(void) -{ - struct stat stat_buf; - char Path_name[PATH_MAX]; - char write_buf[] = "hello world\n"; + for (i = 0; i < buf_len; i++) + if (buf[i] == 'X' || buf[i] == 'Y' || buf[i] == 'Z') + break; - tst_sig(FORK, DEF_HANDLER, cleanup); + if (i == buf_len) + tst_res(TPASS, "Functionality of mmap() successful"); + else + tst_res(TFAIL, "Specified pattern found in file"); - TEST_PAUSE; + SAFE_CLOSE(fildes); +} - tst_tmpdir(); +static void set_file(void) +{ + char *write_buf = STRING; + struct stat stat_buf; - /* 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"); + /* Reset file */ + if (fildes > 0) { + SAFE_CLOSE(fildes); + SAFE_UNLINK(TEMPFILE); } - /* 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); - } + /* Create a temporary file used for mapping */ + fildes = SAFE_OPEN(TEMPFILE, O_RDWR | O_CREAT, 0666); /* 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_WRITE(SAFE_WRITE_ALL, fildes, write_buf, strlen(write_buf)); /* Get the size of temporary file */ - if (stat(TEMPFILE, &stat_buf) < 0) { - tst_brkm(TFAIL | TERRNO, cleanup, "stat of %s failed", - TEMPFILE); - } + SAFE_STAT(TEMPFILE, &stat_buf); file_sz = stat_buf.st_size; +} - page_sz = getpagesize(); +static void run(void) +{ + pid_t pid; + + set_file(); + + addr = SAFE_MMAP(NULL, page_sz, PROT_READ | PROT_WRITE, + MAP_FILE | MAP_SHARED, fildes, 0); + + /* + * 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_brk(TFAIL, "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_res(TFAIL | TERRNO, "failed to synchronize mapped file"); + return; + } - /* 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)"); + /* + * Now, search for the pattern 'XYZ' in the temporary file. + * The pattern should not be found and the return value should be 1. + */ + pid = SAFE_FORK(); + if (!pid) { + check_file(); + SAFE_MUNMAP(addr, page_sz); + exit(0); } - /* Create the command which will be executed in the test */ - sprintf(cmd_buffer, "grep XYZ %s/%s > /dev/null", Path_name, TEMPFILE); + SAFE_MUNMAP(addr, page_sz); } static void cleanup(void) { - close(fildes); - free(dummy); - tst_rmdir(); + if (dummy) + free(dummy); + if (fildes > 0) + SAFE_CLOSE(fildes); } + +static void setup(void) +{ + page_sz = getpagesize(); + + /* Allocate and initialize dummy string of system page size bytes */ + dummy = SAFE_CALLOC(page_sz, sizeof(char)); +} + +static struct tst_test test = { + .setup = setup, + .cleanup = cleanup, + .needs_tmpdir = 1, + .test_all = run, + .forks_child = 1, +};