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

[Ricardo B. Marliere]

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