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

[Arnaldo Carvalho de Melo]

Em Fri, Jun 02, 2017 at 11:09:36AM -0300, Arnaldo Carvalho de Melo escreveu:
> Em Fri, Jun 02, 2017 at 11:58:46AM +0200, Thomas-Mich Richter escreveu:
> > 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?
> 
> If you have specific patches in Jiri's branch that you think are good to
> go, just point me to them and I'll cherry-pick them.
> 
> I'm looking now at the one you pointed out above (070b9644981e).

Just looked, but the cset comment should state what is the problem and
how it is solved, right now it has just a one line summary :-\

- Arnaldo