Re: [LTP] [PATCH v2 1/2] syscalls/mmap01: Rewrite the test using new LTP API

[Cyril Hrubis <chrubis@suse.cz>]

Hi!
> +static void run(void)
> +{
> +	char buf[20];
>  
> -	/* 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");
> -	}
> +	addr = SAFE_MMAP(NULL, page_sz, PROT_READ | PROT_WRITE, MAP_FILE | MAP_SHARED, fd, 0);
>  
> -	/* 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);
> -	}
> +	if (memcmp(&addr[file_sz], dummy, page_sz - file_sz) != 0)
> +		tst_brk(TFAIL, "mapped memory area contains invalid data");

The tst_brk() does not work correctly with TFAIL, I suppose that the
best we can do here is to tst_res(TFAIL, ...) and goto SAFE_UNMAP().

> -	/* 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);
> -	}
> +	addr[file_sz] = 'X';
> +	addr[file_sz + 1] = 'Y';
> +	addr[file_sz + 2] = 'Z';
>  
> -	/* 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;
> +	if (msync(addr, page_sz, MS_SYNC) != 0)
> +		tst_brk(TFAIL | TERRNO, "failed to sync mapped file");

Here as well. Or alternatively we can add SAFE_MSYNC() to the test
library and use that.

> -	page_sz = getpagesize();
> +	SAFE_READ(0, fd, buf, sizeof(buf));
> +	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 (strstr(buf, "XYZ") == NULL)
> +		tst_res(TPASS, "mmap() functionality successful");
> +	else
> +		tst_res(TFAIL, "mmap() functionality failed");

This can possibly crash, there is no guarantee that the buf is zero
terminated to be suitable for str*() functions.

I guess that the easiest solution would be memchr() for each of the
letters. However the first thing that we should check is actually the
return value from the read(), if that matches the number of bytes we
wrote to the buffer we can actually be sure that we haven't written
anything past the data that did exist in the file.

> -	/* Create the command which will be executed in the test */
> -	sprintf(cmd_buffer, "grep XYZ %s/%s > /dev/null", Path_name, TEMPFILE);
> +	memset(&addr[file_sz], 0, 3);
> +	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);

Again, free(NULL) is no-op, no need for the if here.

>  }
> +
> +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