Re: [LTP] [PATCH 1/2] setpgid/setpgid01.c: cleanup

[Jan S. <jst...@re...>]

----- Original Message -----
> From: "Xiaoguang Wang" <wan...@cn...>
> To: ltp...@li...
> Sent: Monday, 12 May, 2014 8:09:46 AM
> Subject: [LTP] [PATCH 1/2] setpgid/setpgid01.c: cleanup
> 
> Delete some useless comments.
> Do some code re-arrangement.
> Some cleanup.
> 
> Signed-off-by: Xiaoguang Wang <wan...@cn...>
> ---
>  testcases/kernel/syscalls/setpgid/setpgid01.c | 204
>  +++++++-------------------
>  1 file changed, 57 insertions(+), 147 deletions(-)
> 

> -	/*
> -	 * Make sure current process is NOT a session or pgrp leader
> -	 */
> +static void setpgid_test1(void)
> +{
> +	int ret;
> +	pid_t pgid, pid;
>  
>  	pgid = getpgrp();
>  	pid = getpid();
>  

Hi,

> +	/*
> +	 * Make sure current process is NOT a session or pgrp leader
> +	 */
>  	if (pgid == pid) {

What if we dropped the condition above and always make a child?
If we do that, then setpgid_test1() and setpgid_test2() are almost the same,
with the difference of arguments you pass to setpgid(), so there's a chance
to re-use some code.

Thinking about it a bit more, is there any reason we need a child process?

If it is pgrp leader, we create a child, which inherits parent's PGID and then
calls setpgid(0, pgid_of_parent)
If it's not a pgrp leader, we call setpgid(pid, current_pgid).

We always seem to call setpgid with same pgid as current PGID. Can that
fail in any scenario? Does it matter if it's a pgrp leader or not?

$ cat a.c
int main()
{
	printf("%d %d\n", getpid(), getpgrp());
	printf("%d\n", setpgid(getpid(), getpgrp()));
	return 0;
}

$ ./a.out 
22721 22721
0

Am I missing something?

Regards,
Jan

>  		if ((pid = FORK_OR_VFORK()) == -1) {
> -			tst_brkm(TBROK, cleanup,
> -				 "fork() in setup() failed - errno %d", errno);
> +			tst_brkm(TBROK | TERRNO, cleanup, "fork()");
>  		}
>  
> -		if (pid != 0) {	/* parent - sits and waits */
> -			wait(&status);
> -			exit(WEXITSTATUS(status));
> -		} else {	/* child - continues with test */
> +		if (pid != 0) {
> +			ret = wait4child(pid);
> +		} else {
>  			pid = getpid();
> +			TEST(setpgid(pid, pgid));
> +			if (TEST_RETURN == -1 || getpgrp() != pgid)
> +				exit(1);
> +			else
> +				exit(0);
>  		}
> +	} else {
> +		TEST(setpgid(pid, pgid));
> +		if (TEST_RETURN == -1 || getpgrp() != pgid)
> +			ret = 1;
> +		else
> +			ret = 0;
>  	}
> +
> +	if (ret == 0)
> +		tst_resm(TPASS, "test setpgid(%d, %d) success", pid, pgid);
> +	else
> +		tst_resm(TFAIL, "test setpgid(%d, %d) fail", pid, pgid);
>  }
>  
> -/***************************************************************
> - * cleanup() - performs all ONE TIME cleanup for this test at
> - *		completion or premature exit.
> - ***************************************************************/
> -void cleanup(void)
> +static void setup(void)
>  {
> -	/*
> -	 * print timing stats if that option was specified.
> -	 * print errno log if that option was specified.
> -	 */
> -	TEST_CLEANUP;
> +	tst_sig(FORK, DEF_HANDLER, cleanup);
>  
> +	TEST_PAUSE;
> +}
> +
> +static void cleanup(void)
> +{
> +	TEST_CLEANUP;
>  }
> --
> 1.8.2.1
> 
> 
> ------------------------------------------------------------------------------
> "Accelerate Dev Cycles with Automated Cross-Browser Testing - For FREE
> Instantly run your Selenium tests across 300+ browser/OS combos.
> Get unparalleled scalability from the best Selenium testing platform
> available
> Simple to use. Nothing to install. Get started now for free."
> http://p.sf.net/sfu/SauceLabs
> _______________________________________________
> Ltp-list mailing list
> Ltp...@li...
> https://lists.sourceforge.net/lists/listinfo/ltp-list
>