Re: [LTP] [PATCH v3] mmap22.c: Test for new MAP_DROPPABLE flag for mmap

[Cyril Hrubis <chrubis@suse.cz>]

Hi!
> +static void test_mmap(void)
> +{
> +	size_t alloc_size = ALLOC_SIZE;
> +	size_t page_size = getpagesize();
> +	void *alloc;
> +	pid_t child;
> +
> +	cg_child = tst_cg_group_mk(tst_cg, "child");
> +	SAFE_CG_PRINTF(tst_cg, "memory.max", "%d", MEM_LIMIT);
> +	SAFE_CG_PRINTF(cg_child, "cgroup.procs", "%d", getpid());
> +
> +	alloc = SAFE_MMAP(0, alloc_size, PROT_READ | PROT_WRITE,
> +			MAP_ANONYMOUS | MAP_DROPPABLE, -1, 0);
> +
> +	memset(alloc, 'A', alloc_size);
> +	for (size_t i = 0; i < alloc_size; i += page_size) {
> +		if (*(char *)(alloc + i) != 'A')
> +			tst_res(TFAIL, "memset failed");
> +	}
> +
> +	child = SAFE_FORK();
> +	if (!child) {
> +		for (;;)
> +			*(char *)malloc(page_size) = 'B';
> +	}
> +
> +	while (1) {
> +		for (size_t i = 0; i < alloc_size; i += page_size) {
> +			if (!tst_remaining_runtime())
> +				tst_brk(TBROK, "MAP_DROPPABLE did not drop memory within the timeout period.");

This should rather be tst_res(TFAIL, ...) followed by goto kill_child;

> +			if (!*(uint8_t *)(alloc + i)) {

We are casting the alloc pointer on each access, it would probably be
more reasonable to defined it as char * instead of void * and drop the
casts from the code.

> +				tst_res(TPASS, "MAP_DROPPABLE test pass.");
> +				goto kill_child;
> +			}
> +		}
> +	}
> +
> +kill_child:
> +	SAFE_KILL(child, SIGKILL);
> +	SAFE_WAITPID(child, NULL, 0);
> +	SAFE_MUNMAP(alloc, alloc_size);
> +}
> +
> +static void setup(void)
> +{
> +	void *addr = mmap(0, 1, PROT_READ | PROT_WRITE,
> +			MAP_ANONYMOUS | MAP_DROPPABLE, -1, 0);
> +	if (addr == MAP_FAILED && errno == EINVAL)
> +		tst_brk(TCONF, "MAP_DROPPABLE not support");

We are missing munmap() here in the case that addr != MAP_FAILED.


Other than these minor points the rest of the code looks good to me.

-- 
Cyril Hrubis
chrubis@suse.cz

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