[LTP] [PATCH v3 1/5] syscalls/clone02: Convert to new API

[zhanglianjie]

Hi,
I will revise it seriously, thank you.

On 2021-10-11 23:40, Cyril Hrubis wrote:
> Hi!
>> -/*
>> - * test_SIG() - This function changes the signal handler for SIGUSR2
>> - *		signal for child. If CLONE_SIGHAND flag is set, this
>> - *		affects parent also.
>> - */
>> -static int test_SIG(void)
>> +static void verify_clone(void)
>>   {
>> +	TST_EXP_PID_SILENT(ltp_clone(tcases[tst_variant].flags, child_fn, NULL,
>> +				CHILD_STACK_SIZE, child_stack));
>>
>> -	struct sigaction new_act;
>> +	if (!TST_PASS)
>> +		return;
>>
>> -	new_act.sa_handler = sig_child_defined_handler;
>> -	new_act.sa_flags = SA_RESTART;
>> -	sigemptyset(&new_act.sa_mask);
>> -
>> -	/* Set signal handler to sig_child_defined_handler */
>> -	if (sigaction(SIGUSR2, &new_act, NULL) == -1) {
>> -		tst_resm(TWARN | TERRNO, "signal failed in test_SIG");
>> -		return -1;
>> -	}
>> -
>> -	/* Send SIGUSR2 signal to parent */
>> -	if (kill(getppid(), SIGUSR2) == -1) {
>> -		tst_resm(TWARN | TERRNO, "kill failed in test_SIG");
>> -		return -1;
>> -	}
>> +	tst_reap_children();
>>
>> -	return 0;
>> +	TST_EXP_PASS(tcases[tst_variant].parent_fn(), "%s", tcases[tst_variant].desc);
> 
> Can we, instead of this, print PASS/FAIL for each check we do, so that
> if something fails the log explains what exactly has failed?
> 
>>   }
>>...
>> - */
>> -static int modified_SIG(void)
>> -{
>> +	/*
>> +	 * Open file from parent, which will be closed by
>> +	 * child, used for testing CLONE_FILES flag
>> +	 */
>> +	fd_parent = SAFE_OPEN(TESTFILE, O_CREAT | O_RDWR, 0777);
>>
>> -	if (parent_got_signal)
>> -		/*
>> -		 * parent came through sig_child_defined_handler()
>> -		 * this means child has changed parent's handler
>> -		 */
>> -		return 1;
>> +	/*
>> +	 * set parent_got_signal to 0, used for testing
>> +	 * CLONE_SIGHAND flag
>> +	 */
>> +	parent_got_signal = 0;
> 
> We have to make sure we reset the $PWD, variable, got_signal flag and
> open() the file before each test iteration otherwise the test will fail
> on subsequent iterations with -i 2 command line parameter.
> 
>> -	return 0;
>> -}
>> +	def_act.sa_handler = sig_parent_default_handler;
>> +	def_act.sa_flags = SA_RESTART;
>> +	SAFE_SIGEMPTYSET(&def_act.sa_mask);
>> +	SAFE_SIGACTION(SIGUSR2, &def_act, NULL);
>>
>> -/*
>> - * sig_child_defined_handler()  - Signal handler installed by child
>> - */
>> -static void sig_child_defined_handler(int pid)
>> -{
>> -	if ((syscall(__NR_gettid)) == child_pid)
>> -		/* Child got signal, give warning */
>> -		tst_resm(TWARN, "Child got SIGUSR2 signal");
>> -	else
>> -		parent_got_signal = TRUE;
>> +	SAFE_MKDIR(TESTDIR, 0777);
>> +	sprintf(cwd_child, "%s/%s", cwd_parent, TESTDIR);
>> +	child_stack = SAFE_MALLOC(CHILD_STACK_SIZE);
> 
> Can we use the guarded buffer instead of MALLOC in this test as well?
> Just as we do in clone01.c now.
> 
>>   }
>>
>> -/* sig_default_handler() - Default handler for parent */
>> -static void sig_default_handler(void)
>> -{
>> -}
>> +static struct tst_test test = {
>> +	.setup = setup,
>> +	.cleanup = cleanup,
>> +	.test_variants = ARRAY_SIZE(tcases),
> 
> This should rather be .tcnt and .test = verify_clone instead of
> variants.
> 
> Test variants are usually used when the whole test is exactly same but
> the TEST_*() function calls different variant of the syscall instead.
> 
>> +	.test_all = verify_clone,
>> +	.needs_tmpdir = 1,
>> +};
> 

-- 
Regards,
Zhang Lianjie