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

[Andrea Cervesato]

Hi!

On 12/18/23 12:58, Petr Vorel wrote:
> 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?
I think the original idea was to wait some memory was released by the 
system, but now I see that the algorithm is just broken, since we are 
not skipping any cycle if mmap fails. So I will just use SAFE_MMAP()
>
>> -		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.
This will go away if we use SAFE_MMAP()
>> +			}
>> +		}
>> -		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.
Because child might end before calling waitpid().
>
>> +
>> +		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

Andrea