[LTP] [PATCH 2/4] syscalls/modify_ldt: Replace TINFO with TPASS or TFAIL

[LTP] [PATCH 2/4] syscalls/modify_ldt: Replace TINFO with TPASS or TFAIL

[Zhao Gongyi]

1)remove redundant Variables flag/fail
2)remove redundant log
3)replace TINFO with TPASS or TFAIL
4)check signal generated by child

For those:
	testcases/kernel/syscalls/modify_ldt/modify_ldt01.c
	testcases/kernel/syscalls/modify_ldt/modify_ldt02.c

Signed-off-by: Zhao Gongyi <zhaogongyi@huawei.com>
---
 .../kernel/syscalls/modify_ldt/modify_ldt01.c | 44 +++++--------------
 .../kernel/syscalls/modify_ldt/modify_ldt02.c | 35 ++++-----------
 2 files changed, 18 insertions(+), 61 deletions(-)

diff --git a/testcases/kernel/syscalls/modify_ldt/modify_ldt01.c b/testcases/kernel/syscalls/modify_ldt/modify_ldt01.c
index 248111814..86c5b3e5a 100644
--- a/testcases/kernel/syscalls/modify_ldt/modify_ldt01.c
+++ b/testcases/kernel/syscalls/modify_ldt/modify_ldt01.c
@@ -98,7 +98,6 @@ int main(int ac, char **av)
 	void *ptr;
 	int retval, func;

-	int flag;
 	int seg[4];

 	tst_parse_opts(ac, av, NULL, NULL);
@@ -115,8 +114,6 @@ int main(int ac, char **av)
 		/*
 		 * Check for ENOSYS.
 		 */
-		tst_resm(TINFO, "Enter block 1");
-		flag = 0;
 		ptr = malloc(10);
 		func = 100;
 		retval = modify_ldt(func, ptr, sizeof(ptr));
@@ -125,29 +122,21 @@ int main(int ac, char **av)
 				tst_resm(TFAIL, "modify_ldt() set invalid "
 					 "errno, expected ENOSYS, got: %d",
 					 errno);
-				flag = FAILED;
+			} else {
+				tst_resm(TPASS,
+					"modify_ldt() set expected errno");
 			}
 		} else {
 			tst_resm(TFAIL, "modify_ldt error: "
 				 "unexpected return value %d", retval);
-			flag = FAILED;
 		}

-		if (flag) {
-			tst_resm(TINFO, "block 1 FAILED");
-		} else {
-			tst_resm(TINFO, "block 1 PASSED");
-		}
-		tst_resm(TINFO, "Exit block 1");
 		free(ptr);

 //block2:
 		/*
 		 * Check for EINVAL
 		 */
-		tst_resm(TINFO, "Enter block 2");
-		flag = 0;
-
 		ptr = 0;

 		retval = modify_ldt(1, ptr, sizeof(ptr));
@@ -156,20 +145,14 @@ int main(int ac, char **av)
 				tst_resm(TFAIL, "modify_ldt() set invalid "
 					 "errno, expected EINVAL, got: %d",
 					 errno);
-				flag = FAILED;
+			} else {
+				tst_resm(TPASS,
+					"modify_ldt() set expected errno");
 			}
 		} else {
 			tst_resm(TFAIL, "modify_ldt error: "
 				 "unexpected return value %d", retval);
-			flag = FAILED;
-		}
-
-		if (flag) {
-			tst_resm(TINFO, "block 2 FAILED");
-		} else {
-			tst_resm(TINFO, "block 2 PASSED");
 		}
-		tst_resm(TINFO, "Exit block 2");

 //block3:

@@ -177,7 +160,7 @@ int main(int ac, char **av)
 		 * Create a new LDT segment.
 		 */
 		if (create_segment(seg, sizeof(seg)) == -1) {
-			tst_brkm(TINFO, cleanup, "Creation of segment failed");
+			tst_brkm(TBROK, cleanup, "Creation of segment failed");
 		}

 		/*
@@ -191,21 +174,14 @@ int main(int ac, char **av)
 				tst_resm(TFAIL, "modify_ldt() set invalid "
 					 "errno, expected EFAULT, got: %d",
 					 errno);
-				flag = FAILED;
+			} else {
+				tst_resm(TPASS,
+					"modify_ldt() set expected errno");
 			}
 		} else {
 			tst_resm(TFAIL, "modify_ldt error: "
 				 "unexpected return value %d", retval);
-			flag = FAILED;
 		}
-
-		if (flag) {
-			tst_resm(TINFO, "block 3 FAILED");
-		} else {
-			tst_resm(TINFO, "block 3 PASSED");
-		}
-		tst_resm(TINFO, "Exit block 3");
-
 	}
 	cleanup();
 	tst_exit();
diff --git a/testcases/kernel/syscalls/modify_ldt/modify_ldt02.c b/testcases/kernel/syscalls/modify_ldt/modify_ldt02.c
index c953ac420..769cf6701 100644
--- a/testcases/kernel/syscalls/modify_ldt/modify_ldt02.c
+++ b/testcases/kernel/syscalls/modify_ldt/modify_ldt02.c
@@ -86,15 +86,12 @@ int read_segment(unsigned int);
 void cleanup(void);
 void setup(void);

-#define FAILED 1
-
 int main(int ac, char **av)
 {
 	int lc;

 	int val, pid, status;

-	int flag;
 	int seg[4];

 	tst_parse_opts(ac, av, NULL, NULL);
@@ -108,12 +105,9 @@ int main(int ac, char **av)
 		tst_count = 0;

 //block1:
-		tst_resm(TINFO, "Enter block 1");
-		flag = 0;
-
 		seg[0] = 12345;
 		if (create_segment(seg, sizeof(seg)) == -1) {
-			tst_brkm(TINFO, cleanup, "Creation of segment failed");
+			tst_brkm(TBROK, cleanup, "Creation of segment failed");
 		}

 		val = read_segment(0);
@@ -121,23 +115,12 @@ int main(int ac, char **av)
 		if (val != seg[0]) {
 			tst_resm(TFAIL, "Invalid value read %d, expected %d",
 				 val, seg[0]);
-			flag = FAILED;
-		}
-
-		if (flag) {
-			tst_resm(TINFO, "block 1 FAILED");
-		} else {
-			tst_resm(TINFO, "block 1 PASSED");
-		}
-
-		tst_resm(TINFO, "Exit block 1");
+		} else
+			tst_resm(TPASS, "value read as expected");

 //block2:
-		tst_resm(TINFO, "Enter block 2");
-		flag = 0;
-
 		if (create_segment(0, 10) == -1) {
-			tst_brkm(TINFO, cleanup, "Creation of segment failed");
+			tst_brkm(TBROK, cleanup, "Creation of segment failed");
 		}

 		tst_old_flush();
@@ -149,15 +132,13 @@ int main(int ac, char **av)
 		(void)waitpid(pid, &status, 0);

 		if (WEXITSTATUS(status) != 0) {
-			flag = FAILED;
 			tst_resm(TFAIL, "Did not generate SEGV, child returned "
 				 "unexpected status");
-		}
-
-		if (flag) {
-			tst_resm(TINFO, "block 2 FAILED");
 		} else {
-			tst_resm(TINFO, "block 2 PASSED");
+			if (WIFSIGNALED(status) && (WTERMSIG(status) == SIGSEGV))
+				tst_resm(TPASS, "generate SEGV as expected");
+			else
+				tst_resm(TFAIL, "Did not generate SEGV");
 		}
 	}
 	cleanup();
--
2.17.1

[LTP] [PATCH 2/4] syscalls/modify_ldt: Replace TINFO with TPASS or TFAIL

[Cyril Hrubis]

Hi!
Pushed with a minor changes, thanks.

I've removed a few useless comments as well, i.e. things that were
commenting the obvious such as "setup() - performs setup" and the
"//block1:" etc.

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH 2/4] syscalls/modify_ldt: Replace TINFO with TPASS or TFAIL

[Petr Vorel]

Hi Zhao, Cyril,

> diff --git a/testcases/kernel/syscalls/modify_ldt/modify_ldt01.c b/testcases/kernel/syscalls/modify_ldt/modify_ldt01.c
...
> @@ -149,15 +132,13 @@ int main(int ac, char **av)
>  		(void)waitpid(pid, &status, 0);

>  		if (WEXITSTATUS(status) != 0) {
> -			flag = FAILED;
>  			tst_resm(TFAIL, "Did not generate SEGV, child returned "
>  				 "unexpected status");
> -		}
> -
> -		if (flag) {
> -			tst_resm(TINFO, "block 2 FAILED");
>  		} else {
> -			tst_resm(TINFO, "block 2 PASSED");
> +			if (WIFSIGNALED(status) && (WTERMSIG(status) == SIGSEGV))
> +				tst_resm(TPASS, "generate SEGV as expected");
> +			else
> +				tst_resm(TFAIL, "Did not generate SEGV");
FYI: since merging this patch (f5e8e6b11) test fails on this. Is it expected?
I haven't looked closer whether it's a test bug or real issue.

Kind regards,
Petr

[LTP] [PATCH 2/4] syscalls/modify_ldt: Replace TINFO with TPASS or TFAIL

[Cyril Hrubis]

Hi!
> FYI: since merging this patch (f5e8e6b11) test fails on this. Is it expected?
> I haven't looked closer whether it's a test bug or real issue.

What is the test failure output? The only part that was change that
could introduce difference in the output is as far as I can tell this:

                /*
                 * Create a new LDT segment.
                 */
                if (create_segment(seg, sizeof(seg)) == -1) {
-                       tst_brkm(TINFO, cleanup, "Creation of segment failed");
+                       tst_brkm(TBROK, cleanup, "Creation of segment failed");
                }


Which would mean that the test was broken before the change but just
haven't produced any errors.

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH 2/4] syscalls/modify_ldt: Replace TINFO with TPASS or TFAIL

[zhaogongyi]

Hi Petr,

Before my MR merged, the test case would not eixt a nozero value, is it the root cause of the problem?

> 
> Hi Zhao, Cyril,
> 
> > diff --git a/testcases/kernel/syscalls/modify_ldt/modify_ldt01.c
> b/testcases/kernel/syscalls/modify_ldt/modify_ldt01.c
> ...
> > @@ -149,15 +132,13 @@ int main(int ac, char **av)
> >  		(void)waitpid(pid, &status, 0);
> 
> >  		if (WEXITSTATUS(status) != 0) {
> > -			flag = FAILED;
> >  			tst_resm(TFAIL, "Did not generate SEGV, child returned "
> >  				 "unexpected status");
> > -		}
> > -
> > -		if (flag) {
> > -			tst_resm(TINFO, "block 2 FAILED");
> >  		} else {
> > -			tst_resm(TINFO, "block 2 PASSED");
> > +			if (WIFSIGNALED(status) && (WTERMSIG(status) == SIGSEGV))
> > +				tst_resm(TPASS, "generate SEGV as expected");
> > +			else
> > +				tst_resm(TFAIL, "Did not generate SEGV");
> FYI: since merging this patch (f5e8e6b11) test fails on this. Is it expected?
> I haven't looked closer whether it's a test bug or real issue.
> 
> Kind regards,
> Petr

[LTP] [PATCH 2/4] syscalls/modify_ldt: Replace TINFO with TPASS or TFAIL

[zhaogongyi]

Hi,

I have reviewed the patch for some time, but I have not find the possible reason of it. Theoretically the patch didn't change the logic.

Because I have not the i386 machine, so I can't run it.

Best Regards,
Gongyi

> 
> Hi Petr,
> 
> Before my MR merged, the test case would not eixt a nozero value, is it
> the root cause of the problem?
> 
> >
> > Hi Zhao, Cyril,
> >
> > > diff --git a/testcases/kernel/syscalls/modify_ldt/modify_ldt01.c
> > b/testcases/kernel/syscalls/modify_ldt/modify_ldt01.c
> > ...
> > > @@ -149,15 +132,13 @@ int main(int ac, char **av)
> > >  		(void)waitpid(pid, &status, 0);
> >
> > >  		if (WEXITSTATUS(status) != 0) {
> > > -			flag = FAILED;
> > >  			tst_resm(TFAIL, "Did not generate SEGV, child returned "
> > >  				 "unexpected status");
> > > -		}
> > > -
> > > -		if (flag) {
> > > -			tst_resm(TINFO, "block 2 FAILED");
> > >  		} else {
> > > -			tst_resm(TINFO, "block 2 PASSED");
> > > +			if (WIFSIGNALED(status) && (WTERMSIG(status) ==
> SIGSEGV))
> > > +				tst_resm(TPASS, "generate SEGV as expected");
> > > +			else
> > > +				tst_resm(TFAIL, "Did not generate SEGV");
> > FYI: since merging this patch (f5e8e6b11) test fails on this. Is it
> expected?
> > I haven't looked closer whether it's a test bug or real issue.
> >
> > Kind regards,
> > Petr

[LTP] [PATCH 2/4] syscalls/modify_ldt: Replace TINFO with TPASS or TFAIL

[Cyril Hrubis]

Hi!
> $ ./modify_ldt02
> modify_ldt02    1  TPASS  :  value read as expected
> modify_ldt02    0  TINFO  :  received signal: 11
> modify_ldt02    2  TFAIL  :  modify_ldt02.c:136: Did not generate SEGV
> 
> (I wrote my report below affected line)
> 
> 11 is SIGSEGV, thus there must be a bug in a test, which probably has been there
> before (test just didn't check). It's strange that WTERMSIG(status) returns 0
> instead of 11.

Looking at the code the bug is obvious, we setup a handler for SIGSEGV
that does exit(0). And since this is old API test we have to reset the
signal handler in the child as well. I will push a fix ASAP.

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH 2/4] syscalls/modify_ldt: Replace TINFO with TPASS or TFAIL

[Petr Vorel]

HI Gongyi,

> I have reviewed the patch for some time, but I have not find the possible reason of it. Theoretically the patch didn't change the logic.
I don't see the problem on first quick look.

> Because I have not the i386 machine, so I can't run it.
FYI You can compile 32bit on 64bit machine:

$ PKG_CONFIG_LIBDIR=/usr/lib/pkgconfig CFLAGS=-m32 LDFLAGS=-m32 ./configure \
&& make && make install

NOTE: PKG_CONFIG_LIBDIR can vary, see INSTALL file. Or you can build LTP 32bit
with build.sh script:

$ ./build.sh -t 32

Kind regards,
Petr

[LTP] [PATCH 2/4] syscalls/modify_ldt: Replace TINFO with TPASS or TFAIL

[Petr Vorel]

Hi,

> Hi!
> > FYI: since merging this patch (f5e8e6b11) test fails on this. Is it expected?
> > I haven't looked closer whether it's a test bug or real issue.

> What is the test failure output? The only part that was change that

$ ./modify_ldt02
modify_ldt02    1  TPASS  :  value read as expected
modify_ldt02    0  TINFO  :  received signal: 11
modify_ldt02    2  TFAIL  :  modify_ldt02.c:136: Did not generate SEGV

(I wrote my report below affected line)

11 is SIGSEGV, thus there must be a bug in a test, which probably has been there
before (test just didn't check). It's strange that WTERMSIG(status) returns 0
instead of 11.

> could introduce difference in the output is as far as I can tell this:

>                 /*
>                  * Create a new LDT segment.
>                  */
>                 if (create_segment(seg, sizeof(seg)) == -1) {
> -                       tst_brkm(TINFO, cleanup, "Creation of segment failed");
> +                       tst_brkm(TBROK, cleanup, "Creation of segment failed");
>                 }


> Which would mean that the test was broken before the change but just
> haven't produced any errors.
Yes.

Old results:
modify_ldt02    0  TINFO  :  Enter block 1
modify_ldt02    0  TINFO  :  block 1 PASSED
modify_ldt02    0  TINFO  :  Exit block 1
modify_ldt02    0  TINFO  :  Enter block 2
modify_ldt02    0  TINFO  :  received signal: 11
modify_ldt02    0  TINFO  :  block 2 PASSED

NOTE: no PASS/FAIL.

Kind regards,
Petr