lib: add LTP_IPC_PATH to env. variables by default

PATCH New Jan Stancek 3 participants 13 messages Oct 10, 2017 – Oct 12, 2017 Library code

Share & tools

Jan Stancek

author

2017-10-10 15:05 UTC #0 lore raw

PATCH 1/6 lib: add LTP_IPC_PATH to env. variables by default

Child started by exec() that needs to do tst_reinit() in order to use checkpoints will inherit LTP_IPC_PATH by default. Parent can choose to override/omit it in envp array when calling exec[lvp]e().

Also fix couple places to use LTP_IPC_PATH define.

Signed-off-by Jan Stancek <[email protected]>

---
 lib/tst_test.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

lib/tst_test.c +6 -4

index 26414e31ca77..25cb234425ab 100644
@@ -99,10 +99,12 @@ static void setup_ipc(void)
 	results = SAFE_MMAP(NULL, size, PROT_READ | PROT_WRITE, MAP_SHARED, ipc_fd, 0);
 
 	/* Checkpoints needs to be accessible from processes started by exec() */
-	if (tst_test->needs_checkpoints)
+	if (tst_test->needs_checkpoints) {
 		sprintf(ipc_path, IPC_ENV_VAR "=%s", shm_path);
-	else
+		putenv(ipc_path);
+	} else {
 		SAFE_UNLINK(shm_path);
+	}
 
 	SAFE_CLOSE(ipc_fd);

@@ -130,13 +132,13 @@ static void cleanup_ipc(void)
 
 void tst_reinit(void)
 {
-	const char *path = getenv("LTP_IPC_PATH");
+	const char *path = getenv(IPC_ENV_VAR);
 	size_t size = getpagesize();
 	int fd;
 	void *ptr;
 
 	if (!path)
-		tst_brk(TBROK, "LTP_IPC_PATH is not defined");
+		tst_brk(TBROK, IPC_ENV_VAR" is not defined");
 
 	if (access(path, F_OK))
 		tst_brk(TBROK, "File %s does not exist!", path);

Signature · 1 line

1.8.3.1

Jan Stancek

author

2017-10-10 15:05 UTC #1 lore raw

PATCH 2/6 lib: move tst_get_startwd() to old_tmpdir header

Newlib can't include ltp_priv.h due to conflicts with oldlib.

Signed-off-by Jan Stancek <[email protected]>

---
 include/old/ltp_priv.h   | 3 ---
 include/old/old_tmpdir.h | 3 +++
 2 files changed, 3 insertions(+), 3 deletions(-)

include/old/ltp_priv.h +0 -3

index e678a28682eb..0552457e591f 100644
@@ -24,9 +24,6 @@
 
 #include <stdarg.h>
 
-/* declared in tst_tmpdir.c */
-const char *tst_get_startwd(void);
-
 /*
  * This is the default temporary directory used by tst_tmpdir().
  *

include/old/old_tmpdir.h +3 -0

index b39b615a8bdb..9c61172fd71c 100644
@@ -50,4 +50,7 @@ char *tst_get_tmpdir(void);
  */
 int tst_tmpdir_created(void);
 
+/* declared in tst_tmpdir.c */
+const char *tst_get_startwd(void);
+
 #endif	/* OLD_TMPDIR_H__ */

Signature · 1 line

1.8.3.1

Jan Stancek

author

2017-10-10 15:05 UTC #2 lore raw

PATCH 3/6 safe_macros: add SAFE_SETENV()

In reply to Jan Stancek

Signed-off-by Jan Stancek <[email protected]>

---
 include/tst_safe_macros.h | 7 +++++++
 1 file changed, 7 insertions(+)

include/tst_safe_macros.h +7 -0

index 1e034b8e29d7..5d8952544be3 100644
@@ -447,5 +447,12 @@ int safe_personality(const char *filename, unsigned int lineno,
 		    unsigned long persona);
 #define SAFE_PERSONALITY(persona) safe_personality(__FILE__, __LINE__, persona)
 
+#define SAFE_SETENV(name, value, overwrite) do {		\
+	if (setenv(name, value, overwrite)) {			\
+		tst_brk_(__FILE__, __LINE__, TBROK | TERRNO,	\
+			"setenv(%s, %s, %d) failed",		\
+			name, value, overwrite);		\
+	}							\
+	} while (0)
 
 #endif /* SAFE_MACROS_H__ */

Signature · 1 line

1.8.3.1

Jan Stancek

author

2017-10-10 15:05 UTC #3 lore raw

PATCH 4/6 lib: extend PATH as part of test setup

In reply to Jan Stancek

This patch adds "." and "startdir" to $PATH during setup, so that exec*p() correctly finds child processes. And it also makes it convenient to run some tests directly from git tree.

Signed-off-by Jan Stancek <[email protected]>

---
 lib/tst_test.c | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

lib/tst_test.c +29 -0

index 25cb234425ab..78cce19b6e4a 100644
@@ -841,12 +841,41 @@ static unsigned long long get_time_ms(void)
 	return tv.tv_sec * 1000 + tv.tv_usec / 1000;
 }
 
+static void add_paths(void)
+{
+	char *old_path = getenv("PATH");
+	const char *start_dir;
+	char *new_path;
+	int new_path_size;
+
+	start_dir = tst_get_startwd();
+
+	/* ".:" + "$start_dir" + '\0' */
+	new_path_size = 2 + strlen(start_dir) + 1;
+	if (old_path)
+		/* ":" + "$old_path" */
+		new_path_size += 1 + strlen(old_path);
+
+	new_path = SAFE_MALLOC(new_path_size);
+	strcpy(new_path, ".:");
+	strcat(new_path, start_dir);
+
+	if (old_path) {
+		strcat(new_path, ":");
+		strcat(new_path, old_path);
+	}
+
+	SAFE_SETENV("PATH", new_path, 1);
+	free(new_path);
+}
+
 static void testrun(void)
 {
 	unsigned int i = 0;
 	unsigned long long stop_time = 0;
 	int cont = 1;
 
+	add_paths();
 	do_test_setup();
 
 	if (duration > 0)

Signature · 1 line

1.8.3.1

Cyril Hrubis

2017-10-10 15:18 UTC #4 lore raw

PATCH 4/6 lib: extend PATH as part of test setup

Hi!

Quoted 13 lines

lib/tst_test.c +13 -0

+static void add_paths(void)
+{
+	char *old_path = getenv("PATH");
+	const char *start_dir;
+	char *new_path;
+	int new_path_size;
+
+	start_dir = tst_get_startwd();
+
+	/* ".:" + "$start_dir" + '\0' */
+	new_path_size = 2 + strlen(start_dir) + 1;
+	if (old_path)
+		/* ":" + "$old_path" */

lib/tst_test.c static unsigned long long get_time_ms(void) · line 866

Quoted 10 lines

lib/tst_test.c +10 -0

+		new_path_size += 1 + strlen(old_path);
+
+	new_path = SAFE_MALLOC(new_path_size);
+	strcpy(new_path, ".:");
+	strcat(new_path, start_dir);
+
+	if (old_path) {
+		strcat(new_path, ":");
+		strcat(new_path, old_path);
+	}

I do not like the strcat() function much, I would have probably handled these with asprintf();

	if (old_path)
		SAFE_ASPRINTF(&new_path, "%s::%s", old_path, start_dir);
	else
		SAFE_ASPRINTF(&new_path, "::%s", start_dir);

The rest of the patchset looks good.

BTW, we should also drop the note about setting $PATH from the documentation once this is merged.

Quoted 20 lines

lib/tst_test.c +5 -0

+	SAFE_SETENV("PATH", new_path, 1);
+	free(new_path);
+}
+
 static void testrun(void)
 {
 	unsigned int i = 0;
 	unsigned long long stop_time = 0;
 	int cont = 1;
 
+	add_paths();
 	do_test_setup();
 
 	if (duration > 0)

Signature · 5 lines

1.8.3.1


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

Signature · 2 lines

Cyril Hrubis
[email protected]

Jan Stancek

author

2017-10-11 07:45 UTC #5 lore raw

PATCH 4/6 lib: extend PATH as part of test setup

----- Original Message -----

Quoted 7 lines

I do not like the strcat() function much, I would have probably handled these with asprintf();
	if (old_path)
		SAFE_ASPRINTF(&new_path, "%s::%s", old_path, start_dir);
	else
		SAFE_ASPRINTF(&new_path, "::%s", start_dir);

This makes it simpler, I've modified it to use SAFE_ASPRINTF.

Quoted 3 lines

The rest of the patchset looks good.

I pushed all with your ACK.

Quoted 3 lines

BTW, we should also drop the note about setting $PATH from the documentation once this is merged.

Or tweak it to say, it's no longer necessary for newlib tests.

Regards,
Jan

Cyril Hrubis

2017-10-11 08:00 UTC #6 lore raw

PATCH 4/6 lib: extend PATH as part of test setup

Hi!

Quoted 4 lines

Quoted 2 lines
BTW, we should also drop the note about setting $PATH from the documentation once this is merged.
Or tweak it to say, it's no longer necessary for newlib tests.

Ah, right we still do have oldlib tests.

Signature · 2 lines

Cyril Hrubis
[email protected]

Jan Stancek

author

2017-10-10 15:05 UTC #7 lore raw

PATCH 5/6 creat07: no need to pass tst_ipc_envp anymore

In reply to Jan Stancek

Now that env. variable LTP_IPC_PATH is inherited by default, it's not necessary to pass tst_ipc_envp anymore.

Signed-off-by Jan Stancek <[email protected]>

---
 testcases/kernel/syscalls/creat/creat07.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

testcases/kernel/syscalls/creat/creat07.c +1 -3

index e1b267661c39..236f4edf843f 100644
@@ -38,9 +38,7 @@ static void verify_creat(void)
 
 	pid = SAFE_FORK();
 	if (pid == 0) {
-		char *av[] = {TEST_APP, NULL};
-		(void)execve(TEST_APP, av, tst_ipc_envp);
-		perror("execve failed");
+		SAFE_EXECLP(TEST_APP, TEST_APP, NULL);
 		exit(1);
 	}

Signature · 1 line

1.8.3.1

Cyril Hrubis

2017-10-12 08:08 UTC #8 lore raw

PATCH 5/6 creat07: no need to pass tst_ipc_envp anymore

Hi!

Quoted 10 lines

Signed-off-by Jan Stancek <[email protected]>
---
 testcases/kernel/syscalls/creat/creat07.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)
testcases/kernel/syscalls/creat/creat07.c +0 -0
index e1b267661c39..236f4edf843f 100644

testcases/kernel/syscalls/creat/creat07.c static void verify_creat(void) · line 41

Quoted 7 lines

testcases/kernel/syscalls/creat/creat07.c +1 -3

@@ -38,9 +38,7 @@ static void verify_creat(void)
 
 	pid = SAFE_FORK();
 	if (pid == 0) {
-		char *av[] = {TEST_APP, NULL};
-		(void)execve(TEST_APP, av, tst_ipc_envp);
-		perror("execve failed");
+		SAFE_EXECLP(TEST_APP, TEST_APP, NULL);

And it looks like this broke the test when executed from installed LTP, that is because the execlp() picks up the TEST_APP from /opt/ltp/testcasese/bin/ instead of the local copy. I guess that we have to use exec() variant without the p at the end here.

Signature · 2 lines

Cyril Hrubis
[email protected]

Li Wang

2017-10-12 08:47 UTC #9 lore raw

PATCH 5/6 creat07: no need to pass tst_ipc_envp anymore

Quoted 12 lines · Cyril Hrubis

Hi!
Quoted 11 lines
Signed-off-by Jan Stancek <[email protected]>
---
 testcases/kernel/syscalls/creat/creat07.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)
testcases/kernel/syscalls/creat/creat07.c +0 -0
index e1b267661c39..236f4edf843f 100644
@@ -38,9 +38,7 @@ static void verify_creat(void)
 

Quoted 9 lines

testcases/kernel/syscalls/creat/creat07.c static void verify_creat(void) · line 41
Quoted 6 lines
testcases/kernel/syscalls/creat/creat07.c +1 -3
      pid = SAFE_FORK();
      if (pid == 0) {
-             char *av[] = {TEST_APP, NULL};
-             (void)execve(TEST_APP, av, tst_ipc_envp);
-             perror("execve failed");
+             SAFE_EXECLP(TEST_APP, TEST_APP, NULL);
And it looks like this broke the test when executed from installed LTP, that is because the execlp() picks up the TEST_APP from

Yes, I also hit the problem when running creat07 with LTP framework.

# /opt/ltp/runltp -s creat07
...
incrementing stop
tst_test.c:958: INFO: Timeout per run is 0h 05m 00s
creat07.c:50: FAIL: creat() succeeded unexpectedly
^Cincrementing stop
Sending SIGKILL to test process...
tst_test.c:1008: INFO: If you are running on slow machine, try
exporting LTP_TIMEOUT_MUL > 1
tst_test.c:1009: BROK: Test killed! (timeout?)

Quoted 2 lines

/opt/ltp/testcasese/bin/ instead of the local copy. I guess that we have to use exec() variant without the p at the end here.

Right! After replacing the execlp() with execl(), the issue is gone.

# /opt/ltp/runltp -s creat07
...
incrementing stop
tst_test.c:958: INFO: Timeout per run is 0h 05m 00s
creat07.c:55: PASS: creat() received EXTBSY

Summary:

passed   1
failed   0
skipped  0

warnings 0

------------------

$ git diff

include/tst_safe_macros.h +0 -0

index 5d89525..a865cbd 100644
@@ -403,6 +403,12 @@ static inline sighandler_t safe_signal(const char

*file, const int lineno,

                 "execlp(%s, %s, ...) failed", file, arg); \
        } while (0)

+#define SAFE_EXECL(file, arg, ...) do { \
+ execl((file), (arg), ##__VA_ARGS__); \
+ tst_brk_(__FILE__, __LINE__, TBROK | TERRNO, \
+ "execlp(%s, %s, ...) failed", file, arg); \
+ } while (0)
+
int safe_getpriority(const char *file, const int lineno, int which, id_t who);

 #define SAFE_GETPRIORITY(which, who) \
        safe_getpriority(__FILE__, __LINE__, (which), (who))

(quoted diff) +0 -0

b/testcases/kernel/syscalls/creat/creat07.c
index 236f4ed..a016604 100644

testcases/kernel/syscalls/creat/creat07.c +1 -1

@@ -38,7 +38,7 @@ static void verify_creat(void)
 
        pid = SAFE_FORK();
        if (pid == 0) {
-               SAFE_EXECLP(TEST_APP, TEST_APP, NULL);
+               SAFE_EXECL(TEST_APP, TEST_APP, NULL);
                exit(1);
        }

Signature · 2 lines

Li Wang
[email protected]

Jan Stancek

author

2017-10-12 08:50 UTC #10 lore raw

PATCH 5/6 creat07: no need to pass tst_ipc_envp anymore

In reply to Cyril Hrubis

----- Original Message -----

Quoted 13 lines

Hi!
Quoted 12 lines
Signed-off-by Jan Stancek <[email protected]>
---
 testcases/kernel/syscalls/creat/creat07.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)
testcases/kernel/syscalls/creat/creat07.c +0 -0
index e1b267661c39..236f4edf843f 100644
@@ -38,9 +38,7 @@ static void verify_creat(void)
 

Quoted 11 lines

testcases/kernel/syscalls/creat/creat07.c static void verify_creat(void) · line 41
Quoted 6 lines
testcases/kernel/syscalls/creat/creat07.c +1 -3
 	pid = SAFE_FORK();
 	if (pid == 0) {
-		char *av[] = {TEST_APP, NULL};
-		(void)execve(TEST_APP, av, tst_ipc_envp);
-		perror("execve failed");
+		SAFE_EXECLP(TEST_APP, TEST_APP, NULL);
And it looks like this broke the test when executed from installed LTP, that is because the execlp() picks up the TEST_APP from /opt/ltp/testcasese/bin/ instead of the local copy. I guess that we have to use exec() variant without the p at the end here.

Or we can switch order so that "." is searched as first:
SAFE_ASPRINTF(&new_path, ":%s:%s", start_dir, old_path);
What do you think?

Regards,
Jan

Jan Stancek

author

2017-10-12 09:34 UTC #11 lore raw

PATCH 5/6 creat07: no need to pass tst_ipc_envp anymore

----- Original Message -----

Quoted 33 lines

From: "Jan Stancek" <[email protected]>
To: "Cyril Hrubis" <[email protected]>
Cc: [email protected]
Sent: Thursday, 12 October, 2017 10:50:32 AM
Subject: Re: [LTP] [PATCH 5/6] creat07: no need to pass tst_ipc_envp anymore
----- Original Message -----
Quoted 13 lines
Hi!
Quoted 12 lines
Signed-off-by Jan Stancek <[email protected]>
---
 testcases/kernel/syscalls/creat/creat07.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)
testcases/kernel/syscalls/creat/creat07.c +0 -0
index e1b267661c39..236f4edf843f 100644
@@ -38,9 +38,7 @@ static void verify_creat(void)
 
Quoted 11 lines
testcases/kernel/syscalls/creat/creat07.c static void verify_creat(void) · line 41
Quoted 6 lines
testcases/kernel/syscalls/creat/creat07.c +1 -3
 	pid = SAFE_FORK();
 	if (pid == 0) {
-		char *av[] = {TEST_APP, NULL};
-		(void)execve(TEST_APP, av, tst_ipc_envp);
-		perror("execve failed");
+		SAFE_EXECLP(TEST_APP, TEST_APP, NULL);
And it looks like this broke the test when executed from installed LTP, that is because the execlp() picks up the TEST_APP from /opt/ltp/testcasese/bin/ instead of the local copy. I guess that we have to use exec() variant without the p at the end here.

Quoted 3 lines

Or we can switch order so that "." is searched as first:
SAFE_ASPRINTF(&new_path, ":%s:%s", start_dir, old_path);
What do you think?

I pushed Li's version, while this is being discussed.

Regards,
Jan

Jan Stancek

author

2017-10-10 15:05 UTC #12 lore raw

PATCH 6/6 lib: drop tst_ipc_envp

In reply to Jan Stancek

There are no users left.

Signed-off-by Jan Stancek <[email protected]>

---
 include/tst_checkpoint.h | 1 -
 lib/tst_test.c           | 1 -
 2 files changed, 2 deletions(-)

include/tst_checkpoint.h +0 -1

index fdce2d6f70b6..d6f7607abade 100644
@@ -37,6 +37,5 @@
         tst_safe_checkpoint_wait(__FILE__, __LINE__, NULL, id, 0);
 
 extern const char *tst_ipc_path;
-extern char *const tst_ipc_envp[];
 
 #endif /* TST_CHECKPOINT__ */

lib/tst_test.c +0 -1

index 78cce19b6e4a..184a9f06aa72 100644
@@ -64,7 +64,6 @@ extern unsigned int tst_max_futexes;
 
 static char ipc_path[1024];
 const char *tst_ipc_path = ipc_path;
-char *const tst_ipc_envp[] = {ipc_path, NULL};
 
 static char shm_path[1024];

Signature · 1 line

1.8.3.1