Re: [LTP] [PATCH v6] mmap01: Convert to new API

[Petr Vorel <pvorel@suse.cz>]

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

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