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

[Cyril Hrubis <chrubis@suse.cz>]

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?

>  }
> 
> -/*
> - * modified_VM() - This function is called by parent process to check
> - *		   whether child's modification to parent_variable
> - *		   is visible to parent
> - */
> -
> -static int modified_VM(void)
> -{
> -
> -	if (parent_variable == CHILD_VALUE)
> -		/* child has modified parent_variable */
> -		return 1;
> -
> -	return 0;
> -}
> 
> -/*
> - * modified_FILES() - This function checks for file descriptor table
> - *		      modifications done by child
> - */
> -static int modified_FILES(void)
> +static void cleanup(void)
>  {
> -	char buff[20];
> +	SAFE_CHDIR(cwd_parent);
> +	SAFE_UNLINK(TESTFILE);
> +	SAFE_RMDIR(cwd_child);
> 
> -	if (((read(fd_parent, buff, sizeof(buff))) == -1) && (errno == EBADF))
> -		/* Child has closed this file descriptor */
> -		return 1;
> -
> -	/* close fd_parent */
> -	if ((close(fd_parent)) == -1)
> -		tst_resm(TWARN | TERRNO, "close() failed in modified_FILES()");
> -
> -	return 0;
> +	free(cwd_parent);
> +	free(child_stack);
>  }
> 
> -/*
> - * modified_FS() - This function checks parent's current working directory
> - *		   to see whether its modified by child or not.
> - */
> -static int modified_FS(void)
> +static void setup(void)
>  {
> -	char cwd[FILENAME_MAX];
> -
> -	if ((getcwd(cwd, sizeof(cwd))) == NULL)
> -		tst_resm(TWARN | TERRNO, "getcwd() failed");
> +	struct sigaction def_act;
> 
> -	if (!(strcmp(cwd, cwd_parent)))
> -		/* cwd hasn't changed */
> -		return 0;
> +	/* Save current working directory of parent */
> +	cwd_parent = tst_get_tmpdir();
> 
> -	return 1;
> -}
> +	/*
> +	 * Set value for parent_variable in parent, which will be
> +	 * changed by child for testing CLONE_VM flag
> +	 */
> +	parent_variable = PARENT_VALUE;
> 
> -/*
> - * modified_SIG() - This function checks whether child has changed
> - *		    parent's signal handler for signal, SIGUSR2
> - */
> -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,
> +};

-- 
Cyril Hrubis
chrubis@suse.cz

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