Re: [PATCH] perf: fix perf test case 14 result reporting

[Thomas-Mich Richter]

On 06/01/2017 11:04 PM, Jiri Olsa wrote:
> On Thu, Jun 01, 2017 at 10:20:38AM -0300, Arnaldo Carvalho de Melo wrote:
>> Em Thu, Jun 01, 2017 at 02:34:41PM +0200, Thomas Richter escreveu:
>>> Command perf test -v 14 (Setup struct perf_event_attr test)
>>> always reports success even if the test case fails.
>>> It works correctly if you also specify -F (for don't fork).
>>
>> Thanks for working on this, adding Jiri Olsa, that wrote this test
>> harness, so that he can check and provide his Acked-by or Reviewed-by,
>> Jiri?
>>
>> - Arnaldo
>>  
>>>    root@s35lp76 perf]# ./perf test -v 14
>>>    14: Setup struct perf_event_attr               :
>>>    --- start ---
>>>    running './tests/attr/test-record-no-delay'
>>>    [ perf record: Woken up 1 times to write data ]
>>>    [ perf record: Captured and wrote 0.002 MB /tmp/tmp4E1h7R/perf.data
>>>      (1 samples) ]
>>>    expected task=0, got 1
>>>    expected precise_ip=0, got 3
>>>    expected wakeup_events=1, got 0
>>>    FAILED './tests/attr/test-record-no-delay' - match failure
>>>    test child finished with 0
>>>    ---- end ----
>>>    Setup struct perf_event_attr: Ok
>>>
>>> The reason for the wrong error reporting is the return value of the
>>> system() library call. It is called in run_dir() file tests/attr.c
>>> and returns the exit status, in above case 0xff00.
>>> This value is given as parameter to the exit() function which
>>> can only handle values 0-0xff.
>>> The child process terminates with exit value of 0 and the parent
>>> does not detect any error.
>>>
>>> This patch corrects the error reporting and prints the
>>> correct test result.
>>>
>>> Signed-off-by: Thomas Richter <tmricht@xxxxxxxxxxxxxxxxxx>
>>> Reviewed-by: Hendrik Brueckner <brueckner@xxxxxxxxxxxxxxxxxx>
>>> ---
>>>  tools/perf/tests/attr.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/tools/perf/tests/attr.c b/tools/perf/tests/attr.c
>>> index 88dc51f..131b510 100644
>>> --- a/tools/perf/tests/attr.c
>>> +++ b/tools/perf/tests/attr.c
>>> @@ -150,7 +150,7 @@ static int run_dir(const char *d, const char *perf)
>>>  	snprintf(cmd, 3*PATH_MAX, PYTHON " %s/attr.py -d %s/attr/ -p %s %.*s",
>>>  		 d, d, perf, vcnt, v);
>>>  
>>> -	return system(cmd);
>>> +	return system(cmd) ? TEST_FAIL : TEST_OK;
>>>  }
>>>  
>>>  int test__attr(int subtest __maybe_unused)
>>> -- 
>>> 2.9.3
> 
> seems ok, however "perf test attr" is broken ATM, since it wasn't updated
> for some time as it showed false 'Ok'
> 
> I started fixing it some time ago, but got distracted, if you are
> interested, you're welcome to pick up from my branch ;-)
> 
>   git://git.kernel.org/pub/scm/linux/kernel/git/jolsa/perf.git
>   perf/attr_test
> 
> thanks,
> jirka
> 

I have started work on perf tool very recently to get s390 support
working and up to date.

I downloaded your branch and discovered you have already fixed
another issue I run into this week. For example 
commit 070b9644981e ("perf tests attr: Do not store failed events")

I can certainly help you to get this test case operational again.
Maybe you need to pull some of your patches out of your backlog
and submit them the kernel to get to a common base to work on.

I suggest we should move the discussion to the linux-perf-users
mailing list.

Your thoughts?

-- 
Thomas Richter, Dept 3303, IBM LTC Boeblingen Germany
--
Vorsitzende des Aufsichtsrats: Martina Koederitz 
GeschÃftsfÃhrung: Dirk Wittkopp
Sitz der Gesellschaft: BÃblingen / Registergericht: Amtsgericht Stuttgart, HRB 243294