lib: Introduce tst_strttype()

1) Add tst_check_ttype() to check if TPASS/TFAIL/TWARN/TINFO is passed into tst_brk() and convert it to TBROK forcely.
2) Only update test result in library process and main test process because the exit status of child can be passed into main test process by check_child_status().
3) Increase the number of skipped when calling tst_brk(TCONF).
4) Increase the number of warnings when calling tst_brk(TBROK) in test cleanup(), other than that print "Test broken!" when calling tst_brk(TBROK).

Fix: #408

Signed-off-by Xiao Yang <[email protected]>

---
 lib/tst_test.c | 28 ++++++++++++++++++++++++++--
 1 file changed, 26 insertions(+), 2 deletions(-)

lib/tst_test.c +0 -0

index 661fbbf..c8d8eff 100644

lib/tst_test.c +21 -2

 	unsigned int timeout;
 };
 

@@ -159,6 +160,18 @@ void tst_reinit(void)
 	SAFE_CLOSE(fd);
 }
 
+static int tst_check_ttype(int ttype)
+{
+	if (TTYPE_RESULT(ttype) != TCONF && TTYPE_RESULT(ttype) != TBROK) {
+		tst_res(TINFO, "tst_brk(): invalid type %s, use TBROK forcely",
+			tst_strttype(ttype));
+		ttype &= ~TTYPE_MASK;
+		ttype |= TBROK;
+	}
+
+	return ttype;
+}
+
 static void update_results(int ttype)
 {
 	if (!results)

@@ -177,6 +190,9 @@ static void update_results(int ttype)
 	case TFAIL:
 		tst_atomic_inc(&results->failed);
 	break;
+	case TBROK:
+		tst_atomic_inc(&results->broken);
+	break;
 	}
 }
 

@@ -305,11 +321,15 @@ void tst_vbrk_(const char *file, const int lineno, int
 ttype,
 	 * specified but CLONE_THREAD is not. Use direct syscall to avoid
 	 * cleanup running in the child.
 	 */
-	if (syscall(SYS_getpid) == main_pid)
+	if (syscall(SYS_getpid) == main_pid) {
+		update_results(ttype);
 		do_test_cleanup();
+	}
 
-	if (getpid() == lib_pid)
+	if (getpid() == lib_pid) {
+		update_results(ttype);
 		do_exit(TTYPE_RESULT(ttype));
+	}
 
 	exit(TTYPE_RESULT(ttype));

}

lib/tst_test.c +1 -0

@@ -330,6 +350,7 @@ void tst_brk_(const char *file, const int lineno, int
 ttype,
 	va_list va;
 
 	va_start(va, fmt);
+	ttype = tst_check_ttype(ttype);
 	tst_brk_handler(file, lineno, ttype, fmt, va);
 	va_end(va);

}

lib/tst_test.c +3 -1

@@ -605,6 +626,9 @@ static void do_exit(int ret)
 			ret |= TWARN;
 	}
 
+	if (results->broken)
+		printf("Test broken!\n");
+
 	do_cleanup();
 
 	exit(ret);
--

1.8.3.1

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

Quoted 22 lines

1) Add tst_check_ttype() to check if TPASS/TFAIL/TWARN/TINFO is passed into tst_brk() and convert it to TBROK forcely.
2) Only update test result in library process and main test process because the exit status of child can be passed into main test process by check_child_status().
3) Increase the number of skipped when calling tst_brk(TCONF).
4) Increase the number of warnings when calling tst_brk(TBROK) in test cleanup(), other than that print "Test broken!" when calling tst_brk(TBROK).

Fix: #408

Signed-off-by Xiao Yang<[email protected]>

---
  lib/tst_test.c | 28 ++++++++++++++++++++++++++--
  1 file changed, 26 insertions(+), 2 deletions(-)

lib/tst_test.c +0 -0

index 661fbbf..c8d8eff 100644

lib/tst_test.c struct results { · line 58

Quoted 4 lines

lib/tst_test.c +1 -0

@@ -55,6 +55,7 @@ struct results {
  	int skipped;
  	int failed;
  	int warnings;
+	int broken;

Hi,

I don't follow what benefit this provides. It generates message "Test broken", but we already know that test is broken by message in tst_vbrk_() / tst_cvres().

Regards,
Jan

Quoted 71 lines

lib/tst_test.c +21 -2

  	unsigned int timeout;
  };
 

@@ -159,6 +160,18 @@ void tst_reinit(void)
  	SAFE_CLOSE(fd);
  }
 
+static int tst_check_ttype(int ttype)
+{
+	if (TTYPE_RESULT(ttype) != TCONF&&  TTYPE_RESULT(ttype) != TBROK) {
+		tst_res(TINFO, "tst_brk(): invalid type %s, use TBROK forcely",
+			tst_strttype(ttype));
+		ttype&= ~TTYPE_MASK;
+		ttype |= TBROK;
+	}
+
+	return ttype;
+}
+
  static void update_results(int ttype)
  {
  	if (!results)

@@ -177,6 +190,9 @@ static void update_results(int ttype)
  	case TFAIL:
  		tst_atomic_inc(&results->failed);
  	break;
+	case TBROK:
+		tst_atomic_inc(&results->broken);
+	break;
  	}
  }
 

@@ -305,11 +321,15 @@ void tst_vbrk_(const char *file, const int lineno, int
 ttype,
  	 * specified but CLONE_THREAD is not. Use direct syscall to avoid
  	 * cleanup running in the child.
  	 */
-	if (syscall(SYS_getpid) == main_pid)
+	if (syscall(SYS_getpid) == main_pid) {
+		update_results(ttype);
  		do_test_cleanup();
+	}
 
-	if (getpid() == lib_pid)
+	if (getpid() == lib_pid) {
+		update_results(ttype);
  		do_exit(TTYPE_RESULT(ttype));
+	}
 
  	exit(TTYPE_RESULT(ttype));

  }

lib/tst_test.c +1 -0

@@ -330,6 +350,7 @@ void tst_brk_(const char *file, const int lineno, int
 ttype,
  	va_list va;
 
  	va_start(va, fmt);
+	ttype = tst_check_ttype(ttype);
  	tst_brk_handler(file, lineno, ttype, fmt, va);
  	va_end(va);

  }

lib/tst_test.c +3 -1

@@ -605,6 +626,9 @@ static void do_exit(int ret)
  			ret |= TWARN;
  	}
 
+	if (results->broken)
+		printf("Test broken!\n");
+
  	do_cleanup();
 
  	exit(ret);
--

Quoted 5 lines

1.8.3.1

.

Quoted 29 lines · Jan Stancek

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

Quoted 22 lines

1) Add tst_check_ttype() to check if TPASS/TFAIL/TWARN/TINFO is passed into tst_brk() and convert it to TBROK forcely.
2) Only update test result in library process and main test process because the exit status of child can be passed into main test process by check_child_status().
3) Increase the number of skipped when calling tst_brk(TCONF).
4) Increase the number of warnings when calling tst_brk(TBROK) in test cleanup(), other than that print "Test broken!" when calling tst_brk(TBROK).

Fix: #408

Signed-off-by Xiao Yang<[email protected]>

---
  lib/tst_test.c | 28 ++++++++++++++++++++++++++--
  1 file changed, 26 insertions(+), 2 deletions(-)

lib/tst_test.c +0 -0

index 661fbbf..c8d8eff 100644

lib/tst_test.c struct results { · line 58

Quoted 4 lines

lib/tst_test.c +1 -0

@@ -55,6 +55,7 @@ struct results {
      int skipped;
      int failed;
      int warnings;
+    int broken;

Hi,

Quoted 4 lines

I don't follow what benefit this provides. It generates message "Test broken",
but we already know that test is broken by message in tst_vbrk_() / tst_cvres().

Hi Jan,

We can remove the unnecessary message "Test broken", and also apply the check for
ttype in tst_brk() written by your patch.

According to the issue #$08, we want to increase result counters when calling tst_brk().

e.g.
1) Increase the skipped counter when calling tst_brk(TCONF).
2) Increase the warnings counter when calling tst_brk(TBROK/FAIL) in test cleanup(), other than that increase the failed counter when calling tst_brk(TBROK/FAIL).

Best Regards,
Xiao Yang

Quoted 86 lines

Regards,
Jan

Quoted 80 lines

lib/tst_test.c +12 -0

      unsigned int timeout;
  };
 

@@ -159,6 +160,18 @@ void tst_reinit(void)
      SAFE_CLOSE(fd);
  }
 
+static int tst_check_ttype(int ttype)
+{
+    if (TTYPE_RESULT(ttype) != TCONF&&  TTYPE_RESULT(ttype) != 
 TBROK) {
+        tst_res(TINFO, "tst_brk(): invalid type %s, use TBROK 
 forcely",
+            tst_strttype(ttype));
+        ttype&= ~TTYPE_MASK;
+        ttype |= TBROK;
+    }
+
+    return ttype;
+}
+
  static void update_results(int ttype)

  {
      if (!results)

lib/tst_test.c +3 -0

@@ -177,6 +190,9 @@ static void update_results(int ttype)
      case TFAIL:
          tst_atomic_inc(&results->failed);
      break;
+    case TBROK:
+        tst_atomic_inc(&results->broken);
+    break;
      }
  }
 

@@ -305,11 +321,15 @@ void tst_vbrk_(const char *file, const int 

lineno, int
ttype,
* specified but CLONE_THREAD is not. Use direct syscall to avoid
* cleanup running in the child.
*/

lib/tst_test.c +6 -2

-    if (syscall(SYS_getpid) == main_pid)
+    if (syscall(SYS_getpid) == main_pid) {
+        update_results(ttype);
          do_test_cleanup();
+    }
 
-    if (getpid() == lib_pid)
+    if (getpid() == lib_pid) {
+        update_results(ttype);
          do_exit(TTYPE_RESULT(ttype));
+    }
 
      exit(TTYPE_RESULT(ttype));
  }

@@ -330,6 +350,7 @@ void tst_brk_(const char *file, const int 

lineno, int
ttype,

      va_list va;

      va_start(va, fmt);

lib/tst_test.c +4 -0

+    ttype = tst_check_ttype(ttype);
      tst_brk_handler(file, lineno, ttype, fmt, va);
      va_end(va);
  }

@@ -605,6 +626,9 @@ static void do_exit(int ret)
              ret |= TWARN;
      }
 
+    if (results->broken)
+        printf("Test broken!\n");
+
      do_cleanup();
 
      exit(ret);

Signature · 1 line

1.8.3.1

.

Quoted 29 lines

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

Quoted 22 lines

1) Add tst_check_ttype() to check if TPASS/TFAIL/TWARN/TINFO is passed into tst_brk() and convert it to TBROK forcely.
2) Only update test result in library process and main test process because the exit status of child can be passed into main test process by check_child_status().
3) Increase the number of skipped when calling tst_brk(TCONF).
4) Increase the number of warnings when calling tst_brk(TBROK) in test cleanup(), other than that print "Test broken!" when calling tst_brk(TBROK).

Fix: #408

Signed-off-by Xiao Yang<[email protected]>

---
  lib/tst_test.c | 28 ++++++++++++++++++++++++++--
  1 file changed, 26 insertions(+), 2 deletions(-)

lib/tst_test.c +0 -0

index 661fbbf..c8d8eff 100644

lib/tst_test.c struct results { · line 58

Quoted 4 lines

lib/tst_test.c +1 -0

@@ -55,6 +55,7 @@ struct results {
  	int skipped;
  	int failed;
  	int warnings;
+	int broken;

Hi,

Quoted 4 lines

I don't follow what benefit this provides. It generates message "Test broken",
but we already know that test is broken by message in tst_vbrk_() / tst_cvres().

Hi Jan,

We can remove the unnecessary message "Test broken", and also apply the check for
ttype in tst_brk() written by your patch.

According to the issue #$08, we want to increase result counters when calling tst_brk().
e.g.
1) Increase the skipped counter when calling tst_brk(TCONF).
2) Increase the warnings counter when calling tst_brk(TBROK/FAIL) in test cleanup(), other than that increase the failed counter when calling tst_brk(TBROK/FAIL).

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

Quoted 34 lines · Jan Stancek

Quoted 29 lines

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

Quoted 22 lines

1) Add tst_check_ttype() to check if TPASS/TFAIL/TWARN/TINFO is passed into tst_brk() and convert it to TBROK forcely.
2) Only update test result in library process and main test process because the exit status of child can be passed into main test process by check_child_status().
3) Increase the number of skipped when calling tst_brk(TCONF).
4) Increase the number of warnings when calling tst_brk(TBROK) in test cleanup(), other than that print "Test broken!" when calling tst_brk(TBROK).

Fix: #408

Signed-off-by Xiao Yang<[email protected]>

---
   lib/tst_test.c | 28 ++++++++++++++++++++++++++--
   1 file changed, 26 insertions(+), 2 deletions(-)

lib/tst_test.c +0 -0

index 661fbbf..c8d8eff 100644

lib/tst_test.c struct results { · line 58

Quoted 4 lines

lib/tst_test.c +1 -0

@@ -55,6 +55,7 @@ struct results {
   	int skipped;
   	int failed;
   	int warnings;
+	int broken;

Hi,

Quoted 4 lines

I don't follow what benefit this provides. It generates message "Test broken",
but we already know that test is broken by message in tst_vbrk_() / tst_cvres().

Quoted 5 lines

Hi Jan,

We can remove the unnecessary message "Test broken", and also apply the check for
ttype in tst_brk() written by your patch.

Or maybe add "Broken: " to summary?

Quoted 7 lines

According to the issue #$08, we want to increase result counters when calling tst_brk().
e.g.
1) Increase the skipped counter when calling tst_brk(TCONF).
2) Increase the warnings counter when calling tst_brk(TBROK/FAIL) in test cleanup(), other than that increase the failed counter when calling tst_brk(TBROK/FAIL).

I'd keep counters reflecting the messages. I imagine if summary says "warnings: 1", people would be searching for 'WARN' in output.

TCONF - print CONF message, increase skipped
TFAIL - print FAIL message, increase failed
TBROK - print BROK message, increase broken

What do you think?

Regards,
Jan

.

Quoted 52 lines

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

Quoted 34 lines · Jan Stancek

Quoted 29 lines

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

Quoted 22 lines

1) Add tst_check_ttype() to check if TPASS/TFAIL/TWARN/TINFO is passed into tst_brk() and convert it to TBROK forcely.
2) Only update test result in library process and main test process because the exit status of child can be passed into main test process by check_child_status().
3) Increase the number of skipped when calling tst_brk(TCONF).
4) Increase the number of warnings when calling tst_brk(TBROK) in test cleanup(), other than that print "Test broken!" when calling tst_brk(TBROK).

Fix: #408

Signed-off-by Xiao Yang<[email protected]>

---
   lib/tst_test.c | 28 ++++++++++++++++++++++++++--
   1 file changed, 26 insertions(+), 2 deletions(-)

lib/tst_test.c +0 -0

index 661fbbf..c8d8eff 100644

lib/tst_test.c struct results { · line 58

Quoted 4 lines

lib/tst_test.c +1 -0

@@ -55,6 +55,7 @@ struct results {
   	int skipped;
   	int failed;
   	int warnings;
+	int broken;

Hi,

Quoted 4 lines

I don't follow what benefit this provides. It generates message "Test broken",
but we already know that test is broken by message in tst_vbrk_() / tst_cvres().

Quoted 6 lines

Hi Jan,

We can remove the unnecessary message "Test broken", and also apply the check
for
ttype in tst_brk() written by your patch.

Or maybe add "Broken: " to summary?

Quoted 8 lines

According to the issue #$08, we want to increase result counters when calling
tst_brk().
e.g.
1) Increase the skipped counter when calling tst_brk(TCONF).
2) Increase the warnings counter when calling tst_brk(TBROK/FAIL) in test cleanup(), other than that increase the failed counter when calling tst_brk(TBROK/FAIL).

Quoted 8 lines

I'd keep counters reflecting the messages. I imagine if summary says "warnings: 1", people would be searching for 'WARN' in output.

TCONF - print CONF message, increase skipped
TFAIL - print FAIL message, increase failed
TBROK - print BROK message, increase broken

What do you think?

Hi Jan,

Usually, calling tst_brk() can do above behaviors as you said.

How about doing the following behaviors when calling tst_brk() in test cleanup?
-----------------------------------------------
TCONF - print CONF message, increase skipped
TFAIL - print FAIL message, increase warnings
TBROK - print BROK message, increase warnings
-----------------------------------------------

Best Reagrds,
Xiao Yang

Quoted 7 lines

Regards,
Jan

.

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

Quoted 64 lines · Jan Stancek

Quoted 51 lines

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

Quoted 34 lines · Jan Stancek

Quoted 29 lines

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

Quoted 22 lines

1) Add tst_check_ttype() to check if TPASS/TFAIL/TWARN/TINFO is passed into tst_brk() and convert it to TBROK forcely.
2) Only update test result in library process and main test process because the exit status of child can be passed into main test process by check_child_status().
3) Increase the number of skipped when calling tst_brk(TCONF).
4) Increase the number of warnings when calling tst_brk(TBROK) in test cleanup(), other than that print "Test broken!" when calling tst_brk(TBROK).

Fix: #408

Signed-off-by Xiao Yang<[email protected]>

---
    lib/tst_test.c | 28 ++++++++++++++++++++++++++--
    1 file changed, 26 insertions(+), 2 deletions(-)

lib/tst_test.c +0 -0

index 661fbbf..c8d8eff 100644

lib/tst_test.c struct results { · line 58

Quoted 4 lines

lib/tst_test.c +1 -0

@@ -55,6 +55,7 @@ struct results {
    	int skipped;
    	int failed;
    	int warnings;
+	int broken;

Hi,

Quoted 4 lines

I don't follow what benefit this provides. It generates message "Test broken",
but we already know that test is broken by message in tst_vbrk_() / tst_cvres().

Quoted 6 lines

Hi Jan,

We can remove the unnecessary message "Test broken", and also apply the check
for
ttype in tst_brk() written by your patch.

Or maybe add "Broken: " to summary?

Quoted 8 lines

According to the issue #$08, we want to increase result counters when calling
tst_brk().
e.g.
1) Increase the skipped counter when calling tst_brk(TCONF).
2) Increase the warnings counter when calling tst_brk(TBROK/FAIL) in test cleanup(), other than that increase the failed counter when calling tst_brk(TBROK/FAIL).

Quoted 8 lines

I'd keep counters reflecting the messages. I imagine if summary says "warnings: 1", people would be searching for 'WARN' in output.

TCONF - print CONF message, increase skipped
TFAIL - print FAIL message, increase failed
TBROK - print BROK message, increase broken

What do you think?

Hi Jan,

Usually, calling tst_brk() can do above behaviors as you said.

Quoted 7 lines

How about doing the following behaviors when calling tst_brk() in test cleanup?
-----------------------------------------------
TCONF - print CONF message, increase skipped
TFAIL - print FAIL message, increase warnings
TBROK - print BROK message, increase warnings
-----------------------------------------------

I don't think this matches your v2 patch. In your v2, we would convert FAIL and BROK during test cleanup to WARN. This happens before message is printed. So I think your v2 is proposing:

TCONF - print CONF message, increase skipped
TFAIL - print WARN message, increase warnings
TBROK - print WARN message, increase warnings

I find v2 style more clear, because message in log matches summary at the end.

Regards,
Jan

Quoted 13 lines

Best Reagrds,
Xiao Yang

Quoted 7 lines

Regards,
Jan

.

.

You are right, and i will send a v3 patch as you suggested.

Hi!

Quoted 1 line

You are right, and i will send a v3 patch as you suggested.

Are you going to send v3?

I think that we really should increase counters on tst_brk(), that was an oversight when I designed the library.

However summing up TBROK and TFAIL under results->failed does not look right to me.

Also I'm not sure if we want to convert TFAIL to TWARN inside of test cleanup. And even if we wanted that change it should most likely go in in a separate patch.

Quoted 3 lines

Also I'm not sure if we want to convert TFAIL to TWARN inside of test cleanup. And even if we wanted that change it should most likely go in in a separate patch.

Why do we need to convert TBROK to TWARN in test cleanup? Your fix patch(commit 6440c5d)
has avoided the Infinite loop in test cleanup, so can we just keep the actual result by removing
the existed conversion?

1) Catch and report the TFAIL exit status of child process.

2) Only update result counters in library process and main test process because the exit status of child can be reported by main test process.

3) Print TCONF message and increase skipped when calling tst_brk(TCONF). Print TBROK message and increase broken when calling tst_brk(TBROK). Print TFAIL message and increase failed when calling tst_brk(TFAIL).
4) Remove duplicate update_results() in run_tcases_per_fs().

Hi!

Quoted 1 line

1) Catch and report the TFAIL exit status of child process.

Looking at the codebase we do have a few usages of tst_brk(TFAIL, "...") to exit the child process, which sort of works but it's incorrect. The tst_brk() always meant "unrecoverable failure have happened, exit the current process as fast as possible". Looking over our codebase most of the tst_brk(TFAIL, "...") should not actually cause the main test process to exit, these were only meant to exit the child and report the result in one call. It will for instance break the test with -i option on the first failure, which is incorrect.

So if we ever want to have a function to exit child process with a result we should implement tst_ret() that would be equivalent to tst_res() followed by exit(0).

It could be even implemented as:

#define tst_ret(ttype, fmt, ...) \
	do { \
		tst_res_(__FILE__, __LINE__, (ttype), (fmt), ##__VA_ARGS__); \
		exit(0); \
	} while (0)

This function has one big advantage, it increments the results counters before the child process exits.

Actually one of the big points of the new test library was that the results counters are atomically increased, because passing the results in exit values is nightmare that cannot be done correclty.

Quoted 3 lines

2) Only update result counters in library process and main test process because the exit status of child can be reported by main test process.

Actually after I spend some time on it I think that the best solution is to update the results in the piece of shared memory as fast as possible, anything else is prone to various races and corner cases.

Quoted 5 lines

3) Print TCONF message and increase skipped when calling tst_brk(TCONF). Print TBROK message and increase broken when calling tst_brk(TBROK). Print TFAIL message and increase failed when calling tst_brk(TFAIL).
4) Remove duplicate update_results() in run_tcases_per_fs().

I've been thinking about this and the problem is more complex, and I'm even not sure that it's possible to write the library so that the counters are consistent at the time we exit the test if something unexpected happened and we called tst_brk().

Consider for instance this example:

#include "tst_test.h"

static void do_test(void)

{
        if (!SAFE_FORK())
                tst_brk(TBROK, "child");
        tst_brk(TBROK, "parent");
}

static struct tst_test test = {
.test_all = do_test,
.forks_child = 1,
};

When tst_brk() is called both in parent and child the counter would be incremented only once because the child is not waited for by the main test.

We can close this special case by changing the main test pid to wait for the children before it calls exit() in the tst_brk() but that may cause the main process to get stuck undefinitely if the child processes get stuck, so we would have to be careful.

Also from the very definition of the TBROK return status the test results would be incomplete at best, since TBROK really means "unrecoverable error happened during the test" which would mostly means that something as low level as filesystem got corrupted and there is no point in presenting the results in that case, so I guess that the best we could do in the case of TBROK is to print big message that says "things went horribly wrong!" or something similar.

All in all I would like to avoid applying patches to the test library before we finalize the release, since there is not much time for testing now.

--
Cyril Hrubis
[email protected]

Quoted 8 lines

Looking at the codebase we do have a few usages of tst_brk(TFAIL, "...") to exit the child process, which sort of works but it's incorrect. The tst_brk() always meant "unrecoverable failure have happened, exit the current process as fast as possible". Looking over our codebase most of the tst_brk(TFAIL, "...") should not actually cause the main test process to exit, these were only meant to exit the child and report the result in one call. It will for instance break the test with -i option on the first failure, which is incorrect.

Nice example, would you care to add that to docs?

Quoted 12 lines

So if we ever want to have a function to exit child process with a result we should implement tst_ret() that would be equivalent to tst_res() followed by exit(0).

It could be even implemented as:

#define tst_ret(ttype, fmt, ...) \
	do { \
		tst_res_(__FILE__, __LINE__, (ttype), (fmt), ##__VA_ARGS__); \
		exit(0); \
	} while (0)

Quoted 2 lines

This function has one big advantage, it increments the results counters before the child process exits.

If all call-sites switch to tst_ret(), we could add TFAIL to tst_brk compile time check.

Quoted 12 lines

So if we ever want to have a function to exit child process with a result we should implement tst_ret() that would be equivalent to tst_res() followed by exit(0).

It could be even implemented as:

#define tst_ret(ttype, fmt, ...) \
	do { \
		tst_res_(__FILE__, __LINE__, (ttype), (fmt), ##__VA_ARGS__); \
		exit(0); \
	} while (0)

Quoted 2 lines

This function has one big advantage, it increments the results counters before the child process exits.

If all call-sites switch to tst_ret(), we could add TFAIL to tst_brk compile time check.

Hi!

Quoted 1 line

1) Catch and report the TFAIL exit status of child process.

Looking at the codebase we do have a few usages of tst_brk(TFAIL, "...") to exit the child process, which sort of works but it's incorrect. The tst_brk() always meant "unrecoverable failure have happened, exit the current process as fast as possible". Looking over our codebase most of the tst_brk(TFAIL, "...") should not actually cause the main test process to exit, these were only meant to exit the child and report the result in one call. It will for instance break the test with -i option on the first failure, which is incorrect.

So if we ever want to have a function to exit child process with a result we should implement tst_ret() that would be equivalent to tst_res() followed by exit(0).

It could be even implemented as:

#define tst_ret(ttype, fmt, ...) \
	do { \
		tst_res_(__FILE__, __LINE__, (ttype), (fmt), ##__VA_ARGS__); \
		exit(0); \
	} while (0)

This function has one big advantage, it increments the results counters before the child process exits.

Actually one of the big points of the new test library was that the results counters are atomically increased, because passing the results in exit values is nightmare that cannot be done correclty.

Quoted 3 lines

2) Only update result counters in library process and main test process because the exit status of child can be reported by main test process.

Actually after I spend some time on it I think that the best solution is to update the results in the piece of shared memory as fast as possible, anything else is prone to various races and corner cases.

Quoted 4 lines

3) Print TCONF message and increase skipped when calling tst_brk(TCONF). Print TBROK message and increase broken when calling tst_brk(TBROK). Print TFAIL message and increase failed when calling tst_brk(TFAIL).
4) Remove duplicate update_results() in run_tcases_per_fs().

I've been thinking about this and the problem is more complex, and I'm even not sure that it's possible to write the library so that the counters are consistent at the time we exit the test if something unexpected happened and we called tst_brk().

Consider for instance this example:

#include "tst_test.h"

static void do_test(void)

{
         if (!SAFE_FORK())
                 tst_brk(TBROK, "child");
         tst_brk(TBROK, "parent");
}

static struct tst_test test = {
.test_all = do_test,
.forks_child = 1,
};

When tst_brk() is called both in parent and child the counter would be incremented only once because the child is not waited for by the main test.

We can close this special case by changing the main test pid to wait for the children before it calls exit() in the tst_brk() but that may cause the main process to get stuck undefinitely if the child processes get stuck, so we would have to be careful.

Also from the very definition of the TBROK return status the test results would be incomplete at best, since TBROK really means "unrecoverable error happened during the test" which would mostly means that something as low level as filesystem got corrupted and there is no point in presenting the results in that case, so I guess that the best we could do in the case of TBROK is to print big message that says "things went horribly wrong!" or something similar.

All in all I would like to avoid applying patches to the test library before we finalize the release, since there is not much time for testing now.

1) Both TBROK and TFAIL should be converted to TWARN in test cleanup.

2) Add a hint for the conversion.

Hi!

Quoted 1 line

1) Both TBROK and TFAIL should be converted to TWARN in test cleanup.

Do we actually produce TFAIL somewhere in the test cleanups?

Actually producing TPASS or TFAIL in test cleanup sounds wrong to me.

Quoted 1 line

2) Add a hint for the conversion.

I do not think that this very useful. Jan what do you think?