Re: [LTP] [PATCH 3/4] syscalls/mmap02: Rewrite the test using new LTP API

[Cyril Hrubis <chrubis@suse.cz>]

Hi!
>  static void setup(void)
>  {
> -	char *tst_buff;
> -
> -	tst_sig(FORK, DEF_HANDLER, cleanup);
> -
> -	TEST_PAUSE;
> +	char *buf;
>  
>  	page_sz = getpagesize();
> +	buf = SAFE_CALLOC(page_sz, sizeof(char));
                                   ^
				   this is 1 by definition

So we can do just SAFE_MALLOC(page_sz);

> +	memset(buf, 'A', page_sz);
>  
> -	/* Allocate space for the test buffer */
> -	if ((tst_buff = calloc(page_sz, sizeof(char))) == NULL) {
> -		tst_brkm(TFAIL, NULL, "calloc failed (tst_buff)");
> -	}
> -
> -	/* Fill the test buffer with the known data */
> -	memset(tst_buff, 'A', page_sz);
> +	fd = SAFE_OPEN(TEMPFILE, O_RDWR | O_CREAT, 0666);
> +	SAFE_WRITE(SAFE_WRITE_ALL, fd, buf, page_sz);
> +	free(buf);
> +	SAFE_FCHMOD(fd, 0444);
> +	SAFE_CLOSE(fd);
>  
> -	tst_tmpdir();
> -
> -	/* Creat a temporary file used for mapping */
> -	if ((fildes = open(TEMPFILE, O_RDWR | O_CREAT, 0666)) < 0) {
> -		free(tst_buff);
> -		tst_brkm(TFAIL | TERRNO, cleanup, "opening %s failed",
> -			 TEMPFILE);
> -	}
> -
> -	/* Write test buffer contents into temporary file */
> -	if (write(fildes, tst_buff, page_sz) < (int)page_sz) {
> -		free(tst_buff);
> -		tst_brkm(TFAIL | TERRNO, cleanup,
> -			 "writing to %s failed", TEMPFILE);
> -	}
> -
> -	/* Free the memory allocated for test buffer */
> -	free(tst_buff);
> -
> -	/* Change Mode permissions on Temporary file */
> -	if (fchmod(fildes, 0444) < 0) {
> -		tst_brkm(TFAIL | TERRNO, cleanup, "fchmod(%s, 0444) failed",
> -			 TEMPFILE);
> -	}
> +	fd = SAFE_OPEN(TEMPFILE, O_RDONLY);
> +	dummy = SAFE_CALLOC(page_sz, sizeof(char));

Why do we allocate second buffer? Cant we just keep the buf? Bonus point
is that the buf was alread filled with data we expect in the file, so we
don't have to read them with read() in the run() function.

> +}
>  
> -	/* Close the temporary file */
> -	if (close(fildes) < 0) {
> -		tst_brkm(TFAIL | TERRNO, cleanup, "closing %s failed",
> -			 TEMPFILE);
> +static void run(void)
> +{
> +	addr = mmap(0, page_sz, PROT_READ, MAP_FILE | MAP_SHARED, fd, 0);
> +	if (addr == MAP_FAILED) {
> +		tst_res(TFAIL | TERRNO, "mmap() of %s failed", TEMPFILE);
> +		return;
>  	}
>  
> -	/* Open the temporary file again, - Readonly mode */
> -	if ((fildes = open(TEMPFILE, O_RDONLY)) < 0) {
> -		tst_brkm(TFAIL, cleanup, "reopening %s readonly failed",
> -			 TEMPFILE);
> -	}
> +	SAFE_READ(1, fd, dummy, page_sz);
> +	SAFE_LSEEK(fd, 0, SEEK_SET);
>  
> -	/* 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)");
> -	}
> +	if (memcmp(dummy, addr, page_sz) == 0)
> +		tst_res(TPASS, "mmap() functionality successful");
> +	else
> +		tst_res(TFAIL, "mapped memory area contains invalid data");
>  
> +	SAFE_MUNMAP(addr, page_sz);
>  }
>  
>  static void cleanup(void)
>  {
> -	close(fildes);
> -	free(dummy);
> -	tst_rmdir();
> +	if (fd > 0)
> +		SAFE_CLOSE(fd);
> +	if (dummy)
> +		free(dummy);

free(NULL) is no-op so there is no need for the if ().

>  }
> +
> +static struct tst_test test = {
> +	.setup = setup,
> +	.cleanup = cleanup,
> +	.test_all = run,
> +	.needs_tmpdir = 1
> +};
> -- 
> 2.41.0
> 
> 
> -- 
> Mailing list info: https://lists.linux.it/listinfo/ltp

-- 
Cyril Hrubis
chrubis@suse.cz

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