diff mbox series

[RFC,1/4] syscalls/abort01: convert to new library

Message ID 20190325232012.67123-2-sspatil@android.com
State New
Headers show
Series [RFC,1/4] syscalls/abort01: convert to new library | expand

Commit Message

Sandeep Patil March 25, 2019, 11:20 p.m. UTC 
From: Sandeep Patil <sspatil@google.com>

In the process, drop checks for UCLINUX and WCOREDUMP.
Make the test simple and remove all the logic to detect if a
system is "in stress".

Signed-off-by: Sandeep Patil <sspatil@android.com>
---
 testcases/kernel/syscalls/abort/abort01.c | 171 ++++++----------------
 1 file changed, 48 insertions(+), 123 deletions(-)

Comments

Cyril Hrubis March 26, 2019, 11:58 a.m. UTC | #1 
Hi!
I've further simplified the test and pushed, thanks.

What I have done:

* Got rid of tst_brk(TFAIL, ...) calls
  see: https://github.com/linux-test-project/ltp/issues/462

* Used tst_strsig() and tst_strstatus() to print signal and status

* Used tst_res() API in the child

* Got rid of unused variables, etc.

diff --git a/testcases/kernel/syscalls/abort/abort01.c b/testcases/kernel/syscalls/abort/abort01.c
index ac5ddb140..386a22f26 100644
--- a/testcases/kernel/syscalls/abort/abort01.c
+++ b/testcases/kernel/syscalls/abort/abort01.c
@@ -27,51 +27,45 @@
 static void do_child(void)
 {
 	abort();
-	fprintf(stderr, "\tchild - abort failed.\n");
-	exit(1);
+	tst_res(TFAIL, "Abort returned");
+	exit(0);
 }
 
-void verify_abort(unsigned int nr)
+void verify_abort(void)
 {
-	int i;
-	int status, child, kidpid;
-	int sig, ex;
-	int core;
-	core = ex = sig = 0;
+	int status, kidpid;
+	int sig, core;
 
 	kidpid = SAFE_FORK();
 	if (kidpid == 0)
 		do_child();
 
-	child = SAFE_WAIT(&status);
-
-	if (WIFSIGNALED(status)) {
-		core = WCOREDUMP(status);
-		sig = WTERMSIG(status);
+	SAFE_WAIT(&status);
 
+	if (!WIFSIGNALED(status)) {
+		tst_res(TFAIL, "Child %s, expected SIGIOT",
+			tst_strstatus(status));
+		return;
 	}
 
-	if (WIFEXITED(status))
-		ex = WEXITSTATUS(status);
+	core = WCOREDUMP(status);
+	sig = WTERMSIG(status);
 
 	if (core == 0)
-		tst_brk(TFAIL,
-			"Missing core dump; exit(%d), signal(%d)",
-			ex, sig);
-	else if (core != -1)
+		tst_res(TFAIL, "abort() failed to dump core");
+	else
 		tst_res(TPASS, "abort() dumped core");
 
 	if (sig == SIGIOT)
 		tst_res(TPASS, "abort() raised SIGIOT");
 	else
-		tst_brk(TFAIL,
-			"Unexpected signal(%d), expected SIGIOT(%d)",
-			sig, SIGIOT);
+		tst_res(TFAIL, "abort() raised %s", tst_strsig(sig));
 }
 
+#define MIN_RLIMIT_CORE (1024 * 1024)
+
 static void setup(void)
 {
-#define MIN_RLIMIT_CORE (1024 * 1024)
 	struct rlimit rlim;
 
 	/* make sure we get core dumps */
@@ -83,9 +77,8 @@ static void setup(void)
 }
 
 static struct tst_test test = {
-	.tcnt = 3,
 	.needs_tmpdir = 1,
 	.forks_child = 1,
 	.setup = setup,
-	.test = verify_abort,
+	.test_all = verify_abort,
 };
Sandeep Patil March 26, 2019, 12:47 p.m. UTC | #2 
On Tue, Mar 26, 2019 at 12:58:25PM +0100, Cyril Hrubis wrote:
> Hi!
> I've further simplified the test and pushed, thanks.
> 
> What I have done:
> 
> * Got rid of tst_brk(TFAIL, ...) calls
>   see: https://github.com/linux-test-project/ltp/issues/462

Thanks for this, it is good to know. What is the recommended replacement?
tst_res()?

> 
> * Used tst_strsig() and tst_strstatus() to print signal and status
> 
> * Used tst_res() API in the child
> 
> * Got rid of unused variables, etc.

I am surprised that didn't throw a warning + build error for me.
other than that, thanks for doing this

> 
> diff --git a/testcases/kernel/syscalls/abort/abort01.c b/testcases/kernel/syscalls/abort/abort01.c
> index ac5ddb140..386a22f26 100644
> --- a/testcases/kernel/syscalls/abort/abort01.c
> +++ b/testcases/kernel/syscalls/abort/abort01.c
> @@ -27,51 +27,45 @@
>  static void do_child(void)
>  {
>  	abort();
> -	fprintf(stderr, "\tchild - abort failed.\n");
> -	exit(1);
> +	tst_res(TFAIL, "Abort returned");
> +	exit(0);
>  }
>  
> -void verify_abort(unsigned int nr)
> +void verify_abort(void)
>  {
> -	int i;
> -	int status, child, kidpid;
> -	int sig, ex;
> -	int core;
> -	core = ex = sig = 0;
> +	int status, kidpid;
> +	int sig, core;
>  
>  	kidpid = SAFE_FORK();
>  	if (kidpid == 0)
>  		do_child();
>  
> -	child = SAFE_WAIT(&status);
> -
> -	if (WIFSIGNALED(status)) {
> -		core = WCOREDUMP(status);
> -		sig = WTERMSIG(status);
> +	SAFE_WAIT(&status);
>  
> +	if (!WIFSIGNALED(status)) {
> +		tst_res(TFAIL, "Child %s, expected SIGIOT",
> +			tst_strstatus(status));
> +		return;
>  	}
>  
> -	if (WIFEXITED(status))
> -		ex = WEXITSTATUS(status);
> +	core = WCOREDUMP(status);
> +	sig = WTERMSIG(status);
>  
>  	if (core == 0)
> -		tst_brk(TFAIL,
> -			"Missing core dump; exit(%d), signal(%d)",
> -			ex, sig);
> -	else if (core != -1)
> +		tst_res(TFAIL, "abort() failed to dump core");
> +	else
>  		tst_res(TPASS, "abort() dumped core");
>  
>  	if (sig == SIGIOT)
>  		tst_res(TPASS, "abort() raised SIGIOT");
>  	else
> -		tst_brk(TFAIL,
> -			"Unexpected signal(%d), expected SIGIOT(%d)",
> -			sig, SIGIOT);
> +		tst_res(TFAIL, "abort() raised %s", tst_strsig(sig));
>  }
>  
> +#define MIN_RLIMIT_CORE (1024 * 1024)
> +
>  static void setup(void)
>  {
> -#define MIN_RLIMIT_CORE (1024 * 1024)
>  	struct rlimit rlim;
>  
>  	/* make sure we get core dumps */
> @@ -83,9 +77,8 @@ static void setup(void)
>  }
>  
>  static struct tst_test test = {
> -	.tcnt = 3,
>  	.needs_tmpdir = 1,
>  	.forks_child = 1,
>  	.setup = setup,
> -	.test = verify_abort,
> +	.test_all = verify_abort,
>  };
> 
> 
> -- 
> Cyril Hrubis
> chrubis@suse.cz
Cyril Hrubis March 26, 2019, 12:51 p.m. UTC | #3 
Hi!
> > * Got rid of tst_brk(TFAIL, ...) calls
> >   see: https://github.com/linux-test-project/ltp/issues/462
> 
> Thanks for this, it is good to know. What is the recommended replacement?
> tst_res()?

Yes, tst_res() followed by either return or exit(0) if you need to
actually exit the code flow.

> > 
> > * Used tst_strsig() and tst_strstatus() to print signal and status
> > 
> > * Used tst_res() API in the child
> > 
> > * Got rid of unused variables, etc.
> 
> I am surprised that didn't throw a warning + build error for me.
> other than that, thanks for doing this

That depends on gcc version and mix of warning flags...
Sandeep Patil March 28, 2019, 4:07 a.m. UTC | #4 
On Tue, Mar 26, 2019 at 01:51:32PM +0100, Cyril Hrubis wrote:
> Hi!
> > > * Got rid of tst_brk(TFAIL, ...) calls
> > >   see: https://github.com/linux-test-project/ltp/issues/462
> > 
> > Thanks for this, it is good to know. What is the recommended replacement?
> > tst_res()?
> 
> Yes, tst_res() followed by either return or exit(0) if you need to
> actually exit the code flow.
> 
> > > 
> > > * Used tst_strsig() and tst_strstatus() to print signal and status
> > > 
> > > * Used tst_res() API in the child
> > > 
> > > * Got rid of unused variables, etc.
> > 
> > I am surprised that didn't throw a warning + build error for me.
> > other than that, thanks for doing this
> 
> That depends on gcc version and mix of warning flags...

clang actually throws a lot of warnings for us when we build it natively for
Android. In this case however, I just used my laptop "gcc (Debian 7.3.0-5)"
and default flags for LTP builds.

Do you have plans of enabling -Werror?

- ssp

> 
> -- 
> Cyril Hrubis
> chrubis@suse.cz
Cyril Hrubis April 3, 2019, 12:06 p.m. UTC | #5 
Hi!
> > > I am surprised that didn't throw a warning + build error for me.
> > > other than that, thanks for doing this
> > 
> > That depends on gcc version and mix of warning flags...
> 
> clang actually throws a lot of warnings for us when we build it natively for
> Android. In this case however, I just used my laptop "gcc (Debian 7.3.0-5)"
> and default flags for LTP builds.
> 
> Do you have plans of enabling -Werror?

We cannot do that with the amount of legacy code we have the build would
fail pretty much every time. So no, no plans for the foreseeable future.
Sandeep Patil April 3, 2019, 3:49 p.m. UTC | #6 
On Wed, Apr 03, 2019 at 02:06:53PM +0200, Cyril Hrubis wrote:
> Hi!
> > > > I am surprised that didn't throw a warning + build error for me.
> > > > other than that, thanks for doing this
> > > 
> > > That depends on gcc version and mix of warning flags...
> > 
> > clang actually throws a lot of warnings for us when we build it natively for
> > Android. In this case however, I just used my laptop "gcc (Debian 7.3.0-5)"
> > and default flags for LTP builds.
> > 
> > Do you have plans of enabling -Werror?
> 
> We cannot do that with the amount of legacy code we have the build would
> fail pretty much every time. So no, no plans for the foreseeable future.

Ack, thanks Cyril. I'd rather convert as many tests to the new library before
I get to them then. By the way, I sent V2s for acct01 & accept01 addressing
your comments now.

- ssp
diff mbox series

Patch

diff --git a/testcases/kernel/syscalls/abort/abort01.c b/testcases/kernel/syscalls/abort/abort01.c
index 3a5dff585..ac5ddb140 100644
--- a/testcases/kernel/syscalls/abort/abort01.c
+++ b/testcases/kernel/syscalls/abort/abort01.c
@@ -1,25 +1,12 @@ 
+// SPDX-License-Identifier: GPL-2.0-or-later
+
 /*
  *   Copyright (c) International Business Machines  Corp., 2002
  *   01/02/2003	Port to LTP	avenkat@us.ibm.com
  *   11/11/2002: Ported to LTP Suite by Ananda
  *   06/30/2001	Port to Linux	nsharoff@us.ibm.com
  *
- *   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
- */
-
- /* ALGORITHM
+ * ALGORITHM
  *	Fork child.  Have child abort, check return status.
  *
  * RESTRICTIONS
@@ -35,132 +22,70 @@ 
 #include <unistd.h>
 #include <sys/resource.h>
 
-#include "test.h"
-#include "safe_macros.h"
+#include "tst_test.h"
 
-#define NUM 3
-
-char *TCID = "abort01";
-int TST_TOTAL = 1;
-
-static void setup(void);
-static void cleanup(void);
-static void do_child();
-static int instress();
+static void do_child(void)
+{
+	abort();
+	fprintf(stderr, "\tchild - abort failed.\n");
+	exit(1);
+}
 
-int main(int argc, char *argv[])
+void verify_abort(unsigned int nr)
 {
-	register int i;
-	int status, count, child, kidpid;
+	int i;
+	int status, child, kidpid;
 	int sig, ex;
-
-#ifdef WCOREDUMP
 	int core;
-	core = 0;
-#endif
-	ex = sig = 0;
-
-	tst_parse_opts(argc, argv, NULL, NULL);
-#ifdef UCLINUX
-	maybe_run_child(&do_child, "");
-#endif
-
-	setup();
-
-	for (i = 0; i < NUM; i++) {
-		kidpid = FORK_OR_VFORK();
-		if (kidpid == 0) {
-#ifdef UCLINUX
-			if (self_exec(argv[0], "")) {
-				if (!instress()) {
-					perror("fork failed");
-					exit(1);
-				}
-			}
-#else
-			do_child();
-#endif
-		}
-		if (kidpid < 0)
-			if (!instress())
-				tst_brkm(TBROK | TERRNO, cleanup,
-					 "fork failed");
-		count = 0;
-		while ((child = wait(&status)) > 0)
-			count++;
-		if (count != 1) {
-			tst_brkm(TBROK, cleanup,
-				 "wrong # children waited on; got %d, expected 1",
-				 count);
-		}
-		if (WIFSIGNALED(status)) {
+	core = ex = sig = 0;
 
-#ifdef WCOREDUMP
-			core = WCOREDUMP(status);
-#endif
-			sig = WTERMSIG(status);
+	kidpid = SAFE_FORK();
+	if (kidpid == 0)
+		do_child();
 
-		}
-		if (WIFEXITED(status))
-			ex = WEXITSTATUS(status);
+	child = SAFE_WAIT(&status);
 
-#ifdef WCOREDUMP
-		if (core == 0) {
-			tst_brkm(TFAIL, cleanup,
-				 "Child did not dump core; exit code = %d, "
-				 "signal = %d", ex, sig);
-		} else if (core != -1) {
-			tst_resm(TPASS, "abort dumped core");
-		}
-#endif
-		if (sig == SIGIOT) {
-			tst_resm(TPASS, "abort raised SIGIOT");
-		} else {
-			tst_brkm(TFAIL, cleanup,
-				 "Child did not raise SIGIOT (%d); exit code = %d, "
-				 "signal = %d", SIGIOT, ex, sig);
-		}
+	if (WIFSIGNALED(status)) {
+		core = WCOREDUMP(status);
+		sig = WTERMSIG(status);
 
 	}
 
-	cleanup();
-	tst_exit();
+	if (WIFEXITED(status))
+		ex = WEXITSTATUS(status);
+
+	if (core == 0)
+		tst_brk(TFAIL,
+			"Missing core dump; exit(%d), signal(%d)",
+			ex, sig);
+	else if (core != -1)
+		tst_res(TPASS, "abort() dumped core");
+
+	if (sig == SIGIOT)
+		tst_res(TPASS, "abort() raised SIGIOT");
+	else
+		tst_brk(TFAIL,
+			"Unexpected signal(%d), expected SIGIOT(%d)",
+			sig, SIGIOT);
 }
 
-/* 1024 GNU blocks */
-#define MIN_RLIMIT_CORE (1024 * 1024)
-
 static void setup(void)
 {
+#define MIN_RLIMIT_CORE (1024 * 1024)
 	struct rlimit rlim;
 
-	SAFE_GETRLIMIT(NULL, RLIMIT_CORE, &rlim);
-
+	/* make sure we get core dumps */
+	SAFE_GETRLIMIT(RLIMIT_CORE, &rlim);
 	if (rlim.rlim_cur < MIN_RLIMIT_CORE) {
-		tst_resm(TINFO, "Adjusting RLIMIT_CORE to %i", MIN_RLIMIT_CORE);
 		rlim.rlim_cur = MIN_RLIMIT_CORE;
-		SAFE_SETRLIMIT(NULL, RLIMIT_CORE, &rlim);
+		SAFE_SETRLIMIT(RLIMIT_CORE, &rlim);
 	}
-
-	tst_tmpdir();
 }
 
-static void cleanup(void)
-{
-	unlink("core");
-	tst_rmdir();
-}
-
-static void do_child(void)
-{
-	abort();
-	fprintf(stderr, "\tchild - abort failed.\n");
-	exit(1);
-}
-
-static int instress(void)
-{
-	tst_resm(TINFO,
-		 "System resources may be too low; fork(), select() etc are likely to fail.");
-	return 1;
-}
+static struct tst_test test = {
+	.tcnt = 3,
+	.needs_tmpdir = 1,
+	.forks_child = 1,
+	.setup = setup,
+	.test = verify_abort,
+};