[RFC] lib: Fix result propagation after exec() + tst_reinit()

Message ID 20180731130852.22739-1-chrubis@suse.cz
State Changes Requested
Headers show
Series
  • [RFC] lib: Fix result propagation after exec() + tst_reinit()
Related show

Commit Message

Cyril Hrubis July 31, 2018, 1:08 p.m.
The result pointer wasn't initialized in tst_reinit(), which haven't
allowed for test result propagation to the main test process and there
is actually no reason not to do that. So this commit initializes the
results pointer in tst_reinit() and also adds a needs_ipc_path flag to
tst_test structure, which causes the library not to unlink the IPC file
after it has been mapped and also inserts LTP_IPC_PATH to the
environment.

This commit also adds a test that also shows the usage of the API.

Signed-off-by: Cyril Hrubis <chrubis@suse.cz>
CC: Eddie Horng <eddiehorng.tw@gmail.com>
---
 include/tst_test.h                 |  1 +
 lib/newlib_tests/.gitignore        |  2 ++
 lib/newlib_tests/test_exec.c       | 43 ++++++++++++++++++++++++++++++++++++++
 lib/newlib_tests/test_exec_child.c | 27 ++++++++++++++++++++++++
 lib/tst_test.c                     |  3 ++-
 5 files changed, 75 insertions(+), 1 deletion(-)
 create mode 100644 lib/newlib_tests/test_exec.c
 create mode 100644 lib/newlib_tests/test_exec_child.c

Comments

Li Wang Aug. 1, 2018, 11:09 a.m. | #1
On Tue, Jul 31, 2018 at 9:08 PM, Cyril Hrubis <chrubis@suse.cz> wrote:

> The result pointer wasn't initialized in tst_reinit(), which haven't
> allowed for test result propagation to the main test process and there
> is actually no reason not to do that. So this commit initializes the
> results pointer in tst_reinit() and also adds a needs_ipc_path flag to
> tst_test structure, which causes the library not to unlink the IPC file
> after it has been mapped and also inserts LTP_IPC_PATH to the
> environment.
>
> This commit also adds a test that also shows the usage of the API.
> ...
>
>         ptr = SAFE_MMAP(NULL, size, PROT_READ | PROT_WRITE, MAP_SHARED,
> fd, 0);
> +       results = ptr;
>

The ptr seems redundant here, I suggest to remove it from tst_reinit().

Others looks good to me.
Jan Stancek Aug. 1, 2018, 12:35 p.m. | #2
----- Original Message -----
> The result pointer wasn't initialized in tst_reinit(), which haven't
> allowed for test result propagation to the main test process and there
> is actually no reason not to do that. So this commit initializes the
> results pointer in tst_reinit()

This is obviously correct.

> and also adds a needs_ipc_path flag to
> tst_test structure, which causes the library not to unlink the IPC file
> after it has been mapped and also inserts LTP_IPC_PATH to the
> environment.

What is the negative of skipping unlink() for all tests?
Test itself runs in separate process, so cleanup_ipc() should run
anyway and clean it.

.needs_ipc_path - I'm worried users won't know what this is for.
Maybe if it was called ".child_needs_reinit"? reinit appears
to be the only reason we need to keep shm_path around.

Regards,
Jan

> 
> This commit also adds a test that also shows the usage of the API.
> 
> Signed-off-by: Cyril Hrubis <chrubis@suse.cz>
> CC: Eddie Horng <eddiehorng.tw@gmail.com>
> ---
>  include/tst_test.h                 |  1 +
>  lib/newlib_tests/.gitignore        |  2 ++
>  lib/newlib_tests/test_exec.c       | 43
>  ++++++++++++++++++++++++++++++++++++++
>  lib/newlib_tests/test_exec_child.c | 27 ++++++++++++++++++++++++
>  lib/tst_test.c                     |  3 ++-
>  5 files changed, 75 insertions(+), 1 deletion(-)
>  create mode 100644 lib/newlib_tests/test_exec.c
>  create mode 100644 lib/newlib_tests/test_exec_child.c
> 
> diff --git a/include/tst_test.h b/include/tst_test.h
> index 90938bc29..420597afa 100644
> --- a/include/tst_test.h
> +++ b/include/tst_test.h
> @@ -127,6 +127,7 @@ struct tst_test {
>  	int format_device:1;
>  	int mount_device:1;
>  	int needs_rofs:1;
> +	int needs_ipc_path:1;
>  	/*
>  	 * If set the test function will be executed for all available
>  	 * filesystems and the current filesytem type would be set in the
> diff --git a/lib/newlib_tests/.gitignore b/lib/newlib_tests/.gitignore
> index 8c0981ec8..99edc6404 100644
> --- a/lib/newlib_tests/.gitignore
> +++ b/lib/newlib_tests/.gitignore
> @@ -20,3 +20,5 @@ tst_res_hexd
>  tst_strstatus
>  test17
>  tst_expiration_timer
> +test_exec
> +test_exec_child
> diff --git a/lib/newlib_tests/test_exec.c b/lib/newlib_tests/test_exec.c
> new file mode 100644
> index 000000000..e5b90ffb8
> --- /dev/null
> +++ b/lib/newlib_tests/test_exec.c
> @@ -0,0 +1,43 @@
> +/*
> + * Copyright (c) 2018 Cyril Hrubis <chrubis@suse.cz>
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation; either version 2 of
> + * the License, or (at your option) any later version.
> + *
> + * 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.  See the
> + * GNU General Public License for more details.
> + *
> + * 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 St, Fifth Floor, Boston, MA  02110-1301  USA
> + */
> +
> +/*
> + * Assert that tst_res() from child started by exec() is propagated to the
> main
> + * test process.
> + *
> + * This test should be executed as:
> + * $ PATH=$PATH:$PWD ./test_exec
> + */
> +
> +#define _GNU_SOURCE
> +#include <stdlib.h>
> +#include "tst_test.h"
> +
> +static void do_test(void)
> +{
> +	char *const argv[] = {"test_exec_child", NULL};
> +
> +	execvpe(argv[0], argv, environ);
> +
> +	tst_res(TBROK | TERRNO, "EXEC!");
> +}
> +
> +static struct tst_test test = {
> +	.test_all = do_test,
> +	.needs_ipc_path = 1,
> +};
> diff --git a/lib/newlib_tests/test_exec_child.c
> b/lib/newlib_tests/test_exec_child.c
> new file mode 100644
> index 000000000..696ff5be2
> --- /dev/null
> +++ b/lib/newlib_tests/test_exec_child.c
> @@ -0,0 +1,27 @@
> +/*
> + * Copyright (c) 2018 Cyril Hrubis <chrubis@suse.cz>
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation; either version 2 of
> + * the License, or (at your option) any later version.
> + *
> + * 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.  See the
> + * GNU General Public License for more details.
> + *
> + * 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 St, Fifth Floor, Boston, MA  02110-1301  USA
> + */
> +
> +#define TST_NO_DEFAULT_MAIN
> +#include "tst_test.h"
> +
> +int main(void)
> +{
> +	tst_reinit();
> +	tst_res(TPASS, "Child passed!");
> +	return 0;
> +}
> diff --git a/lib/tst_test.c b/lib/tst_test.c
> index 008bcefe0..8b577fde8 100644
> --- a/lib/tst_test.c
> +++ b/lib/tst_test.c
> @@ -105,7 +105,7 @@ 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 || tst_test->needs_ipc_path) {
>  		sprintf(ipc_path, IPC_ENV_VAR "=%s", shm_path);
>  		putenv(ipc_path);
>  	} else {
> @@ -152,6 +152,7 @@ void tst_reinit(void)
>  	fd = SAFE_OPEN(path, O_RDWR);
>  
>  	ptr = SAFE_MMAP(NULL, size, PROT_READ | PROT_WRITE, MAP_SHARED, fd, 0);
> +	results = ptr;
>  	tst_futexes = (char*)ptr + sizeof(struct results);
>  	tst_max_futexes = (size - sizeof(struct results))/sizeof(futex_t);
>  
> --
> 2.13.6
> 
> 
> --
> Mailing list info: https://lists.linux.it/listinfo/ltp
>
Cyril Hrubis Aug. 3, 2018, 9:13 a.m. | #3
Hi!
> > and also adds a needs_ipc_path flag to
> > tst_test structure, which causes the library not to unlink the IPC file
> > after it has been mapped and also inserts LTP_IPC_PATH to the
> > environment.
> 
> What is the negative of skipping unlink() for all tests?
> Test itself runs in separate process, so cleanup_ipc() should run
> anyway and clean it.

Well we use test ID as well as pid for the filename so it should be
unique enough, we also do O_EXCL on the file so I guess that it should
be safe enough not to unlink() it after it has been mapped.

There is one downside though, in a case that there is no tmpfs mounted
we fall back to mapping a file, in which case we will trigger writeback
to the storage by updating the piece of memory.

And given that there are actually very few testcases that needs to
access the test library from the children started by exec I would like
to keep unlinking the file unless needed.

> .needs_ipc_path - I'm worried users won't know what this is for.
> Maybe if it was called ".child_needs_reinit"? reinit appears
> to be the only reason we need to keep shm_path around.

That sounds definitely better.
Jan Stancek Aug. 3, 2018, 2:16 p.m. | #4
----- Original Message -----
> Hi!
> > > and also adds a needs_ipc_path flag to
> > > tst_test structure, which causes the library not to unlink the IPC file
> > > after it has been mapped and also inserts LTP_IPC_PATH to the
> > > environment.
> > 
> > What is the negative of skipping unlink() for all tests?
> > Test itself runs in separate process, so cleanup_ipc() should run
> > anyway and clean it.
> 
> Well we use test ID as well as pid for the filename so it should be
> unique enough, we also do O_EXCL on the file so I guess that it should
> be safe enough not to unlink() it after it has been mapped.
> 
> There is one downside though, in a case that there is no tmpfs mounted
> we fall back to mapping a file, in which case we will trigger writeback
> to the storage by updating the piece of memory.
> 
> And given that there are actually very few testcases that needs to
> access the test library from the children started by exec I would like
> to keep unlinking the file unless needed.

OK, then let's go with new flag for tst_test struct.
As you said, it's not very commonly used.
Can you add a sentence or two also to docs part that talks about reinit?

Regards,
Jan

> 
> > .needs_ipc_path - I'm worried users won't know what this is for.
> > Maybe if it was called ".child_needs_reinit"? reinit appears
> > to be the only reason we need to keep shm_path around.
> 
> That sounds definitely better.
> 
> --
> Cyril Hrubis
> chrubis@suse.cz
>

Patch

diff --git a/include/tst_test.h b/include/tst_test.h
index 90938bc29..420597afa 100644
--- a/include/tst_test.h
+++ b/include/tst_test.h
@@ -127,6 +127,7 @@  struct tst_test {
 	int format_device:1;
 	int mount_device:1;
 	int needs_rofs:1;
+	int needs_ipc_path:1;
 	/*
 	 * If set the test function will be executed for all available
 	 * filesystems and the current filesytem type would be set in the
diff --git a/lib/newlib_tests/.gitignore b/lib/newlib_tests/.gitignore
index 8c0981ec8..99edc6404 100644
--- a/lib/newlib_tests/.gitignore
+++ b/lib/newlib_tests/.gitignore
@@ -20,3 +20,5 @@  tst_res_hexd
 tst_strstatus
 test17
 tst_expiration_timer
+test_exec
+test_exec_child
diff --git a/lib/newlib_tests/test_exec.c b/lib/newlib_tests/test_exec.c
new file mode 100644
index 000000000..e5b90ffb8
--- /dev/null
+++ b/lib/newlib_tests/test_exec.c
@@ -0,0 +1,43 @@ 
+/*
+ * Copyright (c) 2018 Cyril Hrubis <chrubis@suse.cz>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation; either version 2 of
+ * the License, or (at your option) any later version.
+ *
+ * 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.  See the
+ * GNU General Public License for more details.
+ *
+ * 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 St, Fifth Floor, Boston, MA  02110-1301  USA
+ */
+
+/*
+ * Assert that tst_res() from child started by exec() is propagated to the main
+ * test process.
+ *
+ * This test should be executed as:
+ * $ PATH=$PATH:$PWD ./test_exec
+ */
+
+#define _GNU_SOURCE
+#include <stdlib.h>
+#include "tst_test.h"
+
+static void do_test(void)
+{
+	char *const argv[] = {"test_exec_child", NULL};
+
+	execvpe(argv[0], argv, environ);
+
+	tst_res(TBROK | TERRNO, "EXEC!");
+}
+
+static struct tst_test test = {
+	.test_all = do_test,
+	.needs_ipc_path = 1,
+};
diff --git a/lib/newlib_tests/test_exec_child.c b/lib/newlib_tests/test_exec_child.c
new file mode 100644
index 000000000..696ff5be2
--- /dev/null
+++ b/lib/newlib_tests/test_exec_child.c
@@ -0,0 +1,27 @@ 
+/*
+ * Copyright (c) 2018 Cyril Hrubis <chrubis@suse.cz>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation; either version 2 of
+ * the License, or (at your option) any later version.
+ *
+ * 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.  See the
+ * GNU General Public License for more details.
+ *
+ * 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 St, Fifth Floor, Boston, MA  02110-1301  USA
+ */
+
+#define TST_NO_DEFAULT_MAIN
+#include "tst_test.h"
+
+int main(void)
+{
+	tst_reinit();
+	tst_res(TPASS, "Child passed!");
+	return 0;
+}
diff --git a/lib/tst_test.c b/lib/tst_test.c
index 008bcefe0..8b577fde8 100644
--- a/lib/tst_test.c
+++ b/lib/tst_test.c
@@ -105,7 +105,7 @@  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 || tst_test->needs_ipc_path) {
 		sprintf(ipc_path, IPC_ENV_VAR "=%s", shm_path);
 		putenv(ipc_path);
 	} else {
@@ -152,6 +152,7 @@  void tst_reinit(void)
 	fd = SAFE_OPEN(path, O_RDWR);
 
 	ptr = SAFE_MMAP(NULL, size, PROT_READ | PROT_WRITE, MAP_SHARED, fd, 0);
+	results = ptr;
 	tst_futexes = (char*)ptr + sizeof(struct results);
 	tst_max_futexes = (size - sizeof(struct results))/sizeof(futex_t);