[LTP] [PATCH 1/2] lib: Introduce tst_strttype()

[LTP] [PATCH 1/2] lib: Introduce tst_strttype()

[Xiao Yang]

Rename strttype() to tst_strttype() and introduce the function
for old and new library.

Signed-off-by: Xiao Yang <yangx.jy@cn.fujitsu.com>
---
 include/old/test.h | 2 +-
 include/tst_test.h | 3 +++
 lib/tst_res.c      | 4 ++--
 3 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/include/old/test.h b/include/old/test.h
index 0738237..3dce395 100644
--- a/include/old/test.h
+++ b/include/old/test.h
@@ -113,7 +113,7 @@ void tst_parse_opts(int argc, char *argv[], const option_t *user_optarg,
                     void (*user_help)(void));
 
 /* lib/tst_res.c */
-const char *strttype(int ttype);
+const char *tst_strttype(int ttype);
 
 void tst_resm_(const char *file, const int lineno, int ttype,
 	const char *arg_fmt, ...)
diff --git a/include/tst_test.h b/include/tst_test.h
index 2ebf746..5982638 100644
--- a/include/tst_test.h
+++ b/include/tst_test.h
@@ -222,6 +222,9 @@ extern void *TST_RET_PTR;
  */
 const char *tst_strerrno(int err);
 const char *tst_strsig(int sig);
+
+// Function to convert ttype to its name.
+const char *tst_strttype(int ttype);
 /*
  * Returns string describing status as returned by wait().
  *
diff --git a/lib/tst_res.c b/lib/tst_res.c
index c35f41b..8c69b75 100644
--- a/lib/tst_res.c
+++ b/lib/tst_res.c
@@ -139,7 +139,7 @@ struct pair {
 	return pair_arr[idx].name;                            \
 } while (0)
 
-const char *strttype(int ttype)
+const char *tst_strttype(int ttype)
 {
 	static const struct pair ttype_pairs[] = {
 		PAIR(TPASS)
@@ -296,7 +296,7 @@ static void tst_print(const char *tcid, int tnum, int ttype, const char *tmesg)
 	/*
 	 * Build the result line and print it.
 	 */
-	type = strttype(ttype);
+	type = tst_strttype(ttype);
 
 	if (T_mode == VERBOSE) {
 		size += snprintf(message + size, sizeof(message) - size,
-- 
1.8.3.1

[LTP] [PATCH 2/2] lib/tst_test.c: Restrict that tst_brk() only works with TBROK/TCONF

[Xiao Yang]

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 <yangx.jy@cn.fujitsu.com>
---
 lib/tst_test.c | 28 ++++++++++++++++++++++++++--
 1 file changed, 26 insertions(+), 2 deletions(-)

diff --git a/lib/tst_test.c b/lib/tst_test.c
index 661fbbf..c8d8eff 100644
--- a/lib/tst_test.c
+++ b/lib/tst_test.c
@@ -55,6 +55,7 @@ struct results {
 	int skipped;
 	int failed;
 	int warnings;
+	int broken;
 	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));
 }
@@ -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);
 }
@@ -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

[LTP] [PATCH 2/2] lib/tst_test.c: Restrict that tst_brk() only works with TBROK/TCONF

[Jan Stancek]

----- Original Message -----
> 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 <yangx.jy@cn.fujitsu.com>
> ---
>  lib/tst_test.c | 28 ++++++++++++++++++++++++++--
>  1 file changed, 26 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/tst_test.c b/lib/tst_test.c
> index 661fbbf..c8d8eff 100644
> --- a/lib/tst_test.c
> +++ b/lib/tst_test.c
> @@ -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

>  	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));
>  }
> @@ -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);
>  }
> @@ -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
> 
> 
> 
> 

[LTP] [PATCH 2/2] lib/tst_test.c: Restrict that tst_brk() only works with TBROK/TCONF

[Xiao Yang]

On 2018/11/09 1:53, Jan Stancek wrote:
> ----- Original Message -----
>> 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<yangx.jy@cn.fujitsu.com>
>> ---
>>   lib/tst_test.c | 28 ++++++++++++++++++++++++++--
>>   1 file changed, 26 insertions(+), 2 deletions(-)
>>
>> diff --git a/lib/tst_test.c b/lib/tst_test.c
>> index 661fbbf..c8d8eff 100644
>> --- a/lib/tst_test.c
>> +++ b/lib/tst_test.c
>> @@ -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().
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

> Regards,
> Jan
>
>>   	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));
>>   }
>> @@ -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);
>>   }
>> @@ -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
>>
>>
>>
>>
>
> .
>

[LTP] [PATCH 2/2] lib/tst_test.c: Restrict that tst_brk() only works with TBROK/TCONF

[Xiao Yang]

于 2018/11/09 10:46, Xiao Yang 写道:
> On 2018/11/09 1:53, Jan Stancek wrote:
>> ----- Original Message -----
>>> 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<yangx.jy@cn.fujitsu.com>
>>> ---
>>>   lib/tst_test.c | 28 ++++++++++++++++++++++++++--
>>>   1 file changed, 26 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/lib/tst_test.c b/lib/tst_test.c
>>> index 661fbbf..c8d8eff 100644
>>> --- a/lib/tst_test.c
>>> +++ b/lib/tst_test.c
>>> @@ -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().
> 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().
Hi,

Sorry, The correct number is #408 :-(

Best Regards,
Xiao Yang
> 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
>
>> Regards,
>> Jan
>>
>>>       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));
>>>   }
>>> @@ -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);
>>>   }
>>> @@ -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
>>>
>>>
>>>
>>>
>>
>> .
>>
>
>
>
>

[LTP] [PATCH 2/2] lib/tst_test.c: Restrict that tst_brk() only works with TBROK/TCONF

[Jan Stancek]

----- Original Message -----
> On 2018/11/09 1:53, Jan Stancek wrote:
> > ----- Original Message -----
> >> 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<yangx.jy@cn.fujitsu.com>
> >> ---
> >>   lib/tst_test.c | 28 ++++++++++++++++++++++++++--
> >>   1 file changed, 26 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/lib/tst_test.c b/lib/tst_test.c
> >> index 661fbbf..c8d8eff 100644
> >> --- a/lib/tst_test.c
> >> +++ b/lib/tst_test.c
> >> @@ -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().
> 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?

> 
> 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

[LTP] [PATCH 2/2] lib/tst_test.c: Restrict that tst_brk() only works with TBROK/TCONF

[Xiao Yang]

On 2018/11/09 15:54, Jan Stancek wrote:
>
> ----- Original Message -----
>> On 2018/11/09 1:53, Jan Stancek wrote:
>>> ----- Original Message -----
>>>> 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<yangx.jy@cn.fujitsu.com>
>>>> ---
>>>>    lib/tst_test.c | 28 ++++++++++++++++++++++++++--
>>>>    1 file changed, 26 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/lib/tst_test.c b/lib/tst_test.c
>>>> index 661fbbf..c8d8eff 100644
>>>> --- a/lib/tst_test.c
>>>> +++ b/lib/tst_test.c
>>>> @@ -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().
>> 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?
>
>> 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?
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

> Regards,
> Jan
>
>
>
> .
>

[LTP] [PATCH 2/2] lib/tst_test.c: Restrict that tst_brk() only works with TBROK/TCONF

[Jan Stancek]

----- Original Message -----
> On 2018/11/09 15:54, Jan Stancek wrote:
> >
> > ----- Original Message -----
> >> On 2018/11/09 1:53, Jan Stancek wrote:
> >>> ----- Original Message -----
> >>>> 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<yangx.jy@cn.fujitsu.com>
> >>>> ---
> >>>>    lib/tst_test.c | 28 ++++++++++++++++++++++++++--
> >>>>    1 file changed, 26 insertions(+), 2 deletions(-)
> >>>>
> >>>> diff --git a/lib/tst_test.c b/lib/tst_test.c
> >>>> index 661fbbf..c8d8eff 100644
> >>>> --- a/lib/tst_test.c
> >>>> +++ b/lib/tst_test.c
> >>>> @@ -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().
> >> 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?
> >
> >> 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?
> 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
> -----------------------------------------------

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

> 
> Best Reagrds,
> Xiao Yang
> 
> > Regards,
> > Jan
> >
> >
> >
> > .
> >
> 
> 
> 
> 

[LTP] [PATCH 2/2] lib/tst_test.c: Restrict that tst_brk() only works with TBROK/TCONF

[Xiao Yang]

On 2018/11/10 1:52, Jan Stancek wrote:
>
> ----- Original Message -----
>> On 2018/11/09 15:54, Jan Stancek wrote:
>>> ----- Original Message -----
>>>> On 2018/11/09 1:53, Jan Stancek wrote:
>>>>> ----- Original Message -----
>>>>>> 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<yangx.jy@cn.fujitsu.com>
>>>>>> ---
>>>>>>     lib/tst_test.c | 28 ++++++++++++++++++++++++++--
>>>>>>     1 file changed, 26 insertions(+), 2 deletions(-)
>>>>>>
>>>>>> diff --git a/lib/tst_test.c b/lib/tst_test.c
>>>>>> index 661fbbf..c8d8eff 100644
>>>>>> --- a/lib/tst_test.c
>>>>>> +++ b/lib/tst_test.c
>>>>>> @@ -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().
>>>> 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?
>>>
>>>> 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?
>> 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
>> -----------------------------------------------
> 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.
Hi Jan,

Sorry, i gave a mismatched example.

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

Best Regards,
Xiao Yang
> Regards,
> Jan
>
>> Best Reagrds,
>> Xiao Yang
>>
>>> Regards,
>>> Jan
>>>
>>>
>>>
>>> .
>>>
>>
>>
>>
>
> .
>

[LTP] [PATCH 2/2] lib/tst_test.c: Restrict that tst_brk() only works with TBROK/TCONF

[Cyril Hrubis]

Hi!
> 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.

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH 2/2] lib/tst_test.c: Restrict that tst_brk() only works with TBROK/TCONF

[Xiao Yang]

On 2018/12/11 23:17, Cyril Hrubis wrote:
> Hi!
>> 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.
Hi Cyril,

Yes, I am going to send v3.
I will pass TBROK to results->broken and  TFAIL to results->failed in my 
v3 patch.
> 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?

Best Regards,
Xiao Yang

[LTP] [PATCH 2/2] lib/tst_test.c: Restrict that tst_brk() only works with TBROK/TCONF

[Cyril Hrubis]

Hi!
> > 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?

That is just to be backward compatible with the conventions we used to
have before we allowed for SAFE_MACROS() to be used in cleanups. Before
that most of the tests were coded so that failure in cleanup produced a
warning.

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH v3 1/3] lib: Introduce tst_strttype()

[Xiao Yang]

Rename strttype() to tst_strttype() and introduce the function
for old and new library.

Signed-off-by: Xiao Yang <yangx.jy@cn.fujitsu.com>
---
 include/old/test.h | 2 +-
 include/tst_test.h | 4 ++++
 lib/tst_res.c      | 4 ++--
 3 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/include/old/test.h b/include/old/test.h
index 0738237..3dce395 100644
--- a/include/old/test.h
+++ b/include/old/test.h
@@ -113,7 +113,7 @@ void tst_parse_opts(int argc, char *argv[], const option_t *user_optarg,
                     void (*user_help)(void));
 
 /* lib/tst_res.c */
-const char *strttype(int ttype);
+const char *tst_strttype(int ttype);
 
 void tst_resm_(const char *file, const int lineno, int ttype,
 	const char *arg_fmt, ...)
diff --git a/include/tst_test.h b/include/tst_test.h
index 2ebf746..f3d1546 100644
--- a/include/tst_test.h
+++ b/include/tst_test.h
@@ -222,6 +222,10 @@ extern void *TST_RET_PTR;
  */
 const char *tst_strerrno(int err);
 const char *tst_strsig(int sig);
+
+// Function to convert ttype to its name.
+const char *tst_strttype(int ttype);
+
 /*
  * Returns string describing status as returned by wait().
  *
diff --git a/lib/tst_res.c b/lib/tst_res.c
index c35f41b..8c69b75 100644
--- a/lib/tst_res.c
+++ b/lib/tst_res.c
@@ -139,7 +139,7 @@ struct pair {
 	return pair_arr[idx].name;                            \
 } while (0)
 
-const char *strttype(int ttype)
+const char *tst_strttype(int ttype)
 {
 	static const struct pair ttype_pairs[] = {
 		PAIR(TPASS)
@@ -296,7 +296,7 @@ static void tst_print(const char *tcid, int tnum, int ttype, const char *tmesg)
 	/*
 	 * Build the result line and print it.
 	 */
-	type = strttype(ttype);
+	type = tst_strttype(ttype);
 
 	if (T_mode == VERBOSE) {
 		size += snprintf(message + size, sizeof(message) - size,
-- 
1.8.3.1

[LTP] [PATCH v3 2/3] lib/tst_test.c: Update result counters when calling tst_brk()

[Xiao Yang]

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().

Fixes: #408

Signed-off-by: Xiao Yang <yangx.jy@cn.fujitsu.com>
---
 lib/tst_test.c | 24 ++++++++++++++++--------
 1 file changed, 16 insertions(+), 8 deletions(-)

diff --git a/lib/tst_test.c b/lib/tst_test.c
index 661fbbf..e46ab8e 100644
--- a/lib/tst_test.c
+++ b/lib/tst_test.c
@@ -55,6 +55,7 @@ struct results {
 	int skipped;
 	int failed;
 	int warnings;
+	int broken;
 	unsigned int timeout;
 };
 
@@ -177,6 +178,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 +309,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_RESULT(ttype));
 		do_test_cleanup();
+	}
 
-	if (getpid() == lib_pid)
+	if (getpid() == lib_pid) {
+		update_results(TTYPE_RESULT(ttype));
 		do_exit(TTYPE_RESULT(ttype));
+	}
 
 	exit(TTYPE_RESULT(ttype));
 }
@@ -350,6 +358,7 @@ static void check_child_status(pid_t pid, int status)
 	switch (ret) {
 	case TPASS:
 	break;
+	case TFAIL:
 	case TBROK:
 	case TCONF:
 		tst_brk(ret, "Reported by child (%i)", pid);
@@ -591,6 +600,7 @@ static void do_exit(int ret)
 		printf("failed   %d\n", results->failed);
 		printf("skipped  %d\n", results->skipped);
 		printf("warnings %d\n", results->warnings);
+		printf("broken   %d\n", results->broken);
 
 		if (results->passed && ret == TCONF)
 			ret = 0;
@@ -603,6 +613,9 @@ static void do_exit(int ret)
 
 		if (results->warnings)
 			ret |= TWARN;
+
+		if (results->broken)
+			ret |= TBROK;
 	}
 
 	do_cleanup();
@@ -1155,12 +1168,7 @@ static int run_tcases_per_fs(void)
 			mntpoint_mounted = 0;
 		}
 
-		if (ret == TCONF) {
-			update_results(ret);
-			continue;
-		}
-
-		if (ret == 0)
+		if (ret == TCONF || ret == 0)
 			continue;
 
 		do_exit(ret);
-- 
1.8.3.1

[LTP] [PATCH v3 2/3] lib/tst_test.c: Update result counters when calling tst_brk()

[Cyril Hrubis]

Hi!
> 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.

> 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.

> 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
chrubis@suse.cz

[LTP] [PATCH v3 2/3] lib/tst_test.c: Update result counters when calling tst_brk()

[Jan Stancek]

----- Original Message -----
> Hi!
> > 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.

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

> 
> 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.

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

> 
> 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.
> 
> > 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.

I was thinking this too.

If your parent process happens to wait for the child itself,
then library will never get to see retcode.

Regards,
Jan

> 
> > 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
> chrubis@suse.cz
> 

[LTP] [PATCH v3 2/3] lib/tst_test.c: Update result counters when calling tst_brk()

[Cyril Hrubis]

Hi!
> > 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?

Good idea.

> > 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.
> 
> If all call-sites switch to tst_ret(), we could add TFAIL to tst_brk
> compile time check.

Actually I've started to work on that and all call sites can either be
converted to tst_ret(TFAIL, ...) or tst_brk(TBROK, ...). So it really
looks like we should go this way.

Also I guess that this may be even safe enough to go in before the
release. I will try to finish and post it tomorrow.

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH v3 2/3] lib/tst_test.c: Update result counters when calling tst_brk()

[Cyril Hrubis]

Hi!
> > 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.
> 
> If all call-sites switch to tst_ret(), we could add TFAIL to tst_brk
> compile time check.

And it's even more complicated than this.

The problem is that if we add tst_ret() that calls exit(0) it will have
slightly different semantics for the main test process and for the
children.

The subtle difference is that we run the loop that executes the tests in
the case of the -i parameter or in the case that we defined tcnt is in
the main test process. If we call tst_ret() there it will exit the test
before we manage to run rest of the iterations and I would like to avoid
adding an API that is slightly confusing easy to misuse.

I will try to rethink this, however it looks like there is no silver
bullet to solve the problem. And it's certainly late for the release.

Also I've opened an issue for the problem in order not to forget:

https://github.com/linux-test-project/ltp/issues/462

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH v3 2/3] lib/tst_test.c: Update result counters when calling tst_brk()

[Xiao Yang]

On 2019/01/07 23:06, Cyril Hrubis wrote:
> Hi!
>> 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.
Hi Cyril,

Detailed explanation, and i got it.

> 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.
Agreed.  All of tst_brk(TFAIL, ...) can be converted to tst_ret(TFAIL, ...) or
tst_brk(TBROK, ...) in this way and then add TFAIL to tst_brk compile time check
as Jan replied, so that only TCONF and TBROK can be passed into tst_brk().

>> 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.
...

>> 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.
Sorry, my patch is too rough becasue some suitations are not taken into account.
For tst_brk(TCONF), do you mean to replace the current solution using wait() in
check_child_status() with your suggested shared memory?
For tst_brk(TBROK), do you mean to just print big message instead of updating
test results?

> 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.
Agreed, drop these patches during the upcoming release.  We still need to do
future investigation and testing.

Best Regards,
Xiao Yang

[LTP] [PATCH v3 3/3] lib/tst_test.c: Convert TFAIL to TWARN in test cleanup

[Xiao Yang]

1) Both TBROK and TFAIL should be converted to TWARN in test cleanup.
2) Add a hint for the conversion.
2) Update test-writing-guidelines.

Note:
From test-writing-guidelines, Converting TBROK to TWARN seems
intentional design, so i try to convert TFAIL as well.

Signed-off-by: Xiao Yang <yangx.jy@cn.fujitsu.com>
---
 doc/test-writing-guidelines.txt | 2 +-
 lib/tst_test.c                  | 4 +++-
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/doc/test-writing-guidelines.txt b/doc/test-writing-guidelines.txt
index 4b40760..3e0fa1a 100644
--- a/doc/test-writing-guidelines.txt
+++ b/doc/test-writing-guidelines.txt
@@ -393,7 +393,7 @@ IMPORTANT: 'SAFE_MACROS()' used in cleanup *do not* exit the test. Failure
 	   intentional as we want to execute as much 'cleanup()' as possible.
 
 WARNING: Calling tst_brk() in test 'cleanup()' does not exit the test as well
-         and 'TBROK' is converted to 'TWARN'.
+         and 'TBROK/TFAIL' is converted to 'TWARN'.
 
 NOTE: Creation and removal of the test temporary directory is handled in
       the test library and the directory is removed recursively. Therefore
diff --git a/lib/tst_test.c b/lib/tst_test.c
index e46ab8e..c5826d0 100644
--- a/lib/tst_test.c
+++ b/lib/tst_test.c
@@ -279,7 +279,9 @@ static void (*tst_brk_handler)(const char *file, const int lineno, int ttype,
 static void tst_cvres(const char *file, const int lineno, int ttype,
 		      const char *fmt, va_list va)
 {
-	if (TTYPE_RESULT(ttype) == TBROK) {
+	if (TTYPE_RESULT(ttype) == TBROK || TTYPE_RESULT(ttype) == TFAIL) {
+		tst_res(TINFO, "Convert ttype %s to TWARN in test cleanup",
+			tst_strttype(ttype));
 		ttype &= ~TTYPE_MASK;
 		ttype |= TWARN;
 	}
-- 
1.8.3.1

[LTP] [PATCH v3 3/3] lib/tst_test.c: Convert TFAIL to TWARN in test cleanup

[Cyril Hrubis]

Hi!
> 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.

> 2) Add a hint for the conversion.

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

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH v3 3/3] lib/tst_test.c: Convert TFAIL to TWARN in test cleanup

[Jan Stancek]

----- Original Message -----
> Hi!
> > 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.
> 
> > 2) Add a hint for the conversion.
> 
> I do not think that this very useful. Jan what do you think?

I'd rather not add more conversions.

Actually if we introduce results->broken counter as 2/3 is proposing,
then maybe we can drop current conversion too?

Regards,
Jan

[LTP] [PATCH v2 1/2] lib: Introduce tst_strttype()

[Xiao Yang]

Rename strttype() to tst_strttype() and introduce the function
for old and new library.

Note:
It is based on the following patch:
http://lists.linux.it/pipermail/ltp/2018-November/009923.html

Signed-off-by: Xiao Yang <yangx.jy@cn.fujitsu.com>
---
 include/old/test.h | 2 +-
 include/tst_test.h | 4 ++++
 lib/tst_res.c      | 4 ++--
 3 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/include/old/test.h b/include/old/test.h
index 0738237..3dce395 100644
--- a/include/old/test.h
+++ b/include/old/test.h
@@ -113,7 +113,7 @@ void tst_parse_opts(int argc, char *argv[], const option_t *user_optarg,
                     void (*user_help)(void));
 
 /* lib/tst_res.c */
-const char *strttype(int ttype);
+const char *tst_strttype(int ttype);
 
 void tst_resm_(const char *file, const int lineno, int ttype,
 	const char *arg_fmt, ...)
diff --git a/include/tst_test.h b/include/tst_test.h
index 8b7204a..b0f079a 100644
--- a/include/tst_test.h
+++ b/include/tst_test.h
@@ -225,6 +225,10 @@ extern void *TST_RET_PTR;
  */
 const char *tst_strerrno(int err);
 const char *tst_strsig(int sig);
+
+// Function to convert ttype to its name.
+const char *tst_strttype(int ttype);
+
 /*
  * Returns string describing status as returned by wait().
  *
diff --git a/lib/tst_res.c b/lib/tst_res.c
index c35f41b..8c69b75 100644
--- a/lib/tst_res.c
+++ b/lib/tst_res.c
@@ -139,7 +139,7 @@ struct pair {
 	return pair_arr[idx].name;                            \
 } while (0)
 
-const char *strttype(int ttype)
+const char *tst_strttype(int ttype)
 {
 	static const struct pair ttype_pairs[] = {
 		PAIR(TPASS)
@@ -296,7 +296,7 @@ static void tst_print(const char *tcid, int tnum, int ttype, const char *tmesg)
 	/*
 	 * Build the result line and print it.
 	 */
-	type = strttype(ttype);
+	type = tst_strttype(ttype);
 
 	if (T_mode == VERBOSE) {
 		size += snprintf(message + size, sizeof(message) - size,
-- 
1.8.3.1

[LTP] [PATCH v2 2/2] lib/tst_test.c: Update result counters when calling tst_brk()

[Xiao Yang]

1) Only update result counters in library process and main test
   process because the exit status of child can be reported by
   main test process.
2) Increase the skipped counter when calling tst_brk(TCONF).
3) Increase the warnings counter when calling tst_brk(TBROK/TFAIL)
   in test cleanup(), other than that increase the failed counter
   when calling tst_brk(TBROK/TFAIL).
4) Remove duplicate update_results() in run_tcases_per_fs().

Fix: #408

Note:
It is based on the following patch:
http://lists.linux.it/pipermail/ltp/2018-November/009923.html

Signed-off-by: Xiao Yang <yangx.jy@cn.fujitsu.com>
---
 lib/tst_test.c | 20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/lib/tst_test.c b/lib/tst_test.c
index 661fbbf..b98be0d 100644
--- a/lib/tst_test.c
+++ b/lib/tst_test.c
@@ -174,6 +174,7 @@ static void update_results(int ttype)
 	case TWARN:
 		tst_atomic_inc(&results->warnings);
 	break;
+	case TBROK:
 	case TFAIL:
 		tst_atomic_inc(&results->failed);
 	break;
@@ -275,7 +276,9 @@ static void (*tst_brk_handler)(const char *file, const int lineno, int ttype,
 static void tst_cvres(const char *file, const int lineno, int ttype,
 		      const char *fmt, va_list va)
 {
-	if (TTYPE_RESULT(ttype) == TBROK) {
+	if (TTYPE_RESULT(ttype) == TBROK || TTYPE_RESULT(ttype) == TFAIL) {
+		tst_res(TINFO, "Convert ttype %s to TWARN in test cleanup",
+			tst_strttype(ttype));
 		ttype &= ~TTYPE_MASK;
 		ttype |= TWARN;
 	}
@@ -305,11 +308,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_RESULT(ttype));
 		do_test_cleanup();
+	}
 
-	if (getpid() == lib_pid)
+	if (getpid() == lib_pid) {
+		update_results(TTYPE_RESULT(ttype));
 		do_exit(TTYPE_RESULT(ttype));
+	}
 
 	exit(TTYPE_RESULT(ttype));
 }
@@ -1155,12 +1162,7 @@ static int run_tcases_per_fs(void)
 			mntpoint_mounted = 0;
 		}
 
-		if (ret == TCONF) {
-			update_results(ret);
-			continue;
-		}
-
-		if (ret == 0)
+		if (ret == TCONF || ret == 0)
 			continue;
 
 		do_exit(ret);
-- 
1.8.3.1