Skip to contentSkip to conversation

perf: Integrate logging facilities

RFD Namhyung Kim 2 participants 3 messages Mar 12, 2012 – Mar 13, 2012

lore
Share & tools
lore
#0 lore raw

Hi, all

This is my proposal for integrating various logging facilities used on perf tools. We now have a couple of ways to report something bad happened to user as follows:

* fprintf(stderr, ...)
* perror
* die/error/warning/usage
* pr_err/pr_warning/pr_info/pr_debug
* ui__error/ui__warning

Also we use dump_printf() when -D given. Some of them are aware of TUI (based on the value of use_browser), but other's are not. In addition, we should consider the case of python binding. There also is a patch proposed by Pekka Enberg which add a GTK+ GUI support on perf report. All of these make things complicated when someone - like me - who has no idea of this situation wants to leave a message for the user. And their meaning and expected behaivor is somewhat vague AFAICS.

So I suggest that we should select one method and deprecates other usages, make it flexible to deal with current complexity and possible future improvements. IMHO pr_err or ui__error (and its friends) can be chosen as the candidate. I prefer the pr_err() since it's a standard method of kernel logging and shorter to type :). And it supports more than 4 levels to differentiate its priory so that the developer can use appropriate one depending on the case. Now I think we can provide following semantics for each function:

* pr_err: Fatal error occurred. perf cannot continue to work anymore and exits immediately once user noticed - by pressing keys, clicking buttons (if GUI provided) or simply printing it on terminal.
* pr_warning: Serious error occurred. perf may or may not continue to work - if UI somehow provides a way to alter related options dynamically? User also will be imformed the message by pressing keys or clicking buttons. In case of stdio, the message will be followed by sleep(1).
* pr_info: Just a informative message and can be buried with other messages. UI will put this on the helpline. perf will keep doing its work.
* pr_debug: Messages to make debugging easy. It can be seen only -v option is given. UI will put this on the helpline too.

To implement this, I suggest using a table of function pointers to each method. By default it's connected to terminal printf-style functions. When setup_browser() is called, it'll be overridden to approriate ones. This way, we don't need to care about the timing when this function is called - since there's a time gap between use_browser is set (by config parser) and setup_browser() called, thus simply checking the variable can cause a nasty problem IMHO. Also python binding should have its own implementation - I have no idea on this :).

I think we should stop using fprintf/perror from now on and convert existing ones to generic ones. The generic code have to use above functions only, although UI specific code can use its own ones. The die, error, warning and usage are easy to convert to use generic functions since they're implemented by pointer already. And finally ui__error can remove terminal support(?).

This is a rough idea and there can be things I am missing. So I need to listen to your advices.

Thanks,
Namhyung

#1 lore raw
Quoted 2 lines · Namhyung Kim

This is a rough idea and there can be things I am missing. So I need to listen to your advices.

What Arnaldo proposed was having core, library code return error codes and not print anything to the user. Higher layers (commands only?) can take the return error code and pass it to perf_<subsystem>_strerror() to dump a user friendly string for the error. See

http://lkml.org/lkml/2012/2/13/302

David

#2 lore raw

Hi,

Quoted 11 lines · David Ahern
Quoted 2 lines · Namhyung Kim

This is a rough idea and there can be things I am missing. So I need to listen to your advices.

What Arnaldo proposed was having core, library code return error codes and not print anything to the user. Higher layers (commands only?) can take the return error code and pass it to perf_<subsystem>_strerror() to dump a user friendly string for the error. See

http://lkml.org/lkml/2012/2/13/302

It will work well for error cases, but how about warnings? I mean the case of something like handling new features on old kernels. I want give a (warning) message to user but let the code keep going instead of returning to the user. It seems not possible on above scenario, so should it be handled at all in higher layers? Couldn't the core code have that kind of flexiblity?

And current library code has lots of calls to pr_debug*, dump_printf as well as (something-or-)die. How can we handle this? For debugging functions, it cannot be moved out of the library for obvious reasons. For die, it can be converted to return a error code, but it might require adding non-trivial error handling path especially for deeply nested code IMHO.

Also, I think my proposal can still be applied for higher layer codes. It'd be better to standardize logging facilities at least for high level in order to be flexible for future changes.

Thanks,
Namhyung