Re: [LTP] [PATCH v2] Refactor fork14 using new LTP API

[Petr Vorel <pvorel@suse.cz>]

Hi Andrea,

> From: Andrea Cervesato <andrea.cervesato@suse.com>

> +#ifndef TST_ABI32

> +#include <stdlib.h>
> +#include <sys/wait.h>
> +#include "tst_test.h"
This will fail on 32bit arch, where TST_TEST_TCONF() is used. You need to
include tst_test.h first.

...
> +static void run(void)
{
...
> +	for (i = 0; i < EXTENT; i++) {
> +		mem = mmap(NULL, 1 * TST_GB,
> +			PROT_READ | PROT_WRITE,
> +			MAP_PRIVATE | MAP_ANONYMOUS,
> +			0, 0);
We have SAFE_MMAP(), is there a reason not to use it?

> -		reproduced = fork_test();
> -		if (reproduced == 0)
> -			tst_resm(TPASS, "fork failed as expected.");
> -	}
> -	cleanup();
> -	tst_exit();
> -}
> +		if (mem == MAP_FAILED) {
> +			tst_res(TINFO, "mmap() failed");
...
> +			if (failures > 10) {
> +				tst_brk(TCONF, "mmap() fails too many "
> +					"times, so it's almost impossible to "
> +					"get a vm_area_struct sized 16TB.");
I would join this into single string.
> +			}
> +		}

> -		case -1:
> -			prev_failed = 1;
> -		break;
> -		case 0:
> +		if (!pid)
>  			exit(0);
> -		default:
> -			SAFE_WAITPID(cleanup, -1, NULL, 0);

> -			if (prev_failed > 0 && i >= LARGE) {
> -				tst_resm(TFAIL, "Fork succeeds incorrectly");
> -				reproduced = 1;
> -				goto clear_memory_map;
> -			}
> +		ret = waitpid(pid, NULL, 0);
> +		if (ret == -1 && errno != ECHILD)
> +			tst_brk(TBROK | TERRNO, "waitpid() error");
Why not use SAFE_WAITPID(). It was even used before.

> +
> +		if (prev_failed && i >= LARGE) {
> +			passed = 0;
> +			break;
>  		}
> +
> +		prev_failed = 0;
> +
> +		tst_res(TINFO, "fork() passed at %d attempt", i);
> +	}
> +
> +	for (j = 0; j < i; j++) {
> +		if (memvec[j])
> +			SAFE_MUNMAP(memvec[j], 1 * TST_GB);
>  	}

...
> +static void setup(void)
> +{
> +	memvec = SAFE_MALLOC(EXTENT * sizeof(char *));
> +	memset(memvec, 0, EXTENT);
> +}
> +
> +static void cleanup(void)
> +{
> +	for (long i = 0; i < EXTENT; i++) {
> +		if (memvec[i])
If malloc() in setup() fails (rare case, I know) bad thing happen here,
you should check for (memvec && memvec[i]) here
> +			SAFE_MUNMAP(memvec[i], 1 * TST_GB);
>  	}

Also here should be check if (memvec)
> +	free(memvec);

Kind regards,
Petr

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