diff mbox series

[v3,4/5] syscalls/clone06: Convert to new API

Message ID 20210923085224.868-4-zhanglianjie@uniontech.com
State Changes Requested
Headers show
Series [v3,1/5] syscalls/clone02: Convert to new API | expand

Commit Message

zhanglianjie Sept. 23, 2021, 8:52 a.m. UTC 
Signed-off-by: zhanglianjie <zhanglianjie@uniontech.com>

--
2.20.1

Comments

Cyril Hrubis Oct. 12, 2021, 9:21 a.m. UTC | #1 
Hi!
> -	cleanup();
> -	tst_exit();
> +	parent_env = getenv("TERM") ? : "";

I guess that it would make more sense to create a new variable than
depend on anything that may or may not be present on the system.

What about we setenv a variable instead and use that for the test?

#define ENV_VAL "LTP test variable value"
#define ENV_ID "LTP_CLONE_TEST"

static void setup(void)
{
	int ret;

	ret = setenv(ENV_ID, ENV_VAL, 0)
	if (ret)
		tst_brk(TBROK | TERRNO, "setenv() failed");

}


> +	TST_EXP_VAL(strcmp(buff, parent_env), 0,
> +				"verify environment variables by child");

Also there is no need to propagate the value to the parent like this,
the child process can report the result (in the new library) as well, so
this really could be as simple as:

static int do_child(void *arg LTP_ATTRIBUTE_UNUSED)
{
	const char *env_val = getenv(ENV_ID);

	if (!env_val) {
		tst_res(TFAIL, "Variable " ENV_ID " not defined in child");
		return;
	}

	if (strcmp(env_val, ENV_VAL)) {
		tst_res(TFAIL, "Variable value is different");
		return;
	}

	tst_res(TPASS, ...);
}

>  }
> 
>  static void setup(void)
>  {
> -	tst_sig(FORK, DEF_HANDLER, cleanup);
> -	TEST_PAUSE;
> +	child_stack = SAFE_MALLOC(CHILD_STACK_SIZE);
> +	buff = SAFE_MMAP(NULL, MAX_LINE_LENGTH, PROT_READ | PROT_WRITE,
> +			MAP_SHARED | MAP_ANONYMOUS, -1, 0);
>  }
> 
>  static void cleanup(void)
>  {
> -}
> -
> -/*
> - *	Function executed by child.
> - *	Gets the value for environment variable,TERM &
> - *	writes it to  a pipe.
> - */
> -static int child_environ(void)
> -{
> -
> -	char var[MAX_LINE_LENGTH];
> -
> -	/* Close read end from child */
> -	if ((close(pfd[0])) == -1)
> -		tst_brkm(TBROK | TERRNO, cleanup, "close(pfd[0]) failed");
> -
> -	if ((sprintf(var, "%s", getenv("TERM") ? : "")) < 0)
> -		tst_resm(TWARN | TERRNO, "sprintf() failed");
> -
> -	if ((write(pfd[1], var, MAX_LINE_LENGTH)) == -1)
> -		tst_resm(TWARN | TERRNO, "write to pipe failed");
> -
> -	/* Close write end of pipe from child */
> -	if ((close(pfd[1])) == -1)
> -		tst_resm(TWARN | TERRNO, "close(pfd[1]) failed");
> +	free(child_stack);
> 
> -	exit(0);
> +	if (buff)
> +		SAFE_MUNMAP(buff, MAX_LINE_LENGTH);
>  }
> +
> +static struct tst_test test = {
> +	.setup = setup,
> +	.test_all = verify_clone,
> +	.cleanup = cleanup,
> +};
> --
> 2.20.1
> 
> 
> 
> 
> -- 
> Mailing list info: https://lists.linux.it/listinfo/ltp
zhanglianjie Oct. 13, 2021, 2:49 a.m. UTC | #2 
Hi,
This is a good idea, just do it, thank you.

On 2021-10-12 17:21, Cyril Hrubis wrote:
> Hi!
>> -	cleanup();
>> -	tst_exit();
>> +	parent_env = getenv("TERM") ? : "";
> 
> I guess that it would make more sense to create a new variable than
> depend on anything that may or may not be present on the system.
> 
> What about we setenv a variable instead and use that for the test?
> 
> #define ENV_VAL "LTP test variable value"
> #define ENV_ID "LTP_CLONE_TEST"
> 
> static void setup(void)
> {
> 	int ret;
> 
> 	ret = setenv(ENV_ID, ENV_VAL, 0)
> 	if (ret)
> 		tst_brk(TBROK | TERRNO, "setenv() failed");
> 
> }
> 
> 
>> +	TST_EXP_VAL(strcmp(buff, parent_env), 0,
>> +				"verify environment variables by child");
> 
> Also there is no need to propagate the value to the parent like this,
> the child process can report the result (in the new library) as well, so
> this really could be as simple as:
> 
> static int do_child(void *arg LTP_ATTRIBUTE_UNUSED)
> {
> 	const char *env_val = getenv(ENV_ID);
> 
> 	if (!env_val) {
> 		tst_res(TFAIL, "Variable " ENV_ID " not defined in child");
> 		return;
> 	}
> 
> 	if (strcmp(env_val, ENV_VAL)) {
> 		tst_res(TFAIL, "Variable value is different");
> 		return;
> 	}
> 
> 	tst_res(TPASS, ...);
> }
> 
>>   }
>>
>>   static void setup(void)
>>   {
>> -	tst_sig(FORK, DEF_HANDLER, cleanup);
>> -	TEST_PAUSE;
>> +	child_stack = SAFE_MALLOC(CHILD_STACK_SIZE);
>> +	buff = SAFE_MMAP(NULL, MAX_LINE_LENGTH, PROT_READ | PROT_WRITE,
>> +			MAP_SHARED | MAP_ANONYMOUS, -1, 0);
>>   }
>>
>>   static void cleanup(void)
>>   {
>> -}
>> -
>> -/*
>> - *	Function executed by child.
>> - *	Gets the value for environment variable,TERM &
>> - *	writes it to  a pipe.
>> - */
>> -static int child_environ(void)
>> -{
>> -
>> -	char var[MAX_LINE_LENGTH];
>> -
>> -	/* Close read end from child */
>> -	if ((close(pfd[0])) == -1)
>> -		tst_brkm(TBROK | TERRNO, cleanup, "close(pfd[0]) failed");
>> -
>> -	if ((sprintf(var, "%s", getenv("TERM") ? : "")) < 0)
>> -		tst_resm(TWARN | TERRNO, "sprintf() failed");
>> -
>> -	if ((write(pfd[1], var, MAX_LINE_LENGTH)) == -1)
>> -		tst_resm(TWARN | TERRNO, "write to pipe failed");
>> -
>> -	/* Close write end of pipe from child */
>> -	if ((close(pfd[1])) == -1)
>> -		tst_resm(TWARN | TERRNO, "close(pfd[1]) failed");
>> +	free(child_stack);
>>
>> -	exit(0);
>> +	if (buff)
>> +		SAFE_MUNMAP(buff, MAX_LINE_LENGTH);
>>   }
>> +
>> +static struct tst_test test = {
>> +	.setup = setup,
>> +	.test_all = verify_clone,
>> +	.cleanup = cleanup,
>> +};
>> --
>> 2.20.1
>>
>>
>>
>>
>> -- 
>> Mailing list info: https://lists.linux.it/listinfo/ltp
>
diff mbox series

Patch

diff --git a/testcases/kernel/syscalls/clone/clone06.c b/testcases/kernel/syscalls/clone/clone06.c
index 99e7f3864..868fba4a5 100644
--- a/testcases/kernel/syscalls/clone/clone06.c
+++ b/testcases/kernel/syscalls/clone/clone06.c
@@ -1,140 +1,65 @@ 
+// SPDX-License-Identifier: GPL-2.0-only
 /*
  * Copyright (c) Wipro Technologies Ltd, 2002.  All Rights Reserved.
  * Copyright (c) 2012 Wanlong Gao <gaowanlong@cn.fujitsu.com>
- *
- * This program is free software; you can redistribute it and/or modify it
- * under the terms of version 2 of the GNU General Public License as
- * published by the Free Software Foundation.
- *
- * This program is distributed in the hope that it would be useful, but
- * WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
- *
- * You should have received a copy of the GNU General Public License along
- * with this program; if not, write the Free Software Foundation, Inc.,
- * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
- *
  */

-/*
- *	Test to verify inheritance of environment variables by child.
+/*\
+ * [Description]
+ * Test to verify inheritance of environment variables by child.
  */

-#if defined UCLINUX && !__THROW
-/* workaround for libc bug */
-#define __THROW
-#endif
-
-#include <errno.h>
+#include <stdio.h>
+#include <stdlib.h>
 #include <sched.h>
-#include <sys/wait.h>
-#include <fcntl.h>
-#include "test.h"
+#include "tst_test.h"
 #include "clone_platform.h"

 #define MAX_LINE_LENGTH 256

-static void setup(void);
-static void cleanup(void);
-static int child_environ();
-
-static int pfd[2];
-
-char *TCID = "clone06";
-int TST_TOTAL = 1;
+static char *buff;
+static void *child_stack;

-int main(int ac, char **av)
+static int child_environ(void *arg LTP_ATTRIBUTE_UNUSED)
 {
+	sprintf(buff, "%s", getenv("TERM") ? : "");
+	exit(0);
+}

-	int lc, status;
-	void *child_stack;
+static void verify_clone(void)
+{
 	char *parent_env;
-	char buff[MAX_LINE_LENGTH];
-
-	tst_parse_opts(ac, av, NULL, NULL);
-
-	setup();
-
-	child_stack = malloc(CHILD_STACK_SIZE);
-	if (child_stack == NULL)
-		tst_brkm(TBROK, cleanup, "Cannot allocate stack for child");
-
-	for (lc = 0; TEST_LOOPING(lc); lc++) {
-		tst_count = 0;
-
-		if ((pipe(pfd)) == -1)
-			tst_brkm(TBROK | TERRNO, cleanup, "pipe() failed");
-
-		TEST(ltp_clone(SIGCHLD, child_environ, NULL, CHILD_STACK_SIZE,
-			       child_stack));
-
-		if (TEST_RETURN == -1)
-			tst_brkm(TFAIL | TTERRNO, cleanup, "clone failed");
-
-		/* close write end from parent */
-		if ((close(pfd[1])) == -1)
-			tst_resm(TWARN | TERRNO, "close(pfd[1]) failed");
-
-		/* Read env var from read end */
-		if ((read(pfd[0], buff, sizeof(buff))) == -1)
-			tst_brkm(TBROK | TERRNO, cleanup,
-				 "read from pipe failed");

-		/* Close read end from parent */
-		if ((close(pfd[0])) == -1)
-			tst_resm(TWARN | TERRNO, "close(pfd[0]) failed");
+	TST_EXP_PID_SILENT(ltp_clone(SIGCHLD, child_environ, NULL, CHILD_STACK_SIZE,
+				child_stack));

-		parent_env = getenv("TERM") ? : "";
+	if (!TST_PASS)
+		return;

-		if ((strcmp(buff, parent_env)) == 0)
-			tst_resm(TPASS, "Test Passed");
-		else
-			tst_resm(TFAIL, "Test Failed");
+	tst_reap_children();

-		if ((wait(&status)) == -1)
-			tst_brkm(TBROK | TERRNO, cleanup,
-				 "wait failed, status: %d", status);
-	}
-
-	free(child_stack);
-
-	cleanup();
-	tst_exit();
+	parent_env = getenv("TERM") ? : "";
+	TST_EXP_VAL(strcmp(buff, parent_env), 0,
+				"verify environment variables by child");
 }

 static void setup(void)
 {
-	tst_sig(FORK, DEF_HANDLER, cleanup);
-	TEST_PAUSE;
+	child_stack = SAFE_MALLOC(CHILD_STACK_SIZE);
+	buff = SAFE_MMAP(NULL, MAX_LINE_LENGTH, PROT_READ | PROT_WRITE,
+			MAP_SHARED | MAP_ANONYMOUS, -1, 0);
 }

 static void cleanup(void)
 {
-}
-
-/*
- *	Function executed by child.
- *	Gets the value for environment variable,TERM &
- *	writes it to  a pipe.
- */
-static int child_environ(void)
-{
-
-	char var[MAX_LINE_LENGTH];
-
-	/* Close read end from child */
-	if ((close(pfd[0])) == -1)
-		tst_brkm(TBROK | TERRNO, cleanup, "close(pfd[0]) failed");
-
-	if ((sprintf(var, "%s", getenv("TERM") ? : "")) < 0)
-		tst_resm(TWARN | TERRNO, "sprintf() failed");
-
-	if ((write(pfd[1], var, MAX_LINE_LENGTH)) == -1)
-		tst_resm(TWARN | TERRNO, "write to pipe failed");
-
-	/* Close write end of pipe from child */
-	if ((close(pfd[1])) == -1)
-		tst_resm(TWARN | TERRNO, "close(pfd[1]) failed");
+	free(child_stack);

-	exit(0);
+	if (buff)
+		SAFE_MUNMAP(buff, MAX_LINE_LENGTH);
 }
+
+static struct tst_test test = {
+	.setup = setup,
+	.test_all = verify_clone,
+	.cleanup = cleanup,
+};