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

[Xiaoguang Wang <wangxg.fnst@cn.fujitsu.com>]

Hi,

On 05/14/2014 06:05 PM, Jan Stancek wrote:
>
> ----- Original Message -----
>> From: "Xiaoguang Wang" <wangxg.fnst@cn.fujitsu.com>
>> To: ltp-list@lists.sourceforge.net
>> 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 <wangxg.fnst@cn.fujitsu.com>
>> ---
>>  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?

For pid = 0 & pgid = 0 test, I think we should have tests in child process,
because in this case, according to setpgid()'s manpge, this will set
child process's PGID to its PID, we should verify this.

>
> 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?

Thanks for your explanation.
When I did this cleanup, I also did not know why to have this
setting, so I kept this setting. Now it seems it is useless, I
will send a new version, which would remove this 'pgid == pid' code, thanks.

Regards,
Xiaoguang Wang

>
> $ 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-list@lists.sourceforge.net
>> https://lists.sourceforge.net/lists/listinfo/ltp-list
>>
> .
>


------------------------------------------------------------------------------
"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-list@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/ltp-list