[LTP] [PATCH v1] Refactor pidns16 test using new LTP API

[LTP] [PATCH v1] Refactor pidns16 test using new LTP API

[Andrea Cervesato via ltp]

Signed-off-by: Andrea Cervesato <andrea.cervesato@suse.com>
---
 testcases/kernel/containers/pidns/pidns16.c | 183 +++++++-------------
 1 file changed, 61 insertions(+), 122 deletions(-)

diff --git a/testcases/kernel/containers/pidns/pidns16.c b/testcases/kernel/containers/pidns/pidns16.c
index 2ee61065a..e80b4c438 100644
--- a/testcases/kernel/containers/pidns/pidns16.c
+++ b/testcases/kernel/containers/pidns/pidns16.c
@@ -1,157 +1,96 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
 /*
-* Copyright (c) International Business Machines Corp., 2007
-* 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 will 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 to the Free Software
-* Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
-*
-***************************************************************************
-
-* * Test Assertion.
-* *----------------
-* * kill -USR1 container_init
-* *	- from the parent process and also inside a container
-* *	- Where init has defined a custom handler for USR1
-* *	- Should call the handler and
-* *	- Verify whether the signal handler is called from the proper process.
-* *
-* * Description:
-* *  Create PID namespace container.
-* *  Container init defines the handler for SIGUSR1 and waits indefinetly.
-* *  Parent sends SIGUSR1 to container init.
-* *  The signal handler is handled and the cont-init resumes normally.
-* *  From the container, again the signal SIGUSR1 is sent.
-* *  In the sig-handler check if it's invoked from correct pid(parent/container)
-* *  If cont-init wakes up properly -
-* *  it will return expected value at exit which is verified at the end.
-* *
-* * History:
-* *  DATE	  NAME				   DESCRIPTION
-* *  04/11/08  Veerendra C  <vechandr@in.ibm.com> Verifying cont init kill -USR1
-*
-*******************************************************************************/
-#include "config.h"
+ * Copyright (c) International Business Machines Corp., 2007
+ *               04/11/08  Veerendra C  <vechandr@in.ibm.com>
+ * Copyright (C) 2022 SUSE LLC Andrea Cervesato <andrea.cervesato@suse.com>
+ */
+
+/*\
+ * [Description]
+ *
+ * Clone a process with CLONE_NEWPID flag and verifies that siginfo->si_pid is
+ * set to 0 if sender (parent process) sent the signal. Then send signal from
+ * container itself and check if siginfo->si_pid is set to 1.
+ */
 
 #define _GNU_SOURCE 1
-#include <stdio.h>
-#include <stdlib.h>
-#include <sys/wait.h>
-#include <sys/types.h>
 #include <signal.h>
-#include <unistd.h>
-#include "pidns_helper.h"
-#include "test.h"
+#include "tst_test.h"
+#include "lapi/namespaces_constants.h"
 
-#define CHILD_PID	1
-#define PARENT_PID	0
+static int counter;
 
-char *TCID = "pidns16";
-int TST_TOTAL = 3;
-
-void child_signal_handler(int sig, siginfo_t * si, void *unused)
+static void child_signal_handler(LTP_ATTRIBUTE_UNUSED int sig, siginfo_t *si, LTP_ATTRIBUTE_UNUSED void *unused)
 {
-	static int c = 1;
-	pid_t expected_pid;
-
-	/* Verifying from which process the signal handler is signalled */
+	pid_t exp_pid;
 
-	switch (c) {
-	case 1:
-		expected_pid = PARENT_PID;
+	switch (counter) {
+	case 0:
+		exp_pid = 0;
 		break;
-	case 2:
-		expected_pid = CHILD_PID;
+	case 1:
+		exp_pid = 1;
 		break;
 	default:
-		tst_resm(TBROK, "child should NOT be signalled 3+ times");
+		tst_brk(TBROK, "Child should NOT be signalled 3+ times");
 		return;
 	}
 
-	if (si->si_pid == expected_pid)
-		tst_resm(TPASS, "child is signalled from expected pid %d",
-			 expected_pid);
+	if (si->si_pid == exp_pid)
+		tst_res(TPASS, "Signalling PID is %d as expected", exp_pid);
 	else
-		tst_resm(TFAIL, "child is signalled from unexpected pid %d,"
-			 " expecting pid %d", si->si_pid, expected_pid);
+		tst_res(TFAIL, "Signalling PID is not %d, but %d", exp_pid, si->si_pid);
 
-	c++;
+	counter++;
 }
 
-/*
- * child_fn() - Inside container
- */
-int child_fn(void *ttype)
+static int child_func(LTP_ATTRIBUTE_UNUSED void *arg)
 {
 	struct sigaction sa;
-	pid_t pid, ppid;
+	pid_t cpid, ppid;
 
-	/* Set process id and parent pid */
-	pid = getpid();
+	cpid = getpid();
 	ppid = getppid();
 
-	if ((pid != CHILD_PID) || (ppid != PARENT_PID))
-		tst_resm(TBROK, "pidns is not created.");
+	if (cpid != 1 || ppid != 0) {
+		tst_res(TFAIL, "Got unexpected result of cpid=%d ppid=%d", cpid, ppid);
+		return 1;
+	}
 
-	/* Set signal handler for SIGUSR1, also mask other signals */
 	sa.sa_flags = SA_SIGINFO;
-	sigemptyset(&sa.sa_mask);
+	SAFE_SIGFILLSET(&sa.sa_mask);
 	sa.sa_sigaction = child_signal_handler;
-	if (sigaction(SIGUSR1, &sa, NULL) == -1)
-		tst_resm(TBROK, "%d: sigaction() failed", pid);
-
-	pause();
-	tst_resm(TINFO, "Container: Resumed after receiving SIGUSR1 "
-		 "from parentNS ");
-	if (kill(pid, SIGUSR1) != 0) {
-		tst_resm(TFAIL, "kill(SIGUSR1) fails.");
-	}
-	tst_resm(TINFO, "Container: Resumed after sending SIGUSR1 "
-		 "from container itself");
-	_exit(10);
-}
 
-static void setup(void)
-{
-	tst_require_root();
-	check_newpid();
+	SAFE_SIGACTION(SIGUSR1, &sa, NULL);
+
+	TST_CHECKPOINT_WAKE_AND_WAIT(0);
+
+	tst_res(TINFO, "Container: Resumed after receiving SIGUSR1 from parent namespace");
+
+	SAFE_KILL(cpid, SIGUSR1);
+
+	tst_res(TINFO, "Container: Resumed after sending SIGUSR1 from container itself");
+
+	return 0;
 }
 
-/***********************************************************************
-*   M A I N
-***********************************************************************/
-int main()
+static void run(void)
 {
-	int status;
-	pid_t cpid;
+	int ret;
 
-	setup();
+	ret = ltp_clone_quick(CLONE_NEWPID | SIGCHLD, child_func, NULL);
+	if (ret < 0)
+		tst_brk(TBROK | TERRNO, "clone failed");
 
-	cpid = ltp_clone_quick(CLONE_NEWPID | SIGCHLD, child_fn, NULL);
+	TST_CHECKPOINT_WAIT(0);
 
-	if (cpid < 0) {
-		tst_resm(TBROK, "clone() failed.");
-	}
-
-	sleep(1);
-	if (kill(cpid, SIGUSR1) != 0) {
-		tst_resm(TFAIL, "kill(SIGUSR1) fails.");
-	}
-	sleep(1);
-	if (waitpid(cpid, &status, 0) < 0)
-		tst_resm(TWARN, "waitpid() failed.");
+	SAFE_KILL(ret, SIGUSR1);
 
-	if ((WIFEXITED(status)) && (WEXITSTATUS(status) == 10))
-		tst_resm(TPASS, "container init continued successfuly, "
-			 "after handling signal -USR1");
-	else
-		tst_resm(TFAIL, "c-init failed to continue after "
-			 "passing kill -USR1");
-	tst_exit();
+	TST_CHECKPOINT_WAKE(0);
 }
+
+static struct tst_test test = {
+	.test_all = run,
+	.needs_root = 1,
+	.needs_checkpoints = 1,
+};
-- 
2.35.3


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

Re: [LTP] [PATCH v1] Refactor pidns16 test using new LTP API

[Petr Vorel]

Hi Andrea,

...
> +/*\
> + * [Description]
> + *
> + * Clone a process with CLONE_NEWPID flag and verifies that siginfo->si_pid is
You need to quote variable with +, i.e. +siginfo->si_pid+, otherwise
formatting show siginfo→si_pid instead of siginfo->si_pid.

> + * set to 0 if sender (parent process) sent the signal. Then send signal from
> + * container itself and check if siginfo->si_pid is set to 1.
and here as well.
> + */

>  #define _GNU_SOURCE 1
Do we really need _GNU_SOURCE? Why?

...
> +static int counter;
Shouldn't this be volatile? And maybe also sig_atomic_t instead of int (even
it's the same)?
static volatile sig_atomic_t counter;

> -char *TCID = "pidns16";
> -int TST_TOTAL = 3;
> -
> -void child_signal_handler(int sig, siginfo_t * si, void *unused)
> +static void child_signal_handler(LTP_ATTRIBUTE_UNUSED int sig, siginfo_t *si, LTP_ATTRIBUTE_UNUSED void *unused)
>  {
> -	static int c = 1;
> -	pid_t expected_pid;
> -
> -	/* Verifying from which process the signal handler is signalled */
> +	pid_t exp_pid;

> -	switch (c) {
> -	case 1:
> -		expected_pid = PARENT_PID;
> +	switch (counter) {
> +	case 0:
> +		exp_pid = 0;
>  		break;
> -	case 2:
> -		expected_pid = CHILD_PID;
> +	case 1:
> +		exp_pid = 1;
>  		break;
>  	default:
> -		tst_resm(TBROK, "child should NOT be signalled 3+ times");
> +		tst_brk(TBROK, "Child should NOT be signalled 3+ times");
>  		return;
>  	}

very nit: I'd use if (counter 
if (counter > 1) {
	tst_brk(TBROK, "Child should NOT be signalled 3+ times");
	return;
}
exp_pid = counter;

> +	if (si->si_pid == exp_pid)
> +		tst_res(TPASS, "Signalling PID is %d as expected", exp_pid);
>  	else
> +		tst_res(TFAIL, "Signalling PID is not %d, but %d", exp_pid, si->si_pid);
nit: use TST_EXP_PASS()

...
> +	if (cpid != 1 || ppid != 0) {
> +		tst_res(TFAIL, "Got unexpected result of cpid=%d ppid=%d", cpid, ppid);
> +		return 1;
> +	}
nit: maybe easier for reviewer if these two failures are handled separately.

I also hope we sooner or later get rid of ltp_clone_quick() in favor of
tst_clone(). Not sure where ended the discussion about it, from [1] it looks
like it's needed now. It can wait for another result after we manage to merge
pidns and mqns rewrites.

The rest LGTM. Thanks for your tireless work on rewriting LTP!

Kind regards,
Petr

[1] https://lore.kernel.org/ltp/945eb0ee-b346-5729-3dda-4bff39bb52d9@suse.com/

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

Re: [LTP] [PATCH v1] Refactor pidns16 test using new LTP API

[Cyril Hrubis]

Hi!
> > +static int counter;
> Shouldn't this be volatile? And maybe also sig_atomic_t instead of int (even
> it's the same)?

As long as we only modify the counter from within the signal handler
it's not required.

The probelm that sig_atomic_t fixes is to make sure that the load/store
is single CPU instruction so that we are sure that we don not jump into
the handler in the middle of a sequence of load/store instruction.

And the volatile "turns off" caching of the value, so again anything
that is accessed from both the handler and the rest of the code should
be volatile.

> static volatile sig_atomic_t counter;
> 
> > -char *TCID = "pidns16";
> > -int TST_TOTAL = 3;
> > -
> > -void child_signal_handler(int sig, siginfo_t * si, void *unused)
> > +static void child_signal_handler(LTP_ATTRIBUTE_UNUSED int sig, siginfo_t *si, LTP_ATTRIBUTE_UNUSED void *unused)
> >  {
> > -	static int c = 1;
> > -	pid_t expected_pid;
> > -
> > -	/* Verifying from which process the signal handler is signalled */
> > +	pid_t exp_pid;
> 
> > -	switch (c) {
> > -	case 1:
> > -		expected_pid = PARENT_PID;
> > +	switch (counter) {
> > +	case 0:
> > +		exp_pid = 0;
> >  		break;
> > -	case 2:
> > -		expected_pid = CHILD_PID;
> > +	case 1:
> > +		exp_pid = 1;
> >  		break;
> >  	default:
> > -		tst_resm(TBROK, "child should NOT be signalled 3+ times");
> > +		tst_brk(TBROK, "Child should NOT be signalled 3+ times");
> >  		return;
> >  	}
> 
> very nit: I'd use if (counter 
> if (counter > 1) {
> 	tst_brk(TBROK, "Child should NOT be signalled 3+ times");
> 	return;
> }
> exp_pid = counter;

The bigger problem though is that we call tst_brk() and tst_res() in the
handler, which is not safe at all. The are couple things that can go
wrong here, mostly glibc locks can get deadlocked for the underlying
FILE* and if the underlying fprintf() calls malloc() the malloc data
structures may end up corrupted. It's quite rare for this to happen, but
there have been a few tests that were failing with small probability due
to these mistakes.

-- 
Cyril Hrubis
chrubis@suse.cz

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