Re: [PATCH] perf ui/tui: Show fatal error message only if exists

[Namhyung Kim <namhyung@kernel.org>]

On Tue, Jan 20, 2015 at 01:49:26PM +0100, Jiri Olsa wrote:
> On Tue, Jan 20, 2015 at 03:40:50PM +0900, Namhyung Kim wrote:
> > When perf exits with some error it shows the error message with
> > ui__error() or ui__warning() and then calls ui__exit() during
> > exit_browser().
> > 
> > On TUI, it then shows a window titled "Fatal Error" to inform user a
> > last message which might be related with this condition.  However it
> > sometimes contains no message and just annoyes users.
> > 
> > The usual case for this is running perf top as normal user.  (And
> > /proc/sys/kernel/perf_event_paranoid being 1).
> 
> cool, I couldn't recreate the 'no message' box but I recall seeing it
> 
> > 
> > Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> > ---
> >  tools/perf/ui/tui/helpline.c | 3 +++
> >  tools/perf/ui/tui/setup.c    | 3 ++-
> >  2 files changed, 5 insertions(+), 1 deletion(-)
> > 
> > diff --git a/tools/perf/ui/tui/helpline.c b/tools/perf/ui/tui/helpline.c
> > index 1c8b9afd5d6e..88f5143a5981 100644
> > --- a/tools/perf/ui/tui/helpline.c
> > +++ b/tools/perf/ui/tui/helpline.c
> > @@ -9,6 +9,7 @@
> >  #include "../libslang.h"
> >  
> >  char ui_helpline__last_msg[1024];
> > +bool tui_helpline__set;
> >  
> >  static void tui_helpline__pop(void)
> >  {
> > @@ -35,6 +36,8 @@ static int tui_helpline__show(const char *format, va_list ap)
> >  			sizeof(ui_helpline__last_msg) - backlog, format, ap);
> >  	backlog += ret;
> >  
> > +	tui_helpline__set = true;
> > +
> >  	if (ui_helpline__last_msg[backlog - 1] == '\n') {
> >  		ui_helpline__puts(ui_helpline__last_msg);
> >  		SLsmg_refresh();
> > diff --git a/tools/perf/ui/tui/setup.c b/tools/perf/ui/tui/setup.c
> > index 3c38f25b1695..b77e1d771363 100644
> > --- a/tools/perf/ui/tui/setup.c
> > +++ b/tools/perf/ui/tui/setup.c
> > @@ -17,6 +17,7 @@
> >  static volatile int ui__need_resize;
> >  
> >  extern struct perf_error_ops perf_tui_eops;
> > +extern bool tui_helpline__set;
> >  
> >  extern void hist_browser__init_hpp(void);
> >  
> > @@ -159,7 +160,7 @@ int ui__init(void)
> >  
> >  void ui__exit(bool wait_for_ok)
> >  {
> > -	if (wait_for_ok)
> > +	if (wait_for_ok && tui_helpline__set)
> >  		ui__question_window("Fatal Error",
> >  				    ui_helpline__last_msg,
> >  				    "Press any key...", 0);
> 
> looks like we display ui_helpline__last_msg also in ui/browsers/annotate.c

Right, I also saw it.

> 
> do we want to have some common function that unsets tui_helpline__set
> so we're sure we dont get the message twice?

I doubt that it's really needed..  The annotation code does not
propagate an error so the wait_for_ok arg in ui__exit() will be always
false.  In addition, I think it's better to use ui__error() or
ui__warning() directly at the point when something bad happened and we
need to inform user.

Thanks,
Namhyung