Re: [PATCH v10 2/3] perf config: Reimplement perf_config() introducing new perf_config_init() and perf_config_finish()
Hi, Arnaldo :) On 06/23/2016 08:56 PM, Arnaldo Carvalho de Melo wrote: Em Thu, Jun 23, 2016 at 05:55:18PM +0900, Taeung Song escreveu: Many sub-commands use perf_config() but everytime perf_config() is called, perf_config() always read config files. (i.e. user config '~/.perfconfig' and system config '$(sysconfdir)/perfconfig') But it is better to use the config set that already contains all config key-value pairs to avoid this repetitive work reading the config files in perf_config(). (the config set mean a static variable 'config_set') In other words, if new perf_config_init() is called, only first time 'config_set' is initialized collecting all configs from the config files. And then we could use new perf_config() like old perf_config(). When a sub-command finished, free the config set by perf_config_finish() at run_builtin(). If we do, 'config_set' can be reused wherever perf_config() is called and a feature of old perf_config() is the same as new perf_config() work without the repetitive work that read the config files. In summary, in order to use features about configuration, we can call the functions at perf.c and other source files as below. # initialize a config set perf_config_init() # configure actual variables from a config set perf_config() # eliminate allocated config set perf_config_finish() # destroy existing config set and initialize a new config set. perf_config_refresh() Cc: Namhyung Kim Cc: Jiri Olsa Cc: Wang Nan Cc: Peter Zijlstra Cc: Ingo Molnar Cc: Masami Hiramatsu Cc: Alexander Shishkin Signed-off-by: Taeung Song --- tools/perf/builtin-config.c | 4 ++ tools/perf/perf.c | 2 + tools/perf/util/config.c| 92 +++-- tools/perf/util/config.h| 29 ++ 4 files changed, 82 insertions(+), 45 deletions(-) diff --git a/tools/perf/builtin-config.c b/tools/perf/builtin-config.c index fe1b77f..cfd1036 100644 --- a/tools/perf/builtin-config.c +++ b/tools/perf/builtin-config.c @@ -80,6 +80,10 @@ int cmd_config(int argc, const char **argv, const char *prefix __maybe_unused) else if (use_user_config) config_exclusive_filename = user_config; + /* +* At only 'config' sub-command, individually use the config set +* because of reinitializing with options config file location. +*/ set = perf_config_set__new(); if (!set) { ret = -1; diff --git a/tools/perf/perf.c b/tools/perf/perf.c index 66772da..280967e 100644 --- a/tools/perf/perf.c +++ b/tools/perf/perf.c @@ -355,6 +355,7 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv) perf_env__set_cmdline(&perf_env, argc, argv); status = p->fn(argc, argv, prefix); + perf_config_finish(); exit_browser(status); perf_env__exit(&perf_env); bpf__clear(); @@ -522,6 +523,7 @@ int main(int argc, const char **argv) srandom(time(NULL)); + perf_config_init(); perf_config(perf_default_config, NULL); set_buildid_dir(NULL); diff --git a/tools/perf/util/config.c b/tools/perf/util/config.c index d15c592..a16f95d 100644 --- a/tools/perf/util/config.c +++ b/tools/perf/util/config.c @@ -26,6 +26,7 @@ static FILE *config_file; static const char *config_file_name; static int config_linenr; static int config_file_eof; +static struct perf_config_set *config_set; const char *config_exclusive_filename; @@ -478,51 +479,6 @@ static int perf_config_global(void) return !perf_env_bool("PERF_CONFIG_NOGLOBAL", 0); } -int perf_config(config_fn_t fn, void *data) -{ - int ret = -1; - const char *home = NULL; - - /* Setting $PERF_CONFIG makes perf read _only_ the given config file. */ - if (config_exclusive_filename) - return perf_config_from_file(fn, config_exclusive_filename, data); - if (perf_config_system() && !access(perf_etc_perfconfig(), R_OK)) { - if (perf_config_from_file(fn, perf_etc_perfconfig(), data) < 0) - goto out; - } - - home = getenv("HOME"); - if (perf_config_global() && home) { - char *user_config = strdup(mkpath("%s/.perfconfig", home)); - struct stat st; - - if (user_config == NULL) { - warning("Not enough memory to process %s/.perfconfig, " - "ignoring it.", home); - goto out; - } - - if (stat(user_config, &st) < 0) - goto out_free; - - if (st.st_uid && (st.st_uid != geteuid())) { - warning("File %s not owned by current user or root, " -
[PATCH v10 1/3] perf config: Bring declarations about config from util/cache.h to util/config.h
Lately util/config.h has been added but util/cache.h has declarations of functions and a global variable for config features. To manage codes about configuration at one spot, move them to util/config.h and let source files that need config features include config.h And if the source files that included previous cache.h need only config.h, remove including cache.h. Cc: Alexander Shishkin Cc: Jiri Olsa Cc: Masami Hiramatsu Cc: Namhyung Kim Cc: Wang Nan Signed-off-by: Taeung Song --- tools/perf/builtin-help.c | 2 +- tools/perf/builtin-kmem.c | 2 +- tools/perf/builtin-record.c| 1 + tools/perf/builtin-report.c| 2 +- tools/perf/builtin-top.c | 2 +- tools/perf/perf.c | 2 +- tools/perf/ui/browser.c| 2 +- tools/perf/ui/browsers/annotate.c | 1 + tools/perf/util/alias.c| 1 + tools/perf/util/cache.h| 11 --- tools/perf/util/color.c| 1 + tools/perf/util/config.h | 11 +++ tools/perf/util/help-unknown-cmd.c | 1 + tools/perf/util/intel-pt.c | 1 + tools/perf/util/llvm-utils.c | 1 + 15 files changed, 24 insertions(+), 17 deletions(-) diff --git a/tools/perf/builtin-help.c b/tools/perf/builtin-help.c index f9830c9..268ab73 100644 --- a/tools/perf/builtin-help.c +++ b/tools/perf/builtin-help.c @@ -4,7 +4,7 @@ * Builtin help command */ #include "perf.h" -#include "util/cache.h" +#include "util/config.h" #include "builtin.h" #include #include "common-cmds.h" diff --git a/tools/perf/builtin-kmem.c b/tools/perf/builtin-kmem.c index 58adfee..4defe44 100644 --- a/tools/perf/builtin-kmem.c +++ b/tools/perf/builtin-kmem.c @@ -4,7 +4,7 @@ #include "util/evlist.h" #include "util/evsel.h" #include "util/util.h" -#include "util/cache.h" +#include "util/config.h" #include "util/symbol.h" #include "util/thread.h" #include "util/header.h" diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c index b1304eb..c97b2b69 100644 --- a/tools/perf/builtin-record.c +++ b/tools/perf/builtin-record.c @@ -13,6 +13,7 @@ #include "util/util.h" #include #include "util/parse-events.h" +#include "util/config.h" #include "util/callchain.h" #include "util/cgroup.h" diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c index 9f36b23..bcb49ff 100644 --- a/tools/perf/builtin-report.c +++ b/tools/perf/builtin-report.c @@ -8,7 +8,7 @@ #include "builtin.h" #include "util/util.h" -#include "util/cache.h" +#include "util/config.h" #include "util/annotate.h" #include "util/color.h" diff --git a/tools/perf/builtin-top.c b/tools/perf/builtin-top.c index 81dba80..ec4cba6 100644 --- a/tools/perf/builtin-top.c +++ b/tools/perf/builtin-top.c @@ -22,7 +22,7 @@ #include "perf.h" #include "util/annotate.h" -#include "util/cache.h" +#include "util/config.h" #include "util/color.h" #include "util/evlist.h" #include "util/evsel.h" diff --git a/tools/perf/perf.c b/tools/perf/perf.c index 634bf7c..66772da 100644 --- a/tools/perf/perf.c +++ b/tools/perf/perf.c @@ -10,7 +10,7 @@ #include "util/env.h" #include -#include "util/cache.h" +#include "util/config.h" #include "util/quote.h" #include #include "util/parse-events.h" diff --git a/tools/perf/ui/browser.c b/tools/perf/ui/browser.c index af68a9d..3eb3edb 100644 --- a/tools/perf/ui/browser.c +++ b/tools/perf/ui/browser.c @@ -1,5 +1,5 @@ #include "../util.h" -#include "../cache.h" +#include "../config.h" #include "../../perf.h" #include "libslang.h" #include "ui.h" diff --git a/tools/perf/ui/browsers/annotate.c b/tools/perf/ui/browsers/annotate.c index 4fc208e..0e106bb 100644 --- a/tools/perf/ui/browsers/annotate.c +++ b/tools/perf/ui/browsers/annotate.c @@ -8,6 +8,7 @@ #include "../../util/sort.h" #include "../../util/symbol.h" #include "../../util/evsel.h" +#include "../../util/config.h" #include struct disasm_line_samples { diff --git a/tools/perf/util/alias.c b/tools/perf/util/alias.c index c0b43ee..6c80f83 100644 --- a/tools/perf/util/alias.c +++ b/tools/perf/util/alias.c @@ -1,4 +1,5 @@ #include "cache.h" +#include "config.h" static const char *alias_key; static char *alias_val; diff --git a/tools/perf/util/cache.h b/tools/perf/util/cache.h index 369f382..9f90e36 100644 --- a/tools/perf/util/cache.h +++ b/tools/perf/util/cache.h @@ -18,17 +18,6 @@ #define PERF_TRACEFS_ENVIRONMENT "PERF_TRACEFS_DIR" #define PERF_PAGER_ENVIRONMENT "PERF_PAGER" -extern co
[PATCH v10 3/3] perf config: Reimplement show_config() using config_set__for_each
Lately config_set__for_each is added. In order to let show_config() be short and clear, remake this function using config_set__for_each macro Cc: Namhyung Kim Cc: Jiri Olsa Cc: Masami Hiramatsu Cc: Alexander Shishkin Cc: Wang Nan Signed-off-by: Taeung Song --- tools/perf/builtin-config.c | 17 + 1 file changed, 5 insertions(+), 12 deletions(-) diff --git a/tools/perf/builtin-config.c b/tools/perf/builtin-config.c index cfd1036..c144643 100644 --- a/tools/perf/builtin-config.c +++ b/tools/perf/builtin-config.c @@ -37,23 +37,16 @@ static int show_config(struct perf_config_set *set) { struct perf_config_section *section; struct perf_config_item *item; - struct list_head *sections; if (set == NULL) return -1; - sections = &set->sections; - if (list_empty(sections)) - return -1; - - list_for_each_entry(section, sections, node) { - list_for_each_entry(item, §ion->items, node) { - char *value = item->value; + config_set__for_each(set, section, item) { + char *value = item->value; - if (value) - printf("%s.%s=%s\n", section->name, - item->name, value); - } + if (value) + printf("%s.%s=%s\n", section->name, + item->name, value); } return 0; -- 2.5.0
[PATCH v10 0/3] perf config: Reimplement perf_config()
Hello, :) This patchset is to reimplement perf_config() for efficient config management. Many sub-commands use perf_config() but everytime perf_config() is called, perf_config() always read config files. (i.e. user config '~/.perfconfig' and system config '$(sysconfdir)/perfconfig') But it is better to use the config set that already contains all config key-value pairs to avoid this repetitive work reading the config files in perf_config(). In summary, in order to use features about configuration, we can call the functions at perf.c and other source files as below. # initialize a config set perf_config_init() # configure actual variables from a config set perf_config() # eliminate allocated config set perf_config_finish() # destroy existing config set and initialize a new config set. perf_config_refresh() IMHO, I think this patchset is needed because not only the repetitive work should be avoided but also in near future, it would be smooth to manage perf configs. If you give me any feedback, I'd apprecicated it. :) Thanks, Taeung v10: - rebased onto current acme/perf/core v9: - add config_set__for_each macro (Arnaldo) - the source files that only need config.h don't include cache.h (Arnaldo) - use 'config_set' as a static variable instead of a global variable. (Namhyung) - add perf_config_init(), perf_config_finish() and perf_config_refresh() (Namhyung) - remove [PATCH v8 4/5] perf config: Use zfree() instead of free() at perf_config_set__delete() - rebased onto current acme/perf/core - applied ([BUGFIX][PATCH v8 1/5] 826424) v8: - handle the error about NULL at perf_config_set__delete() - bring declarations about config from util/config.h to util/config.h - reimplement show_config() using perf_config_set__iter() instead of perf_config() - rebased onto perf-core-for-mingo-20160607 - applied ([PATCH v7 1/7] 25d8f48, [PATCH v7 2/7] 8beeb00) v7: - fill a missing crumb that assign NULL to 'set' variable in perf_config_set__new() (Arnaldo) - two patches applied ([PATCH v6 1/9] 78f71c9, [PATCH v6 3/9] 7db91f2) v6: - add printing error message when perf_config_set__iter() is failed - modify commit messages for bugfix 1~3 (PATCH 1/9 ~ 3/9) to help reviewers easily understand why them is needed v5: - solve the leak when perf_config_set__init() failed (Arnaldo) (to clear the problem it is needed to apply the bottom bugfix 1~3 patches) - bugfix 1) fix the problem of abnormal terminaltion at perf_parse_file() called by perf_config() - bugfix 2) if failed at collect_config(), finally free a config set after it is done instead of freeing the config set in the function - bugfix 3) handle NULL pointer exception of 'set' at collect_config() v4: - Keep perf_config_set__delete() as it is (Arnaldo) - Remove perf_config_set__check() (Arnaldo) - Keep the existing code about the config set at cmd_config() (Arnaldo) v3: - add freeing config set after sub-command work at run_builtin() (Namhyung) - remove needless code about the config set at cmd_config() - add a patch about a global variable 'config_set' v2: - split a patch into several patches - reimplement show_config() using new perf_config() - modify perf_config_set__delete using global variable 'config_set' - reset config set when only 'config' sub-commaned work because of options for config file location Taeung Song (3): perf config: Bring declarations about config from util/cache.h to util/config.h perf config: Reimplement perf_config() introducing new perf_config_init() and perf_config_finish() perf config: Reimplement show_config() using config_set__for_each tools/perf/builtin-config.c| 21 - tools/perf/builtin-help.c | 2 +- tools/perf/builtin-kmem.c | 2 +- tools/perf/builtin-record.c| 1 + tools/perf/builtin-report.c| 2 +- tools/perf/builtin-top.c | 2 +- tools/perf/perf.c | 4 +- tools/perf/ui/browser.c| 2 +- tools/perf/ui/browsers/annotate.c | 1 + tools/perf/util/alias.c| 1 + tools/perf/util/cache.h| 11 - tools/perf/util/color.c| 1 + tools/perf/util/config.c | 92 +++--- tools/perf/util/config.h | 40 + tools/perf/util/help-unknown-cmd.c | 1 + tools/perf/util/intel-pt.c | 1 + tools/perf/util/llvm-utils.c | 1 + 17 files changed, 111 insertions(+), 74 deletions(-) -- 2.5.0
[PATCH v10 2/3] perf config: Reimplement perf_config() introducing new perf_config_init() and perf_config_finish()
Many sub-commands use perf_config() but everytime perf_config() is called, perf_config() always read config files. (i.e. user config '~/.perfconfig' and system config '$(sysconfdir)/perfconfig') But it is better to use the config set that already contains all config key-value pairs to avoid this repetitive work reading the config files in perf_config(). (the config set mean a static variable 'config_set') In other words, if new perf_config_init() is called, only first time 'config_set' is initialized collecting all configs from the config files. And then we could use new perf_config() like old perf_config(). When a sub-command finished, free the config set by perf_config_finish() at run_builtin(). If we do, 'config_set' can be reused wherever perf_config() is called and a feature of old perf_config() is the same as new perf_config() work without the repetitive work that read the config files. In summary, in order to use features about configuration, we can call the functions at perf.c and other source files as below. # initialize a config set perf_config_init() # configure actual variables from a config set perf_config() # eliminate allocated config set perf_config_finish() # destroy existing config set and initialize a new config set. perf_config_refresh() Cc: Namhyung Kim Cc: Jiri Olsa Cc: Wang Nan Cc: Peter Zijlstra Cc: Ingo Molnar Cc: Masami Hiramatsu Cc: Alexander Shishkin Signed-off-by: Taeung Song --- tools/perf/builtin-config.c | 4 ++ tools/perf/perf.c | 2 + tools/perf/util/config.c| 92 +++-- tools/perf/util/config.h| 29 ++ 4 files changed, 82 insertions(+), 45 deletions(-) diff --git a/tools/perf/builtin-config.c b/tools/perf/builtin-config.c index fe1b77f..cfd1036 100644 --- a/tools/perf/builtin-config.c +++ b/tools/perf/builtin-config.c @@ -80,6 +80,10 @@ int cmd_config(int argc, const char **argv, const char *prefix __maybe_unused) else if (use_user_config) config_exclusive_filename = user_config; + /* +* At only 'config' sub-command, individually use the config set +* because of reinitializing with options config file location. +*/ set = perf_config_set__new(); if (!set) { ret = -1; diff --git a/tools/perf/perf.c b/tools/perf/perf.c index 66772da..280967e 100644 --- a/tools/perf/perf.c +++ b/tools/perf/perf.c @@ -355,6 +355,7 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv) perf_env__set_cmdline(&perf_env, argc, argv); status = p->fn(argc, argv, prefix); + perf_config_finish(); exit_browser(status); perf_env__exit(&perf_env); bpf__clear(); @@ -522,6 +523,7 @@ int main(int argc, const char **argv) srandom(time(NULL)); + perf_config_init(); perf_config(perf_default_config, NULL); set_buildid_dir(NULL); diff --git a/tools/perf/util/config.c b/tools/perf/util/config.c index d15c592..a16f95d 100644 --- a/tools/perf/util/config.c +++ b/tools/perf/util/config.c @@ -26,6 +26,7 @@ static FILE *config_file; static const char *config_file_name; static int config_linenr; static int config_file_eof; +static struct perf_config_set *config_set; const char *config_exclusive_filename; @@ -478,51 +479,6 @@ static int perf_config_global(void) return !perf_env_bool("PERF_CONFIG_NOGLOBAL", 0); } -int perf_config(config_fn_t fn, void *data) -{ - int ret = -1; - const char *home = NULL; - - /* Setting $PERF_CONFIG makes perf read _only_ the given config file. */ - if (config_exclusive_filename) - return perf_config_from_file(fn, config_exclusive_filename, data); - if (perf_config_system() && !access(perf_etc_perfconfig(), R_OK)) { - if (perf_config_from_file(fn, perf_etc_perfconfig(), data) < 0) - goto out; - } - - home = getenv("HOME"); - if (perf_config_global() && home) { - char *user_config = strdup(mkpath("%s/.perfconfig", home)); - struct stat st; - - if (user_config == NULL) { - warning("Not enough memory to process %s/.perfconfig, " - "ignoring it.", home); - goto out; - } - - if (stat(user_config, &st) < 0) - goto out_free; - - if (st.st_uid && (st.st_uid != geteuid())) { - warning("File %s not owned by current user or root, " - "ignoring it.", user_config); - goto out_free; - } - - if (!st.st_size) -
Re: [PATCH v8 3/5] perf config: Reimplement perf_config() using perf_config_set__it to
Hi, Arnaldo :-D I also discussed this patchset with Namhyung at offline gathering. I sent v9 patchset that contains what Namhyung and you advice me to do. If having your spare time, please review the v9 patchset. If you do, I'd appreciate it :-) Thanks, Taeung On 06/12/2016 05:57 PM, Taeung Song wrote: Hi, Arnaldo What do you think about setting all actual config variables before a sub-command start at perf.c ? (in order to free the config set that contains all configs by perf_config_set__delete() after using it was finished where it was needed) i.e. initialize all actual config variables at perf.c instead of at each source file as below. (before running a particular sub-command at run_builitin()) util/alias.c 22:perf_config(alias_lookup_cb, NULL); util/intel-pt.c 330:perf_config(intel_pt_config_div, &d); 1993:static int intel_pt_perf_config(const char *var, const char *value, void *data) 2047:perf_config(intel_pt_perf_config, pt); util/data-convert-bt.c 1302:perf_config(convert__config, &c); util/help-unknown-cmd.c 62:perf_config(perf_unknown_cmd_config, NULL); builtin-help.c 454:perf_config(perf_help_config, &help_format); perf.c 94:perf_config(pager_command_config, &c); 117:perf_config(browser_command_config, &c); 561:perf_config(perf_default_config, NULL); builtin-record.c 1432:perf_config(perf_record_config, rec); ui/browsers/annotate.c 1163:perf_config(annotate__config, NULL); ui/browser.c 743:perf_config(ui_browser__color_config, NULL); builtin-report.c 829:perf_config(report__config, &report); builtin-kmem.c 1899:perf_config(kmem_config, NULL); builtin-top.c 1231:perf_config(perf_top_config, &top); If we do, we could set all actual config variables before a sub-command work. And the config set that is made by perf_config_set__new() would be reused when we initialize all actual config variables and then if using is is finished, we could free the config set. Thanks, Taeung On 06/10/2016 07:58 PM, Taeung Song wrote: On 06/09/2016 10:34 PM, Arnaldo Carvalho de Melo wrote: Em Wed, Jun 08, 2016 at 09:36:51PM +0900, Taeung Song escreveu: Many sub-commands use perf_config() so everytime perf_config() is called, perf_config() always read config files. (i.e. user config '~/.perfconfig' and system config '$(sysconfdir)/perfconfig') And this is not always a bad thing, think about being in 'mutt' and adding an entry to ~/.mail_aliases, then going to compose a message, would be good that the just added entry to ~/.mail_aliases be considered when adding recipients to your messages, right? In the same fashion, 'perf report', 'perf annotate', 'perf top' are long running utilities that have operations that could get changes to config files without having to restart them, i.e. do annotation changes in your ~/.perfconfig and then see them reflected next time you hit 'A´ to annotate a function in 'perf top' or 'perf report'. Currently, this suggestion isn't already contained among features of perf. right? I understood that you mean while perf process is already running if user change a config file, the changed config can be reflected in 'perf top', 'perf report' or etc without restarting perf process like mutt. Is it right ? So, I think that if tools want ammortize the cost of reading config files, they should create an instance of the relevant object (perf_config_set?) use it and then delete it, but not keep one around for a long time. I got it. How about the two ideas that I have in mind ? 1) If we eliminate the config set object(perf_config_set) after using it because we don't need to keep it until perf process is done, we could bring many codes using perf_config() at one spot of perf.c ? (because it is hard to decide a point of time we destroy the config set.) For example, (This code isn't executable) diff --git a/tools/perf/perf.c b/tools/perf/perf.c index 15982ce..6a56985 100644 --- a/tools/perf/perf.c +++ b/tools/perf/perf.c @@ -77,6 +77,23 @@ struct pager_config { int val; }; +static void perf_default_config_init(void) +{ +default_colors_config_init(); +default_annoate_config_init(); +default_report_config_init(); +... +} + +static void perf_config_init(void) +{ +perf_default_config_init(); +colors_config_init(); +annotate_config_init(); +report_config_init(); +... +} + static int pager_command_config(const char *var, const char *value, void *data) { struct pager_config *c = data; @@ -558,7 +575,7 @@ int main(int argc, const char **argv) srandom(time(NULL)); -perf_config(perf_default_config, NULL); +perf_config_init(); set_buildid_dir(NULL); /* get debugfs/tracefs mount point from /proc/mounts */ diff --gi
[REFACTORING][PATCH v9 1/3] perf config: Bring declarations about config from util/cache.h to util/config.h
Lately util/config.h has been added but util/cache.h has declarations of functions and a global variable for config features. To manage codes about configuration at one spot, move them to util/config.h and let source files that need config features include config.h And if the source files that included previous cache.h need only config.h, remove including cache.h. Cc: Alexander Shishkin Cc: Jiri Olsa Cc: Masami Hiramatsu Cc: Namhyung Kim Cc: Wang Nan Signed-off-by: Taeung Song --- tools/perf/builtin-help.c | 2 +- tools/perf/builtin-kmem.c | 2 +- tools/perf/builtin-record.c| 1 + tools/perf/builtin-report.c| 2 +- tools/perf/builtin-top.c | 2 +- tools/perf/perf.c | 2 +- tools/perf/ui/browser.c| 2 +- tools/perf/ui/browsers/annotate.c | 1 + tools/perf/util/alias.c| 1 + tools/perf/util/cache.h| 12 tools/perf/util/color.c| 1 + tools/perf/util/config.h | 12 tools/perf/util/help-unknown-cmd.c | 1 + tools/perf/util/intel-pt.c | 1 + 14 files changed, 24 insertions(+), 18 deletions(-) diff --git a/tools/perf/builtin-help.c b/tools/perf/builtin-help.c index f9830c9..268ab73 100644 --- a/tools/perf/builtin-help.c +++ b/tools/perf/builtin-help.c @@ -4,7 +4,7 @@ * Builtin help command */ #include "perf.h" -#include "util/cache.h" +#include "util/config.h" #include "builtin.h" #include #include "common-cmds.h" diff --git a/tools/perf/builtin-kmem.c b/tools/perf/builtin-kmem.c index 58adfee..4defe44 100644 --- a/tools/perf/builtin-kmem.c +++ b/tools/perf/builtin-kmem.c @@ -4,7 +4,7 @@ #include "util/evlist.h" #include "util/evsel.h" #include "util/util.h" -#include "util/cache.h" +#include "util/config.h" #include "util/symbol.h" #include "util/thread.h" #include "util/header.h" diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c index d4cf1b0..74e8133 100644 --- a/tools/perf/builtin-record.c +++ b/tools/perf/builtin-record.c @@ -13,6 +13,7 @@ #include "util/util.h" #include #include "util/parse-events.h" +#include "util/config.h" #include "util/callchain.h" #include "util/cgroup.h" diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c index 9f36b23..bcb49ff 100644 --- a/tools/perf/builtin-report.c +++ b/tools/perf/builtin-report.c @@ -8,7 +8,7 @@ #include "builtin.h" #include "util/util.h" -#include "util/cache.h" +#include "util/config.h" #include "util/annotate.h" #include "util/color.h" diff --git a/tools/perf/builtin-top.c b/tools/perf/builtin-top.c index 81dba80..ec4cba6 100644 --- a/tools/perf/builtin-top.c +++ b/tools/perf/builtin-top.c @@ -22,7 +22,7 @@ #include "perf.h" #include "util/annotate.h" -#include "util/cache.h" +#include "util/config.h" #include "util/color.h" #include "util/evlist.h" #include "util/evsel.h" diff --git a/tools/perf/perf.c b/tools/perf/perf.c index 15982ce..35553c7 100644 --- a/tools/perf/perf.c +++ b/tools/perf/perf.c @@ -10,7 +10,7 @@ #include "util/env.h" #include -#include "util/cache.h" +#include "util/config.h" #include "util/quote.h" #include #include "util/parse-events.h" diff --git a/tools/perf/ui/browser.c b/tools/perf/ui/browser.c index af68a9d..3eb3edb 100644 --- a/tools/perf/ui/browser.c +++ b/tools/perf/ui/browser.c @@ -1,5 +1,5 @@ #include "../util.h" -#include "../cache.h" +#include "../config.h" #include "../../perf.h" #include "libslang.h" #include "ui.h" diff --git a/tools/perf/ui/browsers/annotate.c b/tools/perf/ui/browsers/annotate.c index 4fc208e..0e106bb 100644 --- a/tools/perf/ui/browsers/annotate.c +++ b/tools/perf/ui/browsers/annotate.c @@ -8,6 +8,7 @@ #include "../../util/sort.h" #include "../../util/symbol.h" #include "../../util/evsel.h" +#include "../../util/config.h" #include struct disasm_line_samples { diff --git a/tools/perf/util/alias.c b/tools/perf/util/alias.c index c0b43ee..6c80f83 100644 --- a/tools/perf/util/alias.c +++ b/tools/perf/util/alias.c @@ -1,4 +1,5 @@ #include "cache.h" +#include "config.h" static const char *alias_key; static char *alias_val; diff --git a/tools/perf/util/cache.h b/tools/perf/util/cache.h index 0d814bb..5df4407 100644 --- a/tools/perf/util/cache.h +++ b/tools/perf/util/cache.h @@ -23,18 +23,6 @@ #define PERF_TRACEFS_ENVIRONMENT "PERF_TRACEFS_DIR" #define PERF_PAGER_ENVIRONMENT "PERF_PAGER" -extern const char *config_exclusive_filename; - -t
[PATCH v9 3/3] perf config: Reimplement show_config() using config_set__for_each
Lately config_set__for_each is added. In order to let show_config() be short and clear, remake this function using config_set__for_each macro Cc: Namhyung Kim Cc: Jiri Olsa Cc: Masami Hiramatsu Cc: Alexander Shishkin Cc: Wang Nan Signed-off-by: Taeung Song --- tools/perf/builtin-config.c | 17 + 1 file changed, 5 insertions(+), 12 deletions(-) diff --git a/tools/perf/builtin-config.c b/tools/perf/builtin-config.c index cfd1036..c144643 100644 --- a/tools/perf/builtin-config.c +++ b/tools/perf/builtin-config.c @@ -37,23 +37,16 @@ static int show_config(struct perf_config_set *set) { struct perf_config_section *section; struct perf_config_item *item; - struct list_head *sections; if (set == NULL) return -1; - sections = &set->sections; - if (list_empty(sections)) - return -1; - - list_for_each_entry(section, sections, node) { - list_for_each_entry(item, §ion->items, node) { - char *value = item->value; + config_set__for_each(set, section, item) { + char *value = item->value; - if (value) - printf("%s.%s=%s\n", section->name, - item->name, value); - } + if (value) + printf("%s.%s=%s\n", section->name, + item->name, value); } return 0; -- 2.5.0
[RFC][PATCH v9 0/3] perf config: Reimplement perf_config() adding perf_config_init() and perf_config_finish()
Hello, :) This patchset is to reimplement perf_config() for efficient config management. Everytime perf_config() is called, perf_config() always read config files. (i.e. user config '~/.perfconfig' and system config '$(sysconfdir)/perfconfig') But we need to use 'struct perf_config_set config_set' variable that already contains all config key-value pairs to avoid this repetitive work in perf_config(). In other words, if new perf_config() is called, only first time 'config_set' is initialized collecting all configs from config files and it work with perf_config_set__iter(). If we do, 'config_set' can be reused wherever using perf_config() and a feature of old perf_config() is the same as new perf_config() work without the repetitive work that read the config files. IMHO, I think this patchset is needed because not only the repetitive work should be avoided but also in near future, it would be smooth to manage perf configs. If you give me any feedback, I'd apprecicated it. :) Thanks, Taeung v9: - add config_set__for_each macro (Arnaldo) - the source files that only need config.h don't include cache.h (Arnaldo) - use 'config_set' as a static variable instead of a global variable. (Namhyung) - add perf_config_init(), perf_config_finish() and perf_config_refresh() (Namhyung) - remove [PATCH v8 4/5] perf config: Use zfree() instead of free() at perf_config_set__delete() - rebased onto current acme/perf/core - applied ([BUGFIX][PATCH v8 1/5] 826424) v8: - handle the error about NULL at perf_config_set__delete() - bring declarations about config from util/config.h to util/config.h - reimplement show_config() using perf_config_set__iter() instead of perf_config() - rebased onto perf-core-for-mingo-20160607 - applied ([PATCH v7 1/7] 25d8f48, [PATCH v7 2/7] 8beeb00) v7: - fill a missing crumb that assign NULL to 'set' variable in perf_config_set__new() (Arnaldo) - two patches applied ([PATCH v6 1/9] 78f71c9, [PATCH v6 3/9] 7db91f2) v6: - add printing error message when perf_config_set__iter() is failed - modify commit messages for bugfix 1~3 (PATCH 1/9 ~ 3/9) to help reviewers easily understand why them is needed v5: - solve the leak when perf_config_set__init() failed (Arnaldo) (to clear the problem it is needed to apply the bottom bugfix 1~3 patches) - bugfix 1) fix the problem of abnormal terminaltion at perf_parse_file() called by perf_config() - bugfix 2) if failed at collect_config(), finally free a config set after it is done instead of freeing the config set in the function - bugfix 3) handle NULL pointer exception of 'set' at collect_config() v4: - Keep perf_config_set__delete() as it is (Arnaldo) - Remove perf_config_set__check() (Arnaldo) - Keep the existing code about the config set at cmd_config() (Arnaldo) v3: - add freeing config set after sub-command work at run_builtin() (Namhyung) - remove needless code about the config set at cmd_config() - add a patch about a global variable 'config_set' v2: - split a patch into several patches - reimplement show_config() using new perf_config() - modify perf_config_set__delete using global variable 'config_set' - reset config set when only 'config' sub-commaned work because of options for config file location Taeung Song (3): perf config: Bring declarations about config from util/cache.h to util/config.h perf config: Reimplement perf_config() adding perf_config_init() and perf_config_finish() perf config: Reimplement show_config() using config_set__for_each tools/perf/builtin-config.c| 21 - tools/perf/builtin-help.c | 2 +- tools/perf/builtin-kmem.c | 2 +- tools/perf/builtin-record.c| 1 + tools/perf/builtin-report.c| 2 +- tools/perf/builtin-top.c | 2 +- tools/perf/perf.c | 4 +- tools/perf/ui/browser.c| 2 +- tools/perf/ui/browsers/annotate.c | 1 + tools/perf/util/alias.c| 1 + tools/perf/util/cache.h| 12 - tools/perf/util/color.c| 1 + tools/perf/util/config.c | 92 +++--- tools/perf/util/config.h | 41 + tools/perf/util/help-unknown-cmd.c | 1 + tools/perf/util/intel-pt.c | 1 + 16 files changed, 111 insertions(+), 75 deletions(-) -- 2.5.0
[RFC][PATCH v9 2/3] perf config: Reimplement perf_config() adding perf_config_init() and perf_config_finish()
Many sub-commands use perf_config() so everytime perf_config() is called, perf_config() always read config files. (i.e. user config '~/.perfconfig' and system config '$(sysconfdir)/perfconfig') But we need to use the config set that already contains all config key-value pairs to avoid this repetitive work reading the config files in perf_config(). (the config set mean a static variable 'config_set') In other words, if new perf_config_init() is called, only first time 'config_set' is initialized collecting all configs from the config files. And then we could use new perf_config() like old perf_config(). When a sub-command finished, free the config set by perf_config_finish() at run_builtin(). If we do, 'config_set' can be reused wherever perf_config() is called and a feature of old perf_config() is the same as new perf_config() work without the repetitive work that read the config files. In summary, in order to use features about configuration, we can call the functions from outside as below. # initialize a config set perf_config_init() # configure actual variables from a config set perf_config() # eliminate allocated config set perf_config_finish() # destroy existing config set and initialize a new config set. perf_config_refresh() Cc: Namhyung Kim Cc: Jiri Olsa Cc: Wang Nan Cc: Peter Zijlstra Cc: Ingo Molnar Cc: Masami Hiramatsu Cc: Alexander Shishkin Signed-off-by: Taeung Song --- tools/perf/builtin-config.c | 4 ++ tools/perf/perf.c | 2 + tools/perf/util/config.c| 92 +++-- tools/perf/util/config.h| 29 ++ 4 files changed, 82 insertions(+), 45 deletions(-) diff --git a/tools/perf/builtin-config.c b/tools/perf/builtin-config.c index fe1b77f..cfd1036 100644 --- a/tools/perf/builtin-config.c +++ b/tools/perf/builtin-config.c @@ -80,6 +80,10 @@ int cmd_config(int argc, const char **argv, const char *prefix __maybe_unused) else if (use_user_config) config_exclusive_filename = user_config; + /* +* At only 'config' sub-command, individually use the config set +* because of reinitializing with options config file location. +*/ set = perf_config_set__new(); if (!set) { ret = -1; diff --git a/tools/perf/perf.c b/tools/perf/perf.c index 35553c7..894bf5d0 100644 --- a/tools/perf/perf.c +++ b/tools/perf/perf.c @@ -391,6 +391,7 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv) perf_env__set_cmdline(&perf_env, argc, argv); status = p->fn(argc, argv, prefix); + perf_config_finish(); exit_browser(status); perf_env__exit(&perf_env); bpf__clear(); @@ -558,6 +559,7 @@ int main(int argc, const char **argv) srandom(time(NULL)); + perf_config_init(); perf_config(perf_default_config, NULL); set_buildid_dir(NULL); diff --git a/tools/perf/util/config.c b/tools/perf/util/config.c index 31e09a4..5b50134 100644 --- a/tools/perf/util/config.c +++ b/tools/perf/util/config.c @@ -26,6 +26,7 @@ static FILE *config_file; static const char *config_file_name; static int config_linenr; static int config_file_eof; +static struct perf_config_set *config_set; const char *config_exclusive_filename; @@ -478,51 +479,6 @@ static int perf_config_global(void) return !perf_env_bool("PERF_CONFIG_NOGLOBAL", 0); } -int perf_config(config_fn_t fn, void *data) -{ - int ret = -1; - const char *home = NULL; - - /* Setting $PERF_CONFIG makes perf read _only_ the given config file. */ - if (config_exclusive_filename) - return perf_config_from_file(fn, config_exclusive_filename, data); - if (perf_config_system() && !access(perf_etc_perfconfig(), R_OK)) { - if (perf_config_from_file(fn, perf_etc_perfconfig(), data) < 0) - goto out; - } - - home = getenv("HOME"); - if (perf_config_global() && home) { - char *user_config = strdup(mkpath("%s/.perfconfig", home)); - struct stat st; - - if (user_config == NULL) { - warning("Not enough memory to process %s/.perfconfig, " - "ignoring it.", home); - goto out; - } - - if (stat(user_config, &st) < 0) - goto out_free; - - if (st.st_uid && (st.st_uid != geteuid())) { - warning("File %s not owned by current user or root, " - "ignoring it.", user_config); - goto out_free; - } - - if (!st.st_size) - goto out_free
[tip:perf/core] perf config: Handle NULL at perf_config_set__delete()
Commit-ID: 826424cc919529d6d234af12c6ba975b63528a74 Gitweb: http://git.kernel.org/tip/826424cc919529d6d234af12c6ba975b63528a74 Author: Taeung Song AuthorDate: Wed, 8 Jun 2016 21:36:49 +0900 Committer: Arnaldo Carvalho de Melo CommitDate: Tue, 14 Jun 2016 09:29:53 -0300 perf config: Handle NULL at perf_config_set__delete() perf_config_set__delete() purge and free the config set that contains all config key-value pairs. But if the config set (i.e. 'set' variable at the function) is NULL, this is wrong so handle it. Signed-off-by: Taeung Song Cc: Alexander Shishkin Cc: Jiri Olsa Cc: Masami Hiramatsu Cc: Namhyung Kim Cc: Peter Zijlstra Cc: Wang Nan Link: http://lkml.kernel.org/r/1465389413-8936-2-git-send-email-treeze.tae...@gmail.com Signed-off-by: Arnaldo Carvalho de Melo --- tools/perf/util/config.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/tools/perf/util/config.c b/tools/perf/util/config.c index 8749eca..31e09a4 100644 --- a/tools/perf/util/config.c +++ b/tools/perf/util/config.c @@ -742,6 +742,9 @@ static void perf_config_set__purge(struct perf_config_set *set) void perf_config_set__delete(struct perf_config_set *set) { + if (set == NULL) + return; + perf_config_set__purge(set); free(set); }
Re: [PATCH v8 3/5] perf config: Reimplement perf_config() using perf_config_set__iter()
Hi, Arnaldo What do you think about setting all actual config variables before a sub-command start at perf.c ? (in order to free the config set that contains all configs by perf_config_set__delete() after using it was finished where it was needed) i.e. initialize all actual config variables at perf.c instead of at each source file as below. (before running a particular sub-command at run_builitin()) util/alias.c 22: perf_config(alias_lookup_cb, NULL); util/intel-pt.c 330:perf_config(intel_pt_config_div, &d); 1993:static int intel_pt_perf_config(const char *var, const char *value, void *data) 2047: perf_config(intel_pt_perf_config, pt); util/data-convert-bt.c 1302: perf_config(convert__config, &c); util/help-unknown-cmd.c 62: perf_config(perf_unknown_cmd_config, NULL); builtin-help.c 454:perf_config(perf_help_config, &help_format); perf.c 94: perf_config(pager_command_config, &c); 117:perf_config(browser_command_config, &c); 561:perf_config(perf_default_config, NULL); builtin-record.c 1432: perf_config(perf_record_config, rec); ui/browsers/annotate.c 1163: perf_config(annotate__config, NULL); ui/browser.c 743:perf_config(ui_browser__color_config, NULL); builtin-report.c 829:perf_config(report__config, &report); builtin-kmem.c 1899: perf_config(kmem_config, NULL); builtin-top.c 1231: perf_config(perf_top_config, &top); If we do, we could set all actual config variables before a sub-command work. And the config set that is made by perf_config_set__new() would be reused when we initialize all actual config variables and then if using is is finished, we could free the config set. Thanks, Taeung On 06/10/2016 07:58 PM, Taeung Song wrote: On 06/09/2016 10:34 PM, Arnaldo Carvalho de Melo wrote: Em Wed, Jun 08, 2016 at 09:36:51PM +0900, Taeung Song escreveu: Many sub-commands use perf_config() so everytime perf_config() is called, perf_config() always read config files. (i.e. user config '~/.perfconfig' and system config '$(sysconfdir)/perfconfig') And this is not always a bad thing, think about being in 'mutt' and adding an entry to ~/.mail_aliases, then going to compose a message, would be good that the just added entry to ~/.mail_aliases be considered when adding recipients to your messages, right? In the same fashion, 'perf report', 'perf annotate', 'perf top' are long running utilities that have operations that could get changes to config files without having to restart them, i.e. do annotation changes in your ~/.perfconfig and then see them reflected next time you hit 'A´ to annotate a function in 'perf top' or 'perf report'. Currently, this suggestion isn't already contained among features of perf. right? I understood that you mean while perf process is already running if user change a config file, the changed config can be reflected in 'perf top', 'perf report' or etc without restarting perf process like mutt. Is it right ? So, I think that if tools want ammortize the cost of reading config files, they should create an instance of the relevant object (perf_config_set?) use it and then delete it, but not keep one around for a long time. I got it. How about the two ideas that I have in mind ? 1) If we eliminate the config set object(perf_config_set) after using it because we don't need to keep it until perf process is done, we could bring many codes using perf_config() at one spot of perf.c ? (because it is hard to decide a point of time we destroy the config set.) For example, (This code isn't executable) diff --git a/tools/perf/perf.c b/tools/perf/perf.c index 15982ce..6a56985 100644 --- a/tools/perf/perf.c +++ b/tools/perf/perf.c @@ -77,6 +77,23 @@ struct pager_config { int val; }; +static void perf_default_config_init(void) +{ +default_colors_config_init(); +default_annoate_config_init(); +default_report_config_init(); +... +} + +static void perf_config_init(void) +{ +perf_default_config_init(); +colors_config_init(); +annotate_config_init(); +report_config_init(); +... +} + static int pager_command_config(const char *var, const char *value, void *data) { struct pager_config *c = data; @@ -558,7 +575,7 @@ int main(int argc, const char **argv) srandom(time(NULL)); -perf_config(perf_default_config, NULL); +perf_config_init(); set_buildid_dir(NULL); /* get debugfs/tracefs mount point from /proc/mounts */ diff --git a/tools/perf/ui/browser.c b/tools/perf/ui/browser.c index af68a9d..4b827c2 100644 --- a/tools/perf/ui/browser.c +++ b/tools/perf/ui/browser.c @@ -740,8 +740,6 @@ void ui_browser__init(void) { int i = 0; -perf_config(ui_browser__color_config, NULL); - while (ui_browser__colorsets[i]
Re: [PATCH v8 2/5] perf config: Bring declarations about config from util/cache.h to util/config.h
Hi, I have a question about header files. I'm cleaning up source files that used cache.h after moving codes about config from cache.h to config.h. But I found there are header files that are repeatedly declared over all. For example, builtin-report.c include util/sort.h, perf.h, util/util.h, util/cache.h and etc. However, util/sort.h also have #include "cache.h" and cache.h even include util.h and perf.h. Isn't this a problem (but this is minor) ? Of course, this patch don't need to contain codes to fix this above problem. Should we fix this problem ? (If we do, I'd individually send patches for this problem.) Thanks, Taeung On 06/11/2016 09:59 AM, Taeung Song wrote: Good evening :) On 06/11/2016 04:06 AM, Arnaldo Carvalho de Melo wrote: Em Fri, Jun 10, 2016 at 03:20:43PM +0900, Taeung Song escreveu: On 06/09/2016 10:29 PM, Arnaldo Carvalho de Melo wrote: +++ b/tools/perf/util/cache.h @@ -7,6 +7,7 @@ #include +#include "config.h" Why have you added that? Are those config functions used in cache.h? Yes, it does. Many source files include cache.h e.g. builtin-annoate.c, util/color.c, builtin-report.c and etc. And They can use perf_config() function including this header file. So, If I totally eliminate not only declarations about config but also #include "util/config.h" at util/cache.h, we should add '#include "util/config.h"' to each source file that need perf_config() overall. Sure, that is how we should do it. We should not include cache.h just to get what is in config.h, we should instead include config.h. This way when we do a change to cache.h we will not be rebuilding all those files that depend on it just to get config.h. What you're doing, removing from cache.h things that shouldn't be there in the first place is good, among other things, because of that. Granted! I've also experienced the situation all those files which include cache.h are rebuilt after I changed cache.h. It also seems a problem as you mention. So, I'll send this patch that reflect what you said with v9. Have a nice weekend :-D Thanks, Taeung
Re: [PATCH v8 2/5] perf config: Bring declarations about config from util/cache.h to util/config.h
Good evening :) On 06/11/2016 04:06 AM, Arnaldo Carvalho de Melo wrote: Em Fri, Jun 10, 2016 at 03:20:43PM +0900, Taeung Song escreveu: On 06/09/2016 10:29 PM, Arnaldo Carvalho de Melo wrote: +++ b/tools/perf/util/cache.h @@ -7,6 +7,7 @@ #include +#include "config.h" Why have you added that? Are those config functions used in cache.h? Yes, it does. Many source files include cache.h e.g. builtin-annoate.c, util/color.c, builtin-report.c and etc. And They can use perf_config() function including this header file. So, If I totally eliminate not only declarations about config but also #include "util/config.h" at util/cache.h, we should add '#include "util/config.h"' to each source file that need perf_config() overall. Sure, that is how we should do it. We should not include cache.h just to get what is in config.h, we should instead include config.h. This way when we do a change to cache.h we will not be rebuilding all those files that depend on it just to get config.h. What you're doing, removing from cache.h things that shouldn't be there in the first place is good, among other things, because of that. Granted! I've also experienced the situation all those files which include cache.h are rebuilt after I changed cache.h. It also seems a problem as you mention. So, I'll send this patch that reflect what you said with v9. Have a nice weekend :-D Thanks, Taeung
Re: [PATCH v8 3/5] perf config: Reimplement perf_config() using perf_config_set__iter()
On 06/09/2016 10:41 PM, Arnaldo Carvalho de Melo wrote: Em Wed, Jun 08, 2016 at 09:36:51PM +0900, Taeung Song escreveu: Many sub-commands use perf_config() so everytime perf_config() is called, perf_config() always read config files. (i.e. user config '~/.perfconfig' and system config '$(sysconfdir)/perfconfig') But we need to use the config set that already contains all config key-value pairs to avoid this repetitive work reading the config files in perf_config(). (the config set mean a global variable 'config_set') In other words, if new perf_config() is called, only first time 'config_set' is initialized collecting all configs from the config files and it work with perf_config_set__iter(). And free the config set after a sub-command work at run_builtin(). If we do, 'config_set' can be reused wherever using perf_config() and a feature of old perf_config() is the same as new perf_config() work without the repetitive work that read the config files. Cc: Namhyung Kim Cc: Jiri Olsa Cc: Wang Nan Cc: Peter Zijlstra Cc: Ingo Molnar Cc: Masami Hiramatsu Cc: Alexander Shishkin Signed-off-by: Taeung Song --- tools/perf/builtin-config.c | 4 +++ tools/perf/perf.c | 1 + tools/perf/util/config.c| 87 ++--- tools/perf/util/config.h| 1 + 4 files changed, 48 insertions(+), 45 deletions(-) diff --git a/tools/perf/builtin-config.c b/tools/perf/builtin-config.c index fe1b77f..cfd1036 100644 --- a/tools/perf/builtin-config.c +++ b/tools/perf/builtin-config.c @@ -80,6 +80,10 @@ int cmd_config(int argc, const char **argv, const char *prefix __maybe_unused) else if (use_user_config) config_exclusive_filename = user_config; + /* +* At only 'config' sub-command, individually use the config set +* because of reinitializing with options config file location. +*/ set = perf_config_set__new(); if (!set) { ret = -1; diff --git a/tools/perf/perf.c b/tools/perf/perf.c index 15982ce..fe2ab7c 100644 --- a/tools/perf/perf.c +++ b/tools/perf/perf.c @@ -391,6 +391,7 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv) perf_env__set_cmdline(&perf_env, argc, argv); status = p->fn(argc, argv, prefix); + perf_config_set__delete(config_set); exit_browser(status); perf_env__exit(&perf_env); bpf__clear(); diff --git a/tools/perf/util/config.c b/tools/perf/util/config.c index 31e09a4..72db134 100644 --- a/tools/perf/util/config.c +++ b/tools/perf/util/config.c @@ -28,6 +28,7 @@ static int config_linenr; static int config_file_eof; const char *config_exclusive_filename; +struct perf_config_set *config_set; static int get_next_char(void) { @@ -478,51 +479,6 @@ static int perf_config_global(void) return !perf_env_bool("PERF_CONFIG_NOGLOBAL", 0); } -int perf_config(config_fn_t fn, void *data) -{ - int ret = -1; - const char *home = NULL; - - /* Setting $PERF_CONFIG makes perf read _only_ the given config file. */ - if (config_exclusive_filename) - return perf_config_from_file(fn, config_exclusive_filename, data); - if (perf_config_system() && !access(perf_etc_perfconfig(), R_OK)) { - if (perf_config_from_file(fn, perf_etc_perfconfig(), data) < 0) - goto out; - } - - home = getenv("HOME"); - if (perf_config_global() && home) { - char *user_config = strdup(mkpath("%s/.perfconfig", home)); - struct stat st; - - if (user_config == NULL) { - warning("Not enough memory to process %s/.perfconfig, " - "ignoring it.", home); - goto out; - } - - if (stat(user_config, &st) < 0) - goto out_free; - - if (st.st_uid && (st.st_uid != geteuid())) { - warning("File %s not owned by current user or root, " - "ignoring it.", user_config); - goto out_free; - } - - if (!st.st_size) - goto out_free; - - ret = perf_config_from_file(fn, user_config, data); - -out_free: - free(user_config); - } -out: - return ret; -} - static struct perf_config_section *find_section(struct list_head *sections, const char *section_name) { @@ -706,6 +662,47 @@ struct perf_config_set *perf_config_set__new(void) return set; } +static int perf_config_set__iter(struct perf_config_set *set, config_fn_t fn, void *data) +{ + struct perf_config_section *section
Re: [PATCH v8 4/5] perf config: Use zfree() instead of free() at perf_config_set__delete()
On 06/09/2016 10:37 PM, Arnaldo Carvalho de Melo wrote: Em Wed, Jun 08, 2016 at 09:36:52PM +0900, Taeung Song escreveu: perf_config_set__delete() delete allocated the config set but the global variable 'config_set' is used all around. So purge and zfree by an address of the global variable , i.e. 'struct perf_config_set **' type instead of using local variable 'set' of which type is 'struct perf_config_set *'. -void perf_config_set__delete(struct perf_config_set *set) +void perf_config_set__delete(struct perf_config_set **set) { - if (set == NULL) + if (*set == NULL) return; - perf_config_set__purge(set); - free(set); + perf_config_set__purge(*set); + zfree(set); } Nope, don't change conventions like taht, a delete method should not receive a pointer to the pointer to be deleted, no odd cases, please. If you really think this is interesting, please introduce zdelete(), i.e.: void perf_config_set__zdelete(struct perf_config_set **set) { if (!set) return; perf_config_set__delete(*set); *set = NULL; } I understood! If we don't use the config set as a global variable, it seems that we wouldn't need to change perf_config_set__delete() to perf_config_set__zdelete() as this patch. Thanks, Taeung
Re: [PATCH v8 3/5] perf config: Reimplement perf_config() using perf_config_set__iter()
On 06/09/2016 10:34 PM, Arnaldo Carvalho de Melo wrote: Em Wed, Jun 08, 2016 at 09:36:51PM +0900, Taeung Song escreveu: Many sub-commands use perf_config() so everytime perf_config() is called, perf_config() always read config files. (i.e. user config '~/.perfconfig' and system config '$(sysconfdir)/perfconfig') And this is not always a bad thing, think about being in 'mutt' and adding an entry to ~/.mail_aliases, then going to compose a message, would be good that the just added entry to ~/.mail_aliases be considered when adding recipients to your messages, right? In the same fashion, 'perf report', 'perf annotate', 'perf top' are long running utilities that have operations that could get changes to config files without having to restart them, i.e. do annotation changes in your ~/.perfconfig and then see them reflected next time you hit 'A´ to annotate a function in 'perf top' or 'perf report'. Currently, this suggestion isn't already contained among features of perf. right? I understood that you mean while perf process is already running if user change a config file, the changed config can be reflected in 'perf top', 'perf report' or etc without restarting perf process like mutt. Is it right ? So, I think that if tools want ammortize the cost of reading config files, they should create an instance of the relevant object (perf_config_set?) use it and then delete it, but not keep one around for a long time. I got it. How about the two ideas that I have in mind ? 1) If we eliminate the config set object(perf_config_set) after using it because we don't need to keep it until perf process is done, we could bring many codes using perf_config() at one spot of perf.c ? (because it is hard to decide a point of time we destroy the config set.) For example, (This code isn't executable) diff --git a/tools/perf/perf.c b/tools/perf/perf.c index 15982ce..6a56985 100644 --- a/tools/perf/perf.c +++ b/tools/perf/perf.c @@ -77,6 +77,23 @@ struct pager_config { int val; }; +static void perf_default_config_init(void) +{ +default_colors_config_init(); +default_annoate_config_init(); +default_report_config_init(); +... +} + +static void perf_config_init(void) +{ +perf_default_config_init(); +colors_config_init(); +annotate_config_init(); +report_config_init(); +... +} + static int pager_command_config(const char *var, const char *value, void *data) { struct pager_config *c = data; @@ -558,7 +575,7 @@ int main(int argc, const char **argv) srandom(time(NULL)); -perf_config(perf_default_config, NULL); +perf_config_init(); set_buildid_dir(NULL); /* get debugfs/tracefs mount point from /proc/mounts */ diff --git a/tools/perf/ui/browser.c b/tools/perf/ui/browser.c index af68a9d..4b827c2 100644 --- a/tools/perf/ui/browser.c +++ b/tools/perf/ui/browser.c @@ -740,8 +740,6 @@ void ui_browser__init(void) { int i = 0; -perf_config(ui_browser__color_config, NULL); - while (ui_browser__colorsets[i].name) { struct ui_browser_colorset *c = &ui_browser__colorsets[i++]; sltt_set_color(c->colorset, c->name, c->fg, c->bg); ... (omitted) Many sub-commands call perf_config() at builtin-report.c, ui/browser.c and etc. So I think it is ambiguous to decide where perf_config_set__delete() will be called instead of at run_builtin(). (If the config set's life cycle is the same as perf's , I would call perf_config_set__delete() at run_builtin() because a sub-command is done) 2) If having the config set for perf's life, we would add configuration features to TUI like TUI configuring options for linux kernel compiling (e.g. make menuconfig) ? Of course, we don't need to have the same function as make menuconfig but IMHO, we could add similar way to menuconfig while perf is running. How do you feel about these ? Thanks, :) Taeung But we need to use the config set that already contains all config key-value pairs to avoid this repetitive work reading the config files in perf_config(). (the config set mean a global variable 'config_set') In other words, if new perf_config() is called, only first time 'config_set' is initialized collecting all configs from the config files and it work with perf_config_set__iter(). And free the config set after a sub-command work at run_builtin(). If we do, 'config_set' can be reused wherever using perf_config() and a feature of old perf_config() is the same as new perf_config() work without the repetitive work that read the config files. Cc: Namhyung Kim Cc: Jiri Olsa Cc: Wang Nan Cc: Peter Zijlstra Cc: Ingo Molnar Cc: Masami Hiramatsu Cc: Alexander Shishkin Signed-off-by: Taeung Song -
Re: [PATCH v8 2/5] perf config: Bring declarations about config from util/cache.h to util/config.h
Hi, Arnaldo :) Thank you for your review. On 06/09/2016 10:29 PM, Arnaldo Carvalho de Melo wrote: Em Wed, Jun 08, 2016 at 09:36:50PM +0900, Taeung Song escreveu: Lately util/config.h has been added but util/cache.h has declarations of functions and extern variable for config features. To manage codes about configuration at one spot, move them to util/config.h and util/cache.h include util/config.h Cc: Alexander Shishkin Cc: Jiri Olsa Cc: Masami Hiramatsu Cc: Namhyung Kim Cc: Wang Nan Signed-off-by: Taeung Song --- tools/perf/util/cache.h | 13 + tools/perf/util/config.h | 12 2 files changed, 13 insertions(+), 12 deletions(-) diff --git a/tools/perf/util/cache.h b/tools/perf/util/cache.h index 0d814bb..cd8cd4d 100644 --- a/tools/perf/util/cache.h +++ b/tools/perf/util/cache.h @@ -7,6 +7,7 @@ #include #include "../perf.h" #include "../ui/ui.h" +#include "config.h" Why have you added that? Are those config functions used in cache.h? Yes, it does. Many source files include cache.h e.g. builtin-annoate.c, util/color.c, builtin-report.c and etc. And They can use perf_config() function including this header file. So, If I totally eliminate not only declarations about config but also #include "util/config.h" at util/cache.h, we should add '#include "util/config.h"' to each source file that need perf_config() overall. But IMHO, if cache.h include config.h, we don't need to find source files that use perf_config(). Thanks, Taeung
Re: [PATCH v7 3/7] perf config: Add global variable 'config_set'
My answer was too long.. To be short, many sub-commands use perf_config() so wherever new perf_config() is called, the config set that already contains all configs from config files should be shared. And when a sub-command is done, we need to free the config set at run_builtin() as below. So, IMHO we need to use 'config_set' as a global variable. Anyway I sent v8 recasting this patchset with other way. (But 'config_set' is still a global variable.) Please, reconsider it again. Thanks, Taeung On 06/08/2016 12:14 AM, Taeung Song wrote: Good afternoon :) On 06/07/2016 11:05 PM, Arnaldo Carvalho de Melo wrote: Em Tue, Jun 07, 2016 at 06:26:13PM +0900, Taeung Song escreveu: The config set is prepared by collecting all configs from config files (i.e. user config ~/.perfconfig and system config $(sysconfdir)/perfconfig) so the config set contains all config key-value pairs. We need to use it as global variable to share it. We should generally avoid global variables, and in this case, from looking just at this patch, I'm not convinced we need to introduce more global variables, isn't this object instantiated and deleted at the cmd_config() function? So, can't we just pass it to any function needing to access it? The reason why I added global variable 'config_set' that all config key-value pairs from config files is because of upcoming perf_config_set__delete() and perf_config(). This variable can be used not only at cmd_config() but also at new perf_config() and future perf_config_set__delete(). Wherever new perf_config() is called, this function check whether the global variable 'config_set' is initialized or not. And this function use the variable 'config_set' like below. (One of intended purposes of new perf_config() is to avoid the repetitive work that read config files, every time it is called and to reuse the global variable 'config_set' that already contains all config info.) +int perf_config(config_fn_t fn, void *data) +{ + if (config_set == NULL) + config_set = perf_config_set__new(); + + return perf_config_set__iter(config_set, fn, data); +} Finally, after sub-command finished, called perf_config_set__delete() use the global variable as argument to free it as below. diff --git a/tools/perf/perf.c b/tools/perf/perf.c index 15982ce..058d5dc 100644 --- a/tools/perf/perf.c +++ b/tools/perf/perf.c @@ -391,6 +391,7 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv) perf_env__set_cmdline(&perf_env, argc, argv); status = p->fn(argc, argv, prefix); + perf_config_set__delete(&config_set); exit_browser(status); perf_env__exit(&perf_env); bpf__clear(); Sure, we can avoid 'extern' for the global variable 'config_set'. But IMHO, we may need to use a global variable 'config_set' at only util/config.c at least. (If perf_config_set__delete() don't take arguments) (But it seems natural that we should pair between destructor and constructor as you said. http://marc.info/?l=linux-kernel&m=146463657721848&w=2 ) And of course, there are other way e.g. modifying arguments of perf_config(), using perf_config_set__new() at the very beginning at main() or etc... Nevertheless, if we haven't to use the global variable, I'd find other way. I applied the first two patches. Thank you! Taeung And in near future, the variable will be handled in perf_config() and other functions at util/config.c Cc: Namhyung Kim Cc: Jiri Olsa Cc: Masami Hiramatsu Cc: Alexander Shishkin Signed-off-by: Taeung Song --- tools/perf/builtin-config.c | 9 - tools/perf/util/config.c| 1 + tools/perf/util/config.h| 2 ++ 3 files changed, 7 insertions(+), 5 deletions(-) diff --git a/tools/perf/builtin-config.c b/tools/perf/builtin-config.c index fe1b77f..b3bc01a 100644 --- a/tools/perf/builtin-config.c +++ b/tools/perf/builtin-config.c @@ -62,7 +62,6 @@ static int show_config(struct perf_config_set *set) int cmd_config(int argc, const char **argv, const char *prefix __maybe_unused) { int ret = 0; -struct perf_config_set *set; char *user_config = mkpath("%s/.perfconfig", getenv("HOME")); argc = parse_options(argc, argv, config_options, config_usage, @@ -80,8 +79,8 @@ int cmd_config(int argc, const char **argv, const char *prefix __maybe_unused) else if (use_user_config) config_exclusive_filename = user_config; -set = perf_config_set__new(); -if (!set) { +config_set = perf_config_set__new(); +if (!config_set) { ret = -1; goto out_err; } @@ -92,7 +91,7 @@ int cmd_config(int argc, const char **argv, const char *prefix __maybe_unused) pr_err("Error: takes no arguments\n"); parse_options_usage(config_usage, config_op
[RFC][PATCH v8 0/5] perf config: Reimplement perf_config() using perf_config_set__inter()
Hello, :) This patchset is to reimplement perf_config() for efficient config management. Everytime perf_config() is called, perf_config() always read config files. (i.e. user config '~/.perfconfig' and system config '$(sysconfdir)/perfconfig') But we need to use 'struct perf_config_set config_set' variable that already contains all config key-value pairs to avoid this repetitive work in perf_config(). In other words, if new perf_config() is called, only first time 'config_set' is initialized collecting all configs from config files and it work with perf_config_set__iter(). If we do, 'config_set' can be reused wherever using perf_config() and a feature of old perf_config() is the same as new perf_config() work without the repetitive work that read the config files. IMHO, I think this patchset is needed because not only the repetitive work should be avoided but also in near future, it would be smooth to manage perf configs. If you give me any feedback, I'd apprecicated it. :) Thanks, Taeung v8: - handle the error about NULL at perf_config_set__delete() - bring declarations about config from util/config.h to util/config.h - reimplement show_config() using perf_config_set__iter() instead of perf_config() - rebased onto perf-core-for-mingo-20160607 - applied ([PATCH v7 1/7] 25d8f48, [PATCH v7 2/7] 8beeb00) v7: - fill a missing crumb that assign NULL to 'set' variable in perf_config_set__new() (Arnaldo) - two patches applied ([PATCH v6 1/9] 78f71c9, [PATCH v6 3/9] 7db91f2) v6: - add printing error message when perf_config_set__iter() is failed - modify commit messages for bugfix 1~3 (PATCH 1/9 ~ 3/9) to help reviewers easily understand why them is needed v5: - solve the leak when perf_config_set__init() failed (Arnaldo) (to clear the problem it is needed to apply the bottom bugfix 1~3 patches) - bugfix 1) fix the problem of abnormal terminaltion at perf_parse_file() called by perf_config() - bugfix 2) if failed at collect_config(), finally free a config set after it is done instead of freeing the config set in the function - bugfix 3) handle NULL pointer exception of 'set' at collect_config() v4: - Keep perf_config_set__delete() as it is (Arnaldo) - Remove perf_config_set__check() (Arnaldo) - Keep the existing code about the config set at cmd_config() (Arnaldo) v3: - add freeing config set after sub-command work at run_builtin() (Namhyung) - remove needless code about the config set at cmd_config() - add a patch about a global variable 'config_set' v2: - split a patch into several patches - reimplement show_config() using new perf_config() - modify perf_config_set__delete using global variable 'config_set' - reset config set when only 'config' sub-commaned work because of options for config file location Taeung Song (5): perf config: Handle the error about NULL at perf_config_set__delete() perf config: Bring declarations about config from util/cache.h to util/config.h perf config: Reimplement perf_config() using perf_config_set__iter() perf config: Use zfree() instead of free() at perf_config_set__delete() perf config: Reimplement show_config() using perf_config_set__iter() tools/perf/builtin-config.c | 35 ++-- tools/perf/perf.c | 1 + tools/perf/util/cache.h | 13 +- tools/perf/util/config.c| 98 ++--- tools/perf/util/config.h| 16 +++- 5 files changed, 78 insertions(+), 85 deletions(-) -- 2.5.0
[PATCH v8 3/5] perf config: Reimplement perf_config() using perf_config_set__iter()
Many sub-commands use perf_config() so everytime perf_config() is called, perf_config() always read config files. (i.e. user config '~/.perfconfig' and system config '$(sysconfdir)/perfconfig') But we need to use the config set that already contains all config key-value pairs to avoid this repetitive work reading the config files in perf_config(). (the config set mean a global variable 'config_set') In other words, if new perf_config() is called, only first time 'config_set' is initialized collecting all configs from the config files and it work with perf_config_set__iter(). And free the config set after a sub-command work at run_builtin(). If we do, 'config_set' can be reused wherever using perf_config() and a feature of old perf_config() is the same as new perf_config() work without the repetitive work that read the config files. Cc: Namhyung Kim Cc: Jiri Olsa Cc: Wang Nan Cc: Peter Zijlstra Cc: Ingo Molnar Cc: Masami Hiramatsu Cc: Alexander Shishkin Signed-off-by: Taeung Song --- tools/perf/builtin-config.c | 4 +++ tools/perf/perf.c | 1 + tools/perf/util/config.c| 87 ++--- tools/perf/util/config.h| 1 + 4 files changed, 48 insertions(+), 45 deletions(-) diff --git a/tools/perf/builtin-config.c b/tools/perf/builtin-config.c index fe1b77f..cfd1036 100644 --- a/tools/perf/builtin-config.c +++ b/tools/perf/builtin-config.c @@ -80,6 +80,10 @@ int cmd_config(int argc, const char **argv, const char *prefix __maybe_unused) else if (use_user_config) config_exclusive_filename = user_config; + /* +* At only 'config' sub-command, individually use the config set +* because of reinitializing with options config file location. +*/ set = perf_config_set__new(); if (!set) { ret = -1; diff --git a/tools/perf/perf.c b/tools/perf/perf.c index 15982ce..fe2ab7c 100644 --- a/tools/perf/perf.c +++ b/tools/perf/perf.c @@ -391,6 +391,7 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv) perf_env__set_cmdline(&perf_env, argc, argv); status = p->fn(argc, argv, prefix); + perf_config_set__delete(config_set); exit_browser(status); perf_env__exit(&perf_env); bpf__clear(); diff --git a/tools/perf/util/config.c b/tools/perf/util/config.c index 31e09a4..72db134 100644 --- a/tools/perf/util/config.c +++ b/tools/perf/util/config.c @@ -28,6 +28,7 @@ static int config_linenr; static int config_file_eof; const char *config_exclusive_filename; +struct perf_config_set *config_set; static int get_next_char(void) { @@ -478,51 +479,6 @@ static int perf_config_global(void) return !perf_env_bool("PERF_CONFIG_NOGLOBAL", 0); } -int perf_config(config_fn_t fn, void *data) -{ - int ret = -1; - const char *home = NULL; - - /* Setting $PERF_CONFIG makes perf read _only_ the given config file. */ - if (config_exclusive_filename) - return perf_config_from_file(fn, config_exclusive_filename, data); - if (perf_config_system() && !access(perf_etc_perfconfig(), R_OK)) { - if (perf_config_from_file(fn, perf_etc_perfconfig(), data) < 0) - goto out; - } - - home = getenv("HOME"); - if (perf_config_global() && home) { - char *user_config = strdup(mkpath("%s/.perfconfig", home)); - struct stat st; - - if (user_config == NULL) { - warning("Not enough memory to process %s/.perfconfig, " - "ignoring it.", home); - goto out; - } - - if (stat(user_config, &st) < 0) - goto out_free; - - if (st.st_uid && (st.st_uid != geteuid())) { - warning("File %s not owned by current user or root, " - "ignoring it.", user_config); - goto out_free; - } - - if (!st.st_size) - goto out_free; - - ret = perf_config_from_file(fn, user_config, data); - -out_free: - free(user_config); - } -out: - return ret; -} - static struct perf_config_section *find_section(struct list_head *sections, const char *section_name) { @@ -706,6 +662,47 @@ struct perf_config_set *perf_config_set__new(void) return set; } +static int perf_config_set__iter(struct perf_config_set *set, config_fn_t fn, void *data) +{ + struct perf_config_section *section; + struct perf_config_item *item; + struct list_head *sections; + char key[BUFSIZ]; + + if (set == NULL) +
[PATCH v8 2/5] perf config: Bring declarations about config from util/cache.h to util/config.h
Lately util/config.h has been added but util/cache.h has declarations of functions and extern variable for config features. To manage codes about configuration at one spot, move them to util/config.h and util/cache.h include util/config.h Cc: Alexander Shishkin Cc: Jiri Olsa Cc: Masami Hiramatsu Cc: Namhyung Kim Cc: Wang Nan Signed-off-by: Taeung Song --- tools/perf/util/cache.h | 13 + tools/perf/util/config.h | 12 2 files changed, 13 insertions(+), 12 deletions(-) diff --git a/tools/perf/util/cache.h b/tools/perf/util/cache.h index 0d814bb..cd8cd4d 100644 --- a/tools/perf/util/cache.h +++ b/tools/perf/util/cache.h @@ -7,6 +7,7 @@ #include #include "../perf.h" #include "../ui/ui.h" +#include "config.h" #include @@ -23,18 +24,6 @@ #define PERF_TRACEFS_ENVIRONMENT "PERF_TRACEFS_DIR" #define PERF_PAGER_ENVIRONMENT "PERF_PAGER" -extern const char *config_exclusive_filename; - -typedef int (*config_fn_t)(const char *, const char *, void *); -int perf_default_config(const char *, const char *, void *); -int perf_config(config_fn_t fn, void *); -int perf_config_int(const char *, const char *); -u64 perf_config_u64(const char *, const char *); -int perf_config_bool(const char *, const char *); -int config_error_nonbool(const char *); -const char *perf_config_dirname(const char *, const char *); -const char *perf_etc_perfconfig(void); - char *alias_lookup(const char *alias); int split_cmdline(char *cmdline, const char ***argv); diff --git a/tools/perf/util/config.h b/tools/perf/util/config.h index 22ec626..35ccddb 100644 --- a/tools/perf/util/config.h +++ b/tools/perf/util/config.h @@ -20,6 +20,18 @@ struct perf_config_set { struct list_head sections; }; +extern const char *config_exclusive_filename; + +typedef int (*config_fn_t)(const char *, const char *, void *); +int perf_default_config(const char *, const char *, void *); +int perf_config(config_fn_t fn, void *); +int perf_config_int(const char *, const char *); +u64 perf_config_u64(const char *, const char *); +int perf_config_bool(const char *, const char *); +int config_error_nonbool(const char *); +const char *perf_config_dirname(const char *, const char *); +const char *perf_etc_perfconfig(void); + struct perf_config_set *perf_config_set__new(void); void perf_config_set__delete(struct perf_config_set *set); -- 2.5.0
[PATCH v8 5/5] perf config: Reimplement show_config() using perf_config_set__iter()
Old show_config() directly use config set so there are many duplicated code with perf_config_set__iter(). So reimplement show_config() using perf_config() that use perf_config_set__iter() with config set that already contains all configs. Cc: Namhyung Kim Cc: Jiri Olsa Cc: Masami Hiramatsu Cc: Alexander Shishkin Cc: Wang Nan Signed-off-by: Taeung Song --- tools/perf/builtin-config.c | 29 +++-- tools/perf/util/config.c| 2 +- tools/perf/util/config.h| 1 + 3 files changed, 9 insertions(+), 23 deletions(-) diff --git a/tools/perf/builtin-config.c b/tools/perf/builtin-config.c index c07f744..9c654f9 100644 --- a/tools/perf/builtin-config.c +++ b/tools/perf/builtin-config.c @@ -33,28 +33,13 @@ static struct option config_options[] = { OPT_END() }; -static int show_config(struct perf_config_set *set) +static int show_config(const char *key, const char *value, + void *cb __maybe_unused) { - struct perf_config_section *section; - struct perf_config_item *item; - struct list_head *sections; - - if (set == NULL) - return -1; - - sections = &set->sections; - if (list_empty(sections)) - return -1; - - list_for_each_entry(section, sections, node) { - list_for_each_entry(item, §ion->items, node) { - char *value = item->value; - - if (value) - printf("%s.%s=%s\n", section->name, - item->name, value); - } - } + if (value) + printf("%s=%s\n", key, value); + else + printf("%s\n", key); return 0; } @@ -96,7 +81,7 @@ int cmd_config(int argc, const char **argv, const char *prefix __maybe_unused) pr_err("Error: takes no arguments\n"); parse_options_usage(config_usage, config_options, "l", 1); } else { - ret = show_config(set); + ret = perf_config_set__iter(set, show_config, NULL); if (ret < 0) { const char * config_filename = config_exclusive_filename; if (!config_exclusive_filename) diff --git a/tools/perf/util/config.c b/tools/perf/util/config.c index 23fb8e4..4775d18 100644 --- a/tools/perf/util/config.c +++ b/tools/perf/util/config.c @@ -662,7 +662,7 @@ struct perf_config_set *perf_config_set__new(void) return set; } -static int perf_config_set__iter(struct perf_config_set *set, config_fn_t fn, void *data) +int perf_config_set__iter(struct perf_config_set *set, config_fn_t fn, void *data) { struct perf_config_section *section; struct perf_config_item *item; diff --git a/tools/perf/util/config.h b/tools/perf/util/config.h index fafba86..3526bb7 100644 --- a/tools/perf/util/config.h +++ b/tools/perf/util/config.h @@ -35,5 +35,6 @@ const char *perf_etc_perfconfig(void); struct perf_config_set *perf_config_set__new(void); void perf_config_set__delete(struct perf_config_set **set); +int perf_config_set__iter(struct perf_config_set *set, config_fn_t fn, void *data); #endif /* __PERF_CONFIG_H */ -- 2.5.0
[PATCH v8 4/5] perf config: Use zfree() instead of free() at perf_config_set__delete()
perf_config_set__delete() delete allocated the config set but the global variable 'config_set' is used all around. So purge and zfree by an address of the global variable , i.e. 'struct perf_config_set **' type instead of using local variable 'set' of which type is 'struct perf_config_set *'. Cc: Namhyung Kim Cc: Jiri Olsa Cc: Masami Hiramatsu Cc: Alexander Shishkin Cc: Wang Nan Signed-off-by: Taeung Song --- tools/perf/builtin-config.c | 2 +- tools/perf/perf.c | 2 +- tools/perf/util/config.c| 10 +- tools/perf/util/config.h| 2 +- 4 files changed, 8 insertions(+), 8 deletions(-) diff --git a/tools/perf/builtin-config.c b/tools/perf/builtin-config.c index cfd1036..c07f744 100644 --- a/tools/perf/builtin-config.c +++ b/tools/perf/builtin-config.c @@ -110,7 +110,7 @@ int cmd_config(int argc, const char **argv, const char *prefix __maybe_unused) usage_with_options(config_usage, config_options); } - perf_config_set__delete(set); + perf_config_set__delete(&set); out_err: return ret; } diff --git a/tools/perf/perf.c b/tools/perf/perf.c index fe2ab7c..058d5dc 100644 --- a/tools/perf/perf.c +++ b/tools/perf/perf.c @@ -391,7 +391,7 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv) perf_env__set_cmdline(&perf_env, argc, argv); status = p->fn(argc, argv, prefix); - perf_config_set__delete(config_set); + perf_config_set__delete(&config_set); exit_browser(status); perf_env__exit(&perf_env); bpf__clear(); diff --git a/tools/perf/util/config.c b/tools/perf/util/config.c index 72db134..23fb8e4 100644 --- a/tools/perf/util/config.c +++ b/tools/perf/util/config.c @@ -654,7 +654,7 @@ struct perf_config_set *perf_config_set__new(void) if (set) { INIT_LIST_HEAD(&set->sections); if (perf_config_set__init(set) < 0) { - perf_config_set__delete(set); + perf_config_set__delete(&set); set = NULL; } } @@ -737,13 +737,13 @@ static void perf_config_set__purge(struct perf_config_set *set) } } -void perf_config_set__delete(struct perf_config_set *set) +void perf_config_set__delete(struct perf_config_set **set) { - if (set == NULL) + if (*set == NULL) return; - perf_config_set__purge(set); - free(set); + perf_config_set__purge(*set); + zfree(set); } /* diff --git a/tools/perf/util/config.h b/tools/perf/util/config.h index 7cc4fea..fafba86 100644 --- a/tools/perf/util/config.h +++ b/tools/perf/util/config.h @@ -34,6 +34,6 @@ const char *perf_config_dirname(const char *, const char *); const char *perf_etc_perfconfig(void); struct perf_config_set *perf_config_set__new(void); -void perf_config_set__delete(struct perf_config_set *set); +void perf_config_set__delete(struct perf_config_set **set); #endif /* __PERF_CONFIG_H */ -- 2.5.0
[BUGFIX][PATCH v8 1/5] perf config: Handle the error about NULL at perf_config_set__delete()
perf_config_set__delete() purge and free the config set that contains all config key-value pairs. But if the config set (i.e. 'set' variable at the function) is NULL, this is wrong so handle it. Cc: Alexander Shishkin Cc: Jiri Olsa Cc: Masami Hiramatsu Cc: Namhyung Kim Cc: Wang Nan Signed-off-by: Taeung Song --- tools/perf/util/config.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/tools/perf/util/config.c b/tools/perf/util/config.c index 8749eca..31e09a4 100644 --- a/tools/perf/util/config.c +++ b/tools/perf/util/config.c @@ -742,6 +742,9 @@ static void perf_config_set__purge(struct perf_config_set *set) void perf_config_set__delete(struct perf_config_set *set) { + if (set == NULL) + return; + perf_config_set__purge(set); free(set); } -- 2.5.0
[tip:perf/core] perf config: Constructor should free its allocated memory when failing
Commit-ID: 25d8f48f78f37dd6af08bd11212ab3d53a4c8cc6 Gitweb: http://git.kernel.org/tip/25d8f48f78f37dd6af08bd11212ab3d53a4c8cc6 Author: Taeung Song AuthorDate: Tue, 7 Jun 2016 18:26:11 +0900 Committer: Arnaldo Carvalho de Melo CommitDate: Tue, 7 Jun 2016 10:58:55 -0300 perf config: Constructor should free its allocated memory when failing Because of die() at perf_parse_file() a config set was freed in collect_config(), if failed. But it is natural to free a config set after collect_config() is done when some problems happened. So, in case of failure, lastly free a config set at perf_config_set__new() instead of freeing the config set in collect_config(). Signed-off-by: Taeung Song Cc: Alexander Shishkin Cc: Jiri Olsa Cc: Masami Hiramatsu Cc: Namhyung Kim Cc: Peter Zijlstra Link: http://lkml.kernel.org/r/1465291577-20973-2-git-send-email-treeze.tae...@gmail.com Signed-off-by: Arnaldo Carvalho de Melo --- tools/perf/util/config.c | 6 -- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/tools/perf/util/config.c b/tools/perf/util/config.c index c73f1c4..e086f59 100644 --- a/tools/perf/util/config.c +++ b/tools/perf/util/config.c @@ -643,7 +643,6 @@ static int collect_config(const char *var, const char *value, out_free: free(key); - perf_config_set__delete(set); return -1; } @@ -653,7 +652,10 @@ struct perf_config_set *perf_config_set__new(void) if (set) { INIT_LIST_HEAD(&set->sections); - perf_config(collect_config, set); + if (perf_config(collect_config, set) < 0) { + perf_config_set__delete(set); + set = NULL; + } } return set;
[tip:perf/core] perf config: Use new perf_config_set__init() to initialize config set
Commit-ID: 8beeb00f2c8498686eee02b8edcd1488b903c90b Gitweb: http://git.kernel.org/tip/8beeb00f2c8498686eee02b8edcd1488b903c90b Author: Taeung Song AuthorDate: Tue, 7 Jun 2016 18:26:12 +0900 Committer: Arnaldo Carvalho de Melo CommitDate: Tue, 7 Jun 2016 11:01:25 -0300 perf config: Use new perf_config_set__init() to initialize config set Instead of perf_config(), this function initializes config set by reading various files: user config ~/.perfconfig and system config $(sysconfdir)/perfconfig). If there are the same config variable in both user and system config files, user config has higher priority than system config. Signed-off-by: Taeung Song Cc: Alexander Shishkin Cc: Jiri Olsa Cc: Masami Hiramatsu Cc: Namhyung Kim Cc: Peter Zijlstra Link: http://lkml.kernel.org/r/1465291577-20973-3-git-send-email-treeze.tae...@gmail.com Signed-off-by: Arnaldo Carvalho de Melo --- tools/perf/util/config.c | 47 ++- 1 file changed, 46 insertions(+), 1 deletion(-) diff --git a/tools/perf/util/config.c b/tools/perf/util/config.c index e086f59..8749eca 100644 --- a/tools/perf/util/config.c +++ b/tools/perf/util/config.c @@ -646,13 +646,58 @@ out_free: return -1; } +static int perf_config_set__init(struct perf_config_set *set) +{ + int ret = -1; + const char *home = NULL; + + /* Setting $PERF_CONFIG makes perf read _only_ the given config file. */ + if (config_exclusive_filename) + return perf_config_from_file(collect_config, config_exclusive_filename, set); + if (perf_config_system() && !access(perf_etc_perfconfig(), R_OK)) { + if (perf_config_from_file(collect_config, perf_etc_perfconfig(), set) < 0) + goto out; + } + + home = getenv("HOME"); + if (perf_config_global() && home) { + char *user_config = strdup(mkpath("%s/.perfconfig", home)); + struct stat st; + + if (user_config == NULL) { + warning("Not enough memory to process %s/.perfconfig, " + "ignoring it.", home); + goto out; + } + + if (stat(user_config, &st) < 0) + goto out_free; + + if (st.st_uid && (st.st_uid != geteuid())) { + warning("File %s not owned by current user or root, " + "ignoring it.", user_config); + goto out_free; + } + + if (!st.st_size) + goto out_free; + + ret = perf_config_from_file(collect_config, user_config, set); + +out_free: + free(user_config); + } +out: + return ret; +} + struct perf_config_set *perf_config_set__new(void) { struct perf_config_set *set = zalloc(sizeof(*set)); if (set) { INIT_LIST_HEAD(&set->sections); - if (perf_config(collect_config, set) < 0) { + if (perf_config_set__init(set) < 0) { perf_config_set__delete(set); set = NULL; }
[tip:perf/core] perf config: Handle the error when config set is NULL at collect_config()
Commit-ID: 7db91f251056f90fec4121f028680ab3153a0f3c Gitweb: http://git.kernel.org/tip/7db91f251056f90fec4121f028680ab3153a0f3c Author: Taeung Song AuthorDate: Mon, 6 Jun 2016 19:52:54 +0900 Committer: Arnaldo Carvalho de Melo CommitDate: Mon, 6 Jun 2016 17:43:19 -0300 perf config: Handle the error when config set is NULL at collect_config() collect_config() collect all config key-value pairs from config files and put each config info in config set. But if config set (i.e. 'set' variable at collect_config()) is NULL, this is wrong so handle it. Signed-off-by: Taeung Song Cc: Alexander Shishkin Cc: Jiri Olsa Cc: Masami Hiramatsu Cc: Namhyung Kim Cc: Peter Zijlstra Link: http://lkml.kernel.org/r/1465210380-26749-4-git-send-email-treeze.tae...@gmail.com Signed-off-by: Arnaldo Carvalho de Melo --- tools/perf/util/config.c | 6 +- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/tools/perf/util/config.c b/tools/perf/util/config.c index b500737..c73f1c4 100644 --- a/tools/perf/util/config.c +++ b/tools/perf/util/config.c @@ -607,8 +607,12 @@ static int collect_config(const char *var, const char *value, struct perf_config_section *section = NULL; struct perf_config_item *item = NULL; struct perf_config_set *set = perf_config_set; - struct list_head *sections = &set->sections; + struct list_head *sections; + if (set == NULL) + return -1; + + sections = &set->sections; key = ptr = strdup(var); if (!key) { pr_debug("%s: strdup failed\n", __func__);
[tip:perf/core] perf config: Fix abnormal termination at perf_parse_file()
Commit-ID: 78f71c996fb92d129ec75d572e2f5367a4f4c757 Gitweb: http://git.kernel.org/tip/78f71c996fb92d129ec75d572e2f5367a4f4c757 Author: Taeung Song AuthorDate: Mon, 6 Jun 2016 19:52:52 +0900 Committer: Arnaldo Carvalho de Melo CommitDate: Mon, 6 Jun 2016 17:43:17 -0300 perf config: Fix abnormal termination at perf_parse_file() If a config file has wrong key-value pairs, the perf process will be forcibly terminated by die() at perf_parse_file() called by perf_config() so terminal settings can be crushed because of unusual termination. For example: If user config file has a wrong value 'red;default' instead of a normal value like 'red, default' for a key 'colors.top', # cat ~/.perfconfig [colors] medium = red;default # wrong value and if running sub-command 'top', # perf top perf process is dead by force and terminal setting is broken with a messge like below. Fatal: bad config file line 2 in /root/.perfconfig So fix it. If perf_config() can return on failure without calling die() at perf_parse_file(), this problem can be solved. And if a config file has wrong values, show the error message and then use default config values instead of wrong config values. Signed-off-by: Taeung Song Tested-by: Arnaldo Carvalho de Melo Cc: Alexander Shishkin Cc: Jiri Olsa Cc: Masami Hiramatsu Cc: Namhyung Kim Cc: Peter Zijlstra Link: http://lkml.kernel.org/r/1465210380-26749-2-git-send-email-treeze.tae...@gmail.com Signed-off-by: Arnaldo Carvalho de Melo --- tools/perf/util/config.c | 16 +++- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/tools/perf/util/config.c b/tools/perf/util/config.c index dad7d82..b500737 100644 --- a/tools/perf/util/config.c +++ b/tools/perf/util/config.c @@ -275,7 +275,8 @@ static int perf_parse_file(config_fn_t fn, void *data) break; } } - die("bad config file line %d in %s", config_linenr, config_file_name); + pr_err("bad config file line %d in %s\n", config_linenr, config_file_name); + return -1; } static int parse_unit_factor(const char *end, unsigned long *val) @@ -479,16 +480,15 @@ static int perf_config_global(void) int perf_config(config_fn_t fn, void *data) { - int ret = 0, found = 0; + int ret = -1; const char *home = NULL; /* Setting $PERF_CONFIG makes perf read _only_ the given config file. */ if (config_exclusive_filename) return perf_config_from_file(fn, config_exclusive_filename, data); if (perf_config_system() && !access(perf_etc_perfconfig(), R_OK)) { - ret += perf_config_from_file(fn, perf_etc_perfconfig(), - data); - found += 1; + if (perf_config_from_file(fn, perf_etc_perfconfig(), data) < 0) + goto out; } home = getenv("HOME"); @@ -514,14 +514,12 @@ int perf_config(config_fn_t fn, void *data) if (!st.st_size) goto out_free; - ret += perf_config_from_file(fn, user_config, data); - found += 1; + ret = perf_config_from_file(fn, user_config, data); + out_free: free(user_config); } out: - if (found == 0) - return -1; return ret; }
Re: [GIT PULL 00/24] perf/core improvements and fixes
Hi, Arnaldo I found something weird about perf/core branch on your repository. (I don't know whether it is just my illusion or not) I can't pull new commits on top of perf-core-for-mingo-20160606 by normal way as below # git remote show acme * remote acme Fetch URL: git://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git # git log --oneline edb13ed tools lib bpf: Rename set_private() to set_priv() be834ff tools lib bpf: Make bpf_program__get_private() use IS_ERR() ... # git pull acme perf/core From git://git.kernel.org/pub/scm/linux/kernel/git/acme/linux * branchperf/core -> FETCH_HEAD Already up-to-date. And then nothing changed, I didn't also find new commits and new tag 'perf-core-for-mingo-20160607'. However, if using tag perf-core-for-mingo-20160607, I can get new commits from your repository as below. # git fetch acme --tags remote: Counting objects: 4888, done. remote: Compressing objects: 100% (4800/4800), done. remote: Total 4888 (delta 266), reused 1212 (delta 59) Receiving objects: 100% (4888/4888), 21.36 MiB | 3.72 MiB/s, done. Resolving deltas: 100% (266/266), done. From git://git.kernel.org/pub/scm/linux/kernel/git/acme/linux * [new tag] perf-core-for-mingo-20160307 -> perf-core-for-mingo-20160307 * [new tag] perf-core-for-mingo-20160329 -> perf-core-for-mingo-20160329 * [new tag] perf-core-for-mingo-20160407 -> perf-core-for-mingo-20160407 * [new tag] perf-core-for-mingo-20160607 -> perf-core-for-mingo-20160607 * [new tag] perf-ebpf-for-mingo -> perf-ebpf-for-mingo * [new tag] perf-urgent-for-mingo-20160510 -> perf-urgent-for-mingo-20160510 * [new tag] v2.6.11-> v2.6.11 * [new tag] v2.6.11-tree -> v2.6.11-tree But there is a strange thing about git branch. I can't find which branch is that have tag perf-core-for-mingo-20160607 like below. # git branch -a --contains perf-core-for-mingo-20160607 As the final outcome, I got new commits on top of perf-core-for-mingo-20160606 directly using a tag 'perf-core-for-mingo-20160607' as below. # git reset --hard perf-core-for-mingo-20160607 HEAD is now at 057fbfb perf callchain: Support aarch64 cross-platform But isn't it a problem ? Just use a tag? Thanks, Taeung On 06/08/2016 05:04 AM, Arnaldo Carvalho de Melo wrote: Hi Ingo, Please consider pulling, this is on top of perf-core-for-mingo-20160606, Thanks, - Arnaldo The following changes since commit 7db91f251056f90fec4121f028680ab3153a0f3c: perf config: Handle the error when config set is NULL at collect_config() (2016-06-06 17:43:19 -0300) are available in the git repository at: git://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git tags/perf-core-for-mingo-20160607 for you to fetch changes up to 057fbfb25cde4a368418f3f720cdc31d48800c4d: perf callchain: Support aarch64 cross-platform (2016-06-07 15:13:35 -0300) perf/core improvements and fixes: User visible: - Support cross unwinding, i.e. collecting '--call-graph dwarf' perf.data files in one machine and then doing analysis in another machine of a different hardware architecture. This enables, for instance, to do: perf record -a --call-graph dwarf on a x86-32 or aarch64 system and then do 'perf report' on it on a x86_64 workstation. (He Kuang) - Fix crash in build_id_cache__kallsyms_path(), recent regression (Wang Nan) Infrastructure: - Make tools/lib/bpf use the IS_ERR return facility consistently and also stop using the _get_ term for non-reference count methods (Arnaldo Carvalho de Melo) - 'perf config' refactorings (Taeung Song) Signed-off-by: Arnaldo Carvalho de Melo Arnaldo Carvalho de Melo (7): tools lib bpf: Use IS_ERR() reporting macros with bpf_map__get_private() tools lib bpf: Rename bpf_map__get_name() to bpf_map__name() tools lib bpf: Use IS_ERR() reporting macros with bpf_map__get_def() tools lib bpf: Rename bpf_map__get_fd() to bpf_map__fd() tools lib bpf: Remove _get_ from non-refcount method names tools lib bpf: Make bpf_program__get_private() use IS_ERR() tools lib bpf: Rename set_private() to set_priv() He Kuang (14): perf unwind: Use LIBUNWIND_DIR for remote libunwind feature check perf unwind: Decouple thread->address_space on libunwind perf unwind: Introduce 'struct unwind_libunwind_ops' for local unwind perf unwind: Move unwind__prepare_access from thread_new into thread__insert_map perf unwind: Don't mix LIBUNWIND_LIBS into LIBUNWIND_LDFLAGS perf unwind: Separate local/remote libunwind config perf unwind: Rename unwind-libunwind.c to unwind-libunwind-local.c perf tools: E
Re: [PATCH v7 3/7] perf config: Add global variable 'config_set'
Good afternoon :) On 06/07/2016 11:05 PM, Arnaldo Carvalho de Melo wrote: Em Tue, Jun 07, 2016 at 06:26:13PM +0900, Taeung Song escreveu: The config set is prepared by collecting all configs from config files (i.e. user config ~/.perfconfig and system config $(sysconfdir)/perfconfig) so the config set contains all config key-value pairs. We need to use it as global variable to share it. We should generally avoid global variables, and in this case, from looking just at this patch, I'm not convinced we need to introduce more global variables, isn't this object instantiated and deleted at the cmd_config() function? So, can't we just pass it to any function needing to access it? The reason why I added global variable 'config_set' that all config key-value pairs from config files is because of upcoming perf_config_set__delete() and perf_config(). This variable can be used not only at cmd_config() but also at new perf_config() and future perf_config_set__delete(). Wherever new perf_config() is called, this function check whether the global variable 'config_set' is initialized or not. And this function use the variable 'config_set' like below. (One of intended purposes of new perf_config() is to avoid the repetitive work that read config files, every time it is called and to reuse the global variable 'config_set' that already contains all config info.) +int perf_config(config_fn_t fn, void *data) +{ + if (config_set == NULL) + config_set = perf_config_set__new(); + + return perf_config_set__iter(config_set, fn, data); +} Finally, after sub-command finished, called perf_config_set__delete() use the global variable as argument to free it as below. diff --git a/tools/perf/perf.c b/tools/perf/perf.c index 15982ce..058d5dc 100644 --- a/tools/perf/perf.c +++ b/tools/perf/perf.c @@ -391,6 +391,7 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv) perf_env__set_cmdline(&perf_env, argc, argv); status = p->fn(argc, argv, prefix); + perf_config_set__delete(&config_set); exit_browser(status); perf_env__exit(&perf_env); bpf__clear(); Sure, we can avoid 'extern' for the global variable 'config_set'. But IMHO, we may need to use a global variable 'config_set' at only util/config.c at least. (If perf_config_set__delete() don't take arguments) (But it seems natural that we should pair between destructor and constructor as you said. http://marc.info/?l=linux-kernel&m=146463657721848&w=2 ) And of course, there are other way e.g. modifying arguments of perf_config(), using perf_config_set__new() at the very beginning at main() or etc... Nevertheless, if we haven't to use the global variable, I'd find other way. I applied the first two patches. Thank you! Taeung And in near future, the variable will be handled in perf_config() and other functions at util/config.c Cc: Namhyung Kim Cc: Jiri Olsa Cc: Masami Hiramatsu Cc: Alexander Shishkin Signed-off-by: Taeung Song --- tools/perf/builtin-config.c | 9 - tools/perf/util/config.c| 1 + tools/perf/util/config.h| 2 ++ 3 files changed, 7 insertions(+), 5 deletions(-) diff --git a/tools/perf/builtin-config.c b/tools/perf/builtin-config.c index fe1b77f..b3bc01a 100644 --- a/tools/perf/builtin-config.c +++ b/tools/perf/builtin-config.c @@ -62,7 +62,6 @@ static int show_config(struct perf_config_set *set) int cmd_config(int argc, const char **argv, const char *prefix __maybe_unused) { int ret = 0; - struct perf_config_set *set; char *user_config = mkpath("%s/.perfconfig", getenv("HOME")); argc = parse_options(argc, argv, config_options, config_usage, @@ -80,8 +79,8 @@ int cmd_config(int argc, const char **argv, const char *prefix __maybe_unused) else if (use_user_config) config_exclusive_filename = user_config; - set = perf_config_set__new(); - if (!set) { + config_set = perf_config_set__new(); + if (!config_set) { ret = -1; goto out_err; } @@ -92,7 +91,7 @@ int cmd_config(int argc, const char **argv, const char *prefix __maybe_unused) pr_err("Error: takes no arguments\n"); parse_options_usage(config_usage, config_options, "l", 1); } else { - ret = show_config(set); + ret = show_config(config_set); if (ret < 0) { const char * config_filename = config_exclusive_filename; if (!config_exclusive_filename) @@ -106,7 +105,7 @@ int cmd_config(int argc, const char **argv, const char *prefix __maybe_unused) usage_with_options(config_usage, con
[PATCH v7 2/7] perf config: Use new perf_config_set__init() to initialize config set
Instead of perf_config(), This function initialize config set collecting all configs from config files (i.e. user config ~/.perfconfig and system config $(sysconfdir)/perfconfig). If there are the same config variable both user and system config file, user config has higher priority than system config. Cc: Namhyung Kim Cc: Jiri Olsa Cc: Masami Hiramatsu Cc: Alexander Shishkin Signed-off-by: Taeung Song --- tools/perf/util/config.c | 47 ++- 1 file changed, 46 insertions(+), 1 deletion(-) diff --git a/tools/perf/util/config.c b/tools/perf/util/config.c index e086f59..8749eca 100644 --- a/tools/perf/util/config.c +++ b/tools/perf/util/config.c @@ -646,13 +646,58 @@ out_free: return -1; } +static int perf_config_set__init(struct perf_config_set *set) +{ + int ret = -1; + const char *home = NULL; + + /* Setting $PERF_CONFIG makes perf read _only_ the given config file. */ + if (config_exclusive_filename) + return perf_config_from_file(collect_config, config_exclusive_filename, set); + if (perf_config_system() && !access(perf_etc_perfconfig(), R_OK)) { + if (perf_config_from_file(collect_config, perf_etc_perfconfig(), set) < 0) + goto out; + } + + home = getenv("HOME"); + if (perf_config_global() && home) { + char *user_config = strdup(mkpath("%s/.perfconfig", home)); + struct stat st; + + if (user_config == NULL) { + warning("Not enough memory to process %s/.perfconfig, " + "ignoring it.", home); + goto out; + } + + if (stat(user_config, &st) < 0) + goto out_free; + + if (st.st_uid && (st.st_uid != geteuid())) { + warning("File %s not owned by current user or root, " + "ignoring it.", user_config); + goto out_free; + } + + if (!st.st_size) + goto out_free; + + ret = perf_config_from_file(collect_config, user_config, set); + +out_free: + free(user_config); + } +out: + return ret; +} + struct perf_config_set *perf_config_set__new(void) { struct perf_config_set *set = zalloc(sizeof(*set)); if (set) { INIT_LIST_HEAD(&set->sections); - if (perf_config(collect_config, set) < 0) { + if (perf_config_set__init(set) < 0) { perf_config_set__delete(set); set = NULL; } -- 2.5.0
[PATCH v7 4/7] perf config: Use zfree() instead of free() at perf_config_set__delete()
perf_config_set__delete() delete allocated the config set but the global variable 'config_set' is used all around. So purge and zfree by an address of the global variable , i.e. 'struct perf_config_set **' type instead of using local variable 'set' of which type is 'struct perf_config_set *'. Cc: Namhyung Kim Cc: Jiri Olsa Cc: Masami Hiramatsu Cc: Alexander Shishkin Signed-off-by: Taeung Song --- tools/perf/builtin-config.c | 2 +- tools/perf/util/config.c| 11 +++ tools/perf/util/config.h| 2 +- 3 files changed, 9 insertions(+), 6 deletions(-) diff --git a/tools/perf/builtin-config.c b/tools/perf/builtin-config.c index b3bc01a..f23fe52 100644 --- a/tools/perf/builtin-config.c +++ b/tools/perf/builtin-config.c @@ -105,7 +105,7 @@ int cmd_config(int argc, const char **argv, const char *prefix __maybe_unused) usage_with_options(config_usage, config_options); } - perf_config_set__delete(config_set); + perf_config_set__delete(&config_set); out_err: return ret; } diff --git a/tools/perf/util/config.c b/tools/perf/util/config.c index 02fc6d5..2441585 100644 --- a/tools/perf/util/config.c +++ b/tools/perf/util/config.c @@ -699,7 +699,7 @@ struct perf_config_set *perf_config_set__new(void) if (set) { INIT_LIST_HEAD(&set->sections); if (perf_config_set__init(set) < 0) { - perf_config_set__delete(set); + perf_config_set__delete(&set); set = NULL; } } @@ -741,10 +741,13 @@ static void perf_config_set__purge(struct perf_config_set *set) } } -void perf_config_set__delete(struct perf_config_set *set) +void perf_config_set__delete(struct perf_config_set **set) { - perf_config_set__purge(set); - free(set); + if (*set == NULL) + return; + + perf_config_set__purge(*set); + zfree(set); } /* diff --git a/tools/perf/util/config.h b/tools/perf/util/config.h index ea157a4..271b429 100644 --- a/tools/perf/util/config.h +++ b/tools/perf/util/config.h @@ -23,6 +23,6 @@ struct perf_config_set { extern struct perf_config_set *config_set; struct perf_config_set *perf_config_set__new(void); -void perf_config_set__delete(struct perf_config_set *set); +void perf_config_set__delete(struct perf_config_set **set); #endif /* __PERF_CONFIG_H */ -- 2.5.0
[BUGFIX][PATCH v7 1/7] perf config: If collect_config() is failed, finally free a config set after it is done
Because of die() at perf_parse_file() a config set was freed in collect_config(), if failed. But it is natural to free a config set after collect_config() is done when some problems happened. So, in case of failure, lastly free a config set at perf_config_set__new() instead of freeing the config set in collect_config(). Cc: Namhyung Kim Cc: Jiri Olsa Cc: Masami Hiramatsu Cc: Alexander Shishkin Signed-off-by: Taeung Song --- tools/perf/util/config.c | 6 -- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/tools/perf/util/config.c b/tools/perf/util/config.c index c73f1c4..e086f59 100644 --- a/tools/perf/util/config.c +++ b/tools/perf/util/config.c @@ -643,7 +643,6 @@ static int collect_config(const char *var, const char *value, out_free: free(key); - perf_config_set__delete(set); return -1; } @@ -653,7 +652,10 @@ struct perf_config_set *perf_config_set__new(void) if (set) { INIT_LIST_HEAD(&set->sections); - perf_config(collect_config, set); + if (perf_config(collect_config, set) < 0) { + perf_config_set__delete(set); + set = NULL; + } } return set; -- 2.5.0
[PATCH v7 3/7] perf config: Add global variable 'config_set'
The config set is prepared by collecting all configs from config files (i.e. user config ~/.perfconfig and system config $(sysconfdir)/perfconfig) so the config set contains all config key-value pairs. We need to use it as global variable to share it. And in near future, the variable will be handled in perf_config() and other functions at util/config.c Cc: Namhyung Kim Cc: Jiri Olsa Cc: Masami Hiramatsu Cc: Alexander Shishkin Signed-off-by: Taeung Song --- tools/perf/builtin-config.c | 9 - tools/perf/util/config.c| 1 + tools/perf/util/config.h| 2 ++ 3 files changed, 7 insertions(+), 5 deletions(-) diff --git a/tools/perf/builtin-config.c b/tools/perf/builtin-config.c index fe1b77f..b3bc01a 100644 --- a/tools/perf/builtin-config.c +++ b/tools/perf/builtin-config.c @@ -62,7 +62,6 @@ static int show_config(struct perf_config_set *set) int cmd_config(int argc, const char **argv, const char *prefix __maybe_unused) { int ret = 0; - struct perf_config_set *set; char *user_config = mkpath("%s/.perfconfig", getenv("HOME")); argc = parse_options(argc, argv, config_options, config_usage, @@ -80,8 +79,8 @@ int cmd_config(int argc, const char **argv, const char *prefix __maybe_unused) else if (use_user_config) config_exclusive_filename = user_config; - set = perf_config_set__new(); - if (!set) { + config_set = perf_config_set__new(); + if (!config_set) { ret = -1; goto out_err; } @@ -92,7 +91,7 @@ int cmd_config(int argc, const char **argv, const char *prefix __maybe_unused) pr_err("Error: takes no arguments\n"); parse_options_usage(config_usage, config_options, "l", 1); } else { - ret = show_config(set); + ret = show_config(config_set); if (ret < 0) { const char * config_filename = config_exclusive_filename; if (!config_exclusive_filename) @@ -106,7 +105,7 @@ int cmd_config(int argc, const char **argv, const char *prefix __maybe_unused) usage_with_options(config_usage, config_options); } - perf_config_set__delete(set); + perf_config_set__delete(config_set); out_err: return ret; } diff --git a/tools/perf/util/config.c b/tools/perf/util/config.c index 8749eca..02fc6d5 100644 --- a/tools/perf/util/config.c +++ b/tools/perf/util/config.c @@ -28,6 +28,7 @@ static int config_linenr; static int config_file_eof; const char *config_exclusive_filename; +struct perf_config_set *config_set; static int get_next_char(void) { diff --git a/tools/perf/util/config.h b/tools/perf/util/config.h index 22ec626..ea157a4 100644 --- a/tools/perf/util/config.h +++ b/tools/perf/util/config.h @@ -20,6 +20,8 @@ struct perf_config_set { struct list_head sections; }; +extern struct perf_config_set *config_set; + struct perf_config_set *perf_config_set__new(void); void perf_config_set__delete(struct perf_config_set *set); -- 2.5.0
[PATCH v7 7/7] perf config: Reimplement show_config() using perf_config()
Old show_config() directly use config set so there are many duplicated code with perf_config_set__iter(). So reimplement show_config() using perf_config() that use perf_config_set__iter() with config set that already contains all configs. Cc: Namhyung Kim Cc: Jiri Olsa Cc: Masami Hiramatsu Cc: Alexander Shishkin Signed-off-by: Taeung Song --- tools/perf/builtin-config.c | 29 +++-- 1 file changed, 7 insertions(+), 22 deletions(-) diff --git a/tools/perf/builtin-config.c b/tools/perf/builtin-config.c index 4dab41e..b6ae8ea 100644 --- a/tools/perf/builtin-config.c +++ b/tools/perf/builtin-config.c @@ -33,28 +33,13 @@ static struct option config_options[] = { OPT_END() }; -static int show_config(struct perf_config_set *set) +static int show_config(const char *key, const char *value, + void *cb __maybe_unused) { - struct perf_config_section *section; - struct perf_config_item *item; - struct list_head *sections; - - if (set == NULL) - return -1; - - sections = &set->sections; - if (list_empty(sections)) - return -1; - - list_for_each_entry(section, sections, node) { - list_for_each_entry(item, §ion->items, node) { - char *value = item->value; - - if (value) - printf("%s.%s=%s\n", section->name, - item->name, value); - } - } + if (value) + printf("%s=%s\n", key, value); + else + printf("%s\n", key); return 0; } @@ -96,7 +81,7 @@ int cmd_config(int argc, const char **argv, const char *prefix __maybe_unused) pr_err("Error: takes no arguments\n"); parse_options_usage(config_usage, config_options, "l", 1); } else { - ret = show_config(config_set); + ret = perf_config(show_config, NULL); if (ret < 0) { const char * config_filename = config_exclusive_filename; if (!config_exclusive_filename) -- 2.5.0
[PATCH v7 5/7] perf config: Reimplement perf_config() using perf_config_set__iter()
Everytime perf_config() is called, perf_config() always read config files. (i.e. user config '~/.perfconfig' and system config '$(sysconfdir)/perfconfig') But we need to use the config set that already contains all config key-value pairs to avoid this repetitive work reading the config files in perf_config(). (the config set mean a global variable 'config_set') In other words, if new perf_config() is called, only first time 'config_set' is initialized collecting all configs from the config files and it work with perf_config_set__iter(). And free the config set after sub-command work at run_builtin(). If we do, 'config_set' can be reused wherever using perf_config() and a feature of old perf_config() is the same as new perf_config() work without the repetitive work that read the config files. Cc: Namhyung Kim Cc: Jiri Olsa Cc: Wang Nan Cc: Peter Zijlstra Cc: Ingo Molnar Cc: Masami Hiramatsu Cc: Alexander Shishkin Signed-off-by: Taeung Song --- tools/perf/perf.c| 1 + tools/perf/util/cache.h | 1 + tools/perf/util/config.c | 86 +++- 3 files changed, 43 insertions(+), 45 deletions(-) diff --git a/tools/perf/perf.c b/tools/perf/perf.c index 15982ce..058d5dc 100644 --- a/tools/perf/perf.c +++ b/tools/perf/perf.c @@ -391,6 +391,7 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv) perf_env__set_cmdline(&perf_env, argc, argv); status = p->fn(argc, argv, prefix); + perf_config_set__delete(&config_set); exit_browser(status); perf_env__exit(&perf_env); bpf__clear(); diff --git a/tools/perf/util/cache.h b/tools/perf/util/cache.h index 0d814bb..54bbd55 100644 --- a/tools/perf/util/cache.h +++ b/tools/perf/util/cache.h @@ -7,6 +7,7 @@ #include #include "../perf.h" #include "../ui/ui.h" +#include "config.h" #include diff --git a/tools/perf/util/config.c b/tools/perf/util/config.c index 2441585..23fb8e4 100644 --- a/tools/perf/util/config.c +++ b/tools/perf/util/config.c @@ -479,51 +479,6 @@ static int perf_config_global(void) return !perf_env_bool("PERF_CONFIG_NOGLOBAL", 0); } -int perf_config(config_fn_t fn, void *data) -{ - int ret = -1; - const char *home = NULL; - - /* Setting $PERF_CONFIG makes perf read _only_ the given config file. */ - if (config_exclusive_filename) - return perf_config_from_file(fn, config_exclusive_filename, data); - if (perf_config_system() && !access(perf_etc_perfconfig(), R_OK)) { - if (perf_config_from_file(fn, perf_etc_perfconfig(), data) < 0) - goto out; - } - - home = getenv("HOME"); - if (perf_config_global() && home) { - char *user_config = strdup(mkpath("%s/.perfconfig", home)); - struct stat st; - - if (user_config == NULL) { - warning("Not enough memory to process %s/.perfconfig, " - "ignoring it.", home); - goto out; - } - - if (stat(user_config, &st) < 0) - goto out_free; - - if (st.st_uid && (st.st_uid != geteuid())) { - warning("File %s not owned by current user or root, " - "ignoring it.", user_config); - goto out_free; - } - - if (!st.st_size) - goto out_free; - - ret = perf_config_from_file(fn, user_config, data); - -out_free: - free(user_config); - } -out: - return ret; -} - static struct perf_config_section *find_section(struct list_head *sections, const char *section_name) { @@ -707,6 +662,47 @@ struct perf_config_set *perf_config_set__new(void) return set; } +static int perf_config_set__iter(struct perf_config_set *set, config_fn_t fn, void *data) +{ + struct perf_config_section *section; + struct perf_config_item *item; + struct list_head *sections; + char key[BUFSIZ]; + + if (set == NULL) + return -1; + + sections = &set->sections; + if (list_empty(sections)) + return -1; + + list_for_each_entry(section, sections, node) { + list_for_each_entry(item, §ion->items, node) { + char *value = item->value; + + if (value) { + scnprintf(key, sizeof(key), "%s.%s", + section->name, item->name); + if (fn(
[PATCH v7 6/7] perf config: Reset the config set at only 'config' sub-command
When first calling perf_config(), the config set is initialized collecting both user and system config files (i.e. user config ~/.perfconfig and system config $(sysconfdir)/perfconfig) so config set contains not only user config but also system config key-value pairs. (User config has higher priority than system config.) But 'config' sub-command individually use the config set so free the existing config set (i.e. a global variable config_set) before reinstantiating it. And 'config' sub-command have '--user' or '--system' options. To reinitialize with the options, the config set should be reset at the very beginning at cmd_config() Cc: Namhyung Kim Cc: Jiri Olsa Cc: Masami Hiramatsu Cc: Alexander Shishkin Signed-off-by: Taeung Song --- tools/perf/builtin-config.c | 5 + 1 file changed, 5 insertions(+) diff --git a/tools/perf/builtin-config.c b/tools/perf/builtin-config.c index f23fe52..4dab41e 100644 --- a/tools/perf/builtin-config.c +++ b/tools/perf/builtin-config.c @@ -79,6 +79,11 @@ int cmd_config(int argc, const char **argv, const char *prefix __maybe_unused) else if (use_user_config) config_exclusive_filename = user_config; + /* +* Reset the config set at only 'config' sub-command +* because of reinitializing with options config file location. +*/ + perf_config_set__delete(&config_set); config_set = perf_config_set__new(); if (!config_set) { ret = -1; -- 2.5.0
[RFC][PATCH v7 0/9] perf config: Reimplement perf_config() using perf_config_set__inter()
Hello, :) This patchset is to reimplement perf_config() for efficient config management. Everytime perf_config() is called, perf_config() always read config files. (i.e. user config '~/.perfconfig' and system config '$(sysconfdir)/perfconfig') But we need to use 'struct perf_config_set config_set' variable that already contains all config key-value pairs to avoid this repetitive work in perf_config(). In other words, if new perf_config() is called, only first time 'config_set' is initialized collecting all configs from config files and it work with perf_config_set__iter(). If we do, 'config_set' can be reused wherever using perf_config() and a feature of old perf_config() is the same as new perf_config() work without the repetitive work that read the config files. IMHO, I think this patchset is needed because not only the repetitive work should be avoided but also in near future, it would be smooth to manage perf configs. If you give me any feedback, I'd apprecicated it. :) Thanks, Taeung v7: - fill a missing crumb that assign NULL to 'set' variable in perf_config_set__new() (Arnaldo) v6: - add printing error message when perf_config_set__iter() is failed - modify commit messages for bugfix 1~3 (PATCH 1/9 ~ 3/9) to help reviewers easily understand why them is needed v5: - solve the leak when perf_config_set__init() failed (Arnaldo) (to clear the problem it is needed to apply the bottom bugfix 1~3 patches) - bugfix 1) fix the problem of abnormal terminaltion at perf_parse_file() called by perf_config() - bugfix 2) if failed at collect_config(), finally free a config set after it is done instead of freeing the config set in the function - bugfix 3) handle NULL pointer exception of 'set' at collect_config() v4: - Keep perf_config_set__delete() as it is (Arnaldo) - Remove perf_config_set__check() (Arnaldo) - Keep the existing code about the config set at cmd_config() (Arnaldo) v3: - add freeing config set after sub-command work at run_builtin() (Namhyung) - remove needless code about the config set at cmd_config() - add a patch about a global variable 'config_set' v2: - split a patch into several patches - reimplement show_config() using new perf_config() - modify perf_config_set__delete using global variable 'config_set' - reset config set when only 'config' sub-commaned work because of options for config file location Taeung Song (7): perf config: If collect_config() is failed, finally free a config set after it is done perf config: Use new perf_config_set__init() to initialize config set perf config: Add global variable 'config_set' perf config: Use zfree() instead of free() at perf_config_set__delete() perf config: Reimplement perf_config() using perf_config_set__iter() perf config: Reset the config set at only 'config' sub-command perf config: Reimplement show_config() using perf_config() tools/perf/builtin-config.c | 41 +--- tools/perf/perf.c | 1 + tools/perf/util/cache.h | 1 + tools/perf/util/config.c| 147 +--- tools/perf/util/config.h| 4 +- 5 files changed, 117 insertions(+), 77 deletions(-) -- 2.5.0
Re: [BUGFIX][PATCH v6 2/9] perf config: If collect_config() is failed, finally free a config set after it is done
On 06/07/2016 06:37 AM, Taeung Song wrote: On 06/07/2016 05:23 AM, Arnaldo Carvalho de Melo wrote: Em Mon, Jun 06, 2016 at 07:52:53PM +0900, Taeung Song escreveu: Because of die() at perf_parse_file() a config set was freed in collect_config(), if failed. But it is natural to free a config set after collect_config() is done when some problems happened. So, in case of failure, lastly free a config set at perf_config_set__new() instead of freeing the config set in collect_config(). Cc: Namhyung Kim Cc: Jiri Olsa Cc: Masami Hiramatsu Cc: Alexander Shishkin Signed-off-by: Taeung Song --- tools/perf/util/config.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tools/perf/util/config.c b/tools/perf/util/config.c index b500737..d013f90 100644 --- a/tools/perf/util/config.c +++ b/tools/perf/util/config.c @@ -639,7 +639,6 @@ static int collect_config(const char *var, const char *value, out_free: free(key); -perf_config_set__delete(set); return -1; } @@ -649,7 +648,8 @@ struct perf_config_set *perf_config_set__new(void) if (set) { INIT_LIST_HEAD(&set->sections); -perf_config(collect_config, set); +if (perf_config(collect_config, set) < 0) +perf_config_set__delete(set); } return set; You can't do that, there is something missing, without looking at the code I think you need: if (set) { INIT_LIST_HEAD(&set->sections); -perf_config(collect_config, set); +if (perf_config(collect_config, set) < 0) { +perf_config_set__delete(set); +set = NULL; +} } return set; No? Granted Sorry for missing above.. I modified using 'return NULL;' instead of 'set = NULL;' as below diff --git a/tools/perf/util/config.c b/tools/perf/util/config.c index c73f1c4..cb749d3 100644 --- a/tools/perf/util/config.c +++ b/tools/perf/util/config.c @@ -643,7 +643,6 @@ static int collect_config(const char *var, const char *value, out_free: free(key); -perf_config_set__delete(set); return -1; } @@ -653,7 +652,10 @@ struct perf_config_set *perf_config_set__new(void) if (set) { INIT_LIST_HEAD(&set->sections); -perf_config(collect_config, set); +if (perf_config(collect_config, set) < 0) { +perf_config_set__delete(set); +return NULL; +} } return set; Because in near future, perf_config_set__delete() will use zfree(). will send changed this patch soon ! Thank you for your review :) Hum.. my answer was stupid. There isn't difference between 'return NULL;' and 'set = NULL;' as a result at perf_config_set__new(). And zfree() at perf_config_set__delete() aren't related to this situation.. Anyway.. I'll send v7 with changed this patch as you said!! Thanks, Taeung
[BUGFIX][RESEND PATCH v6] perf config: If collect_config() is failed, finally free a config set after it is done
Because of die() at perf_parse_file() a config set was freed in collect_config(), if failed. But it is natural to free a config set after collect_config() is done when some problems happened. So, in case of failure, lastly free a config set at perf_config_set__new() instead of freeing the config set in collect_config(). Cc: Namhyung Kim Cc: Jiri Olsa Cc: Masami Hiramatsu Cc: Alexander Shishkin Signed-off-by: Taeung Song --- tools/perf/util/config.c | 6 -- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/tools/perf/util/config.c b/tools/perf/util/config.c index c73f1c4..cb749d3 100644 --- a/tools/perf/util/config.c +++ b/tools/perf/util/config.c @@ -643,7 +643,6 @@ static int collect_config(const char *var, const char *value, out_free: free(key); - perf_config_set__delete(set); return -1; } @@ -653,7 +652,10 @@ struct perf_config_set *perf_config_set__new(void) if (set) { INIT_LIST_HEAD(&set->sections); - perf_config(collect_config, set); + if (perf_config(collect_config, set) < 0) { + perf_config_set__delete(set); + return NULL; + } } return set; -- 2.5.0
Re: [BUGFIX][PATCH v6 2/9] perf config: If collect_config() is failed, finally free a config set after it is done
On 06/07/2016 05:23 AM, Arnaldo Carvalho de Melo wrote: Em Mon, Jun 06, 2016 at 07:52:53PM +0900, Taeung Song escreveu: Because of die() at perf_parse_file() a config set was freed in collect_config(), if failed. But it is natural to free a config set after collect_config() is done when some problems happened. So, in case of failure, lastly free a config set at perf_config_set__new() instead of freeing the config set in collect_config(). Cc: Namhyung Kim Cc: Jiri Olsa Cc: Masami Hiramatsu Cc: Alexander Shishkin Signed-off-by: Taeung Song --- tools/perf/util/config.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tools/perf/util/config.c b/tools/perf/util/config.c index b500737..d013f90 100644 --- a/tools/perf/util/config.c +++ b/tools/perf/util/config.c @@ -639,7 +639,6 @@ static int collect_config(const char *var, const char *value, out_free: free(key); - perf_config_set__delete(set); return -1; } @@ -649,7 +648,8 @@ struct perf_config_set *perf_config_set__new(void) if (set) { INIT_LIST_HEAD(&set->sections); - perf_config(collect_config, set); + if (perf_config(collect_config, set) < 0) + perf_config_set__delete(set); } return set; You can't do that, there is something missing, without looking at the code I think you need: if (set) { INIT_LIST_HEAD(&set->sections); - perf_config(collect_config, set); + if (perf_config(collect_config, set) < 0) { + perf_config_set__delete(set); + set = NULL; + } } return set; No? Granted Sorry for missing above.. I modified using 'return NULL;' instead of 'set = NULL;' as below diff --git a/tools/perf/util/config.c b/tools/perf/util/config.c index c73f1c4..cb749d3 100644 --- a/tools/perf/util/config.c +++ b/tools/perf/util/config.c @@ -643,7 +643,6 @@ static int collect_config(const char *var, const char *value, out_free: free(key); -perf_config_set__delete(set); return -1; } @@ -653,7 +652,10 @@ struct perf_config_set *perf_config_set__new(void) if (set) { INIT_LIST_HEAD(&set->sections); -perf_config(collect_config, set); +if (perf_config(collect_config, set) < 0) { +perf_config_set__delete(set); +return NULL; +} } return set; Because in near future, perf_config_set__delete() will use zfree(). will send changed this patch soon ! Thank you for your review :) Taeung
Re: [PATCH v4 1/6] perf config: Use new perf_config_set__init() to initialize config set
Hi, Arnaldo :) Did you have a nice weekend? I sent this mail for nothing else but to tell the reason of v6 to you. On 06/01/2016 01:52 AM, Taeung Song wrote: On 05/31/2016 10:43 PM, Arnaldo Carvalho de Melo wrote: Em Tue, May 31, 2016 at 10:13:43AM +0900, Taeung Song escreveu: Instead of perf_config(), This function initialize config set collecting all configs from config files (i.e. user config ~/.perfconfig and system config $(sysconfdir)/perfconfig). If there are the same config variable both user and system config file, user config has higher priority than system config. Cc: Namhyung Kim Cc: Jiri Olsa Cc: Masami Hiramatsu Cc: Alexander Shishkin Signed-off-by: Taeung Song --- tools/perf/util/config.c | 50 +++- 1 file changed, 49 insertions(+), 1 deletion(-) diff --git a/tools/perf/util/config.c b/tools/perf/util/config.c index dad7d82..5d01899 100644 --- a/tools/perf/util/config.c +++ b/tools/perf/util/config.c @@ -645,13 +645,61 @@ out_free: return -1; } +static int perf_config_set__init(struct perf_config_set *set) +{ +int ret = 0, found = 0; +const char *home = NULL; + +/* Setting $PERF_CONFIG makes perf read _only_ the given config file. */ +if (config_exclusive_filename) +return perf_config_from_file(collect_config, config_exclusive_filename, set); +if (perf_config_system() && !access(perf_etc_perfconfig(), R_OK)) { +ret += perf_config_from_file(collect_config, perf_etc_perfconfig(), set); +found += 1; +} + +home = getenv("HOME"); +if (perf_config_global() && home) { +char *user_config = strdup(mkpath("%s/.perfconfig", home)); +struct stat st; + +if (user_config == NULL) { +warning("Not enough memory to process %s/.perfconfig, " +"ignoring it.", home); +goto out; +} + +if (stat(user_config, &st) < 0) +goto out_free; + +if (st.st_uid && (st.st_uid != geteuid())) { +warning("File %s not owned by current user or root, " +"ignoring it.", user_config); +goto out_free; +} + +if (!st.st_size) +goto out_free; + +ret += perf_config_from_file(collect_config, user_config, set); +found += 1; +out_free: +free(user_config); +} +out: +if (found == 0) +return -1; +return ret; +} + struct perf_config_set *perf_config_set__new(void) { struct perf_config_set *set = zalloc(sizeof(*set)); if (set) { INIT_LIST_HEAD(&set->sections); -perf_config(collect_config, set); +if (perf_config_set__init(set) < 0) +return NULL; So, the usual pattern is: alloc, init, fail? free, return NULL. I thought you could've been deviating from that pattern and went to look at perf_config_set__init() to see if that was doing the freeing in case of failure, which it shouldn't, it isn't, so I guess this is a leak on failure, no? I can modify the problem of the leak you said by simple handling a case of failure at perf_config_set__init(). But I found preexisting small problems so I sent v6 with the three [BUGFIX] patches !! If you can't agree this way to solve the leak, I'd find other way. :) Thanks, Taeung You are right. And I found additional problems. First of all, as you said, if it is failed in perf_config_set__init(), the config set wouldn't be freed so this is a leak on failure. Secondly, if it is failed in perf_parse_file(), perf_parse_file() cannot return because of die() so perf_config_from_file() and perf_config() don't also return. I guess this is abnormal termination without the freeing. (The important point of this problem is die() at perf_parse_file()) Thirdly, there are problems that are related to collect_config(). If perf_config_from_file(collect_config,..) is failed the config set will be freed at collect_config() like below. static int collect_config(const char *var, const char *value, void *perf_config_set) { ... out_free: free(key); perf_config_set__delete(set); return -1; } And then if calling perf_config_from_file(collect_config,..) at perf_config_set__init() again, an error will happen because the config set is NULL at collect_config(). (the error mean NULL pointer exception.) To conclude, First of all, I'll send preparatory PATCH set for this patch to solve the problems i.e. 1) A problem that perf_config() can't return becuase of die() at perf_parse_file() 2) A problem about the freeing config set at collect_config() 3) NULL pointer exception at collect_config() And then I will send changed this patch following above patchset. (to solve a leak when perf_config_set__init() failed) Thanks, Taeung
[BUGFIX][PATCH v6 2/9] perf config: If collect_config() is failed, finally free a config set after it is done
Because of die() at perf_parse_file() a config set was freed in collect_config(), if failed. But it is natural to free a config set after collect_config() is done when some problems happened. So, in case of failure, lastly free a config set at perf_config_set__new() instead of freeing the config set in collect_config(). Cc: Namhyung Kim Cc: Jiri Olsa Cc: Masami Hiramatsu Cc: Alexander Shishkin Signed-off-by: Taeung Song --- tools/perf/util/config.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tools/perf/util/config.c b/tools/perf/util/config.c index b500737..d013f90 100644 --- a/tools/perf/util/config.c +++ b/tools/perf/util/config.c @@ -639,7 +639,6 @@ static int collect_config(const char *var, const char *value, out_free: free(key); - perf_config_set__delete(set); return -1; } @@ -649,7 +648,8 @@ struct perf_config_set *perf_config_set__new(void) if (set) { INIT_LIST_HEAD(&set->sections); - perf_config(collect_config, set); + if (perf_config(collect_config, set) < 0) + perf_config_set__delete(set); } return set; -- 2.5.0
[PATCH v6 5/9] perf config: Add global variable 'config_set'
The config set is prepared by collecting all configs from config files (i.e. user config ~/.perfconfig and system config $(sysconfdir)/perfconfig) so the config set contains all config key-value pairs. We need to use it as global variable to share it. And in near future, the variable will be handled in perf_config() and other functions at util/config.c Cc: Namhyung Kim Cc: Jiri Olsa Cc: Masami Hiramatsu Cc: Alexander Shishkin Signed-off-by: Taeung Song --- tools/perf/builtin-config.c | 9 - tools/perf/util/config.c| 1 + tools/perf/util/config.h| 2 ++ 3 files changed, 7 insertions(+), 5 deletions(-) diff --git a/tools/perf/builtin-config.c b/tools/perf/builtin-config.c index fe1b77f..b3bc01a 100644 --- a/tools/perf/builtin-config.c +++ b/tools/perf/builtin-config.c @@ -62,7 +62,6 @@ static int show_config(struct perf_config_set *set) int cmd_config(int argc, const char **argv, const char *prefix __maybe_unused) { int ret = 0; - struct perf_config_set *set; char *user_config = mkpath("%s/.perfconfig", getenv("HOME")); argc = parse_options(argc, argv, config_options, config_usage, @@ -80,8 +79,8 @@ int cmd_config(int argc, const char **argv, const char *prefix __maybe_unused) else if (use_user_config) config_exclusive_filename = user_config; - set = perf_config_set__new(); - if (!set) { + config_set = perf_config_set__new(); + if (!config_set) { ret = -1; goto out_err; } @@ -92,7 +91,7 @@ int cmd_config(int argc, const char **argv, const char *prefix __maybe_unused) pr_err("Error: takes no arguments\n"); parse_options_usage(config_usage, config_options, "l", 1); } else { - ret = show_config(set); + ret = show_config(config_set); if (ret < 0) { const char * config_filename = config_exclusive_filename; if (!config_exclusive_filename) @@ -106,7 +105,7 @@ int cmd_config(int argc, const char **argv, const char *prefix __maybe_unused) usage_with_options(config_usage, config_options); } - perf_config_set__delete(set); + perf_config_set__delete(config_set); out_err: return ret; } diff --git a/tools/perf/util/config.c b/tools/perf/util/config.c index 6bfa112..21b7ca8 100644 --- a/tools/perf/util/config.c +++ b/tools/perf/util/config.c @@ -28,6 +28,7 @@ static int config_linenr; static int config_file_eof; const char *config_exclusive_filename; +struct perf_config_set *config_set; static int get_next_char(void) { diff --git a/tools/perf/util/config.h b/tools/perf/util/config.h index 22ec626..ea157a4 100644 --- a/tools/perf/util/config.h +++ b/tools/perf/util/config.h @@ -20,6 +20,8 @@ struct perf_config_set { struct list_head sections; }; +extern struct perf_config_set *config_set; + struct perf_config_set *perf_config_set__new(void); void perf_config_set__delete(struct perf_config_set *set); -- 2.5.0
[PATCH v6 7/9] perf config: Reimplement perf_config() using perf_config_set__iter()
Everytime perf_config() is called, perf_config() always read config files. (i.e. user config '~/.perfconfig' and system config '$(sysconfdir)/perfconfig') But we need to use the config set that already contains all config key-value pairs to avoid this repetitive work reading the config files in perf_config(). (the config set mean a global variable 'config_set') In other words, if new perf_config() is called, only first time 'config_set' is initialized collecting all configs from the config files and it work with perf_config_set__iter(). And free the config set after sub-command work at run_builtin(). If we do, 'config_set' can be reused wherever using perf_config() and a feature of old perf_config() is the same as new perf_config() work without the repetitive work that read the config files. Cc: Namhyung Kim Cc: Jiri Olsa Cc: Wang Nan Cc: Peter Zijlstra Cc: Ingo Molnar Cc: Masami Hiramatsu Cc: Alexander Shishkin Signed-off-by: Taeung Song --- tools/perf/perf.c| 1 + tools/perf/util/cache.h | 1 + tools/perf/util/config.c | 86 +++- 3 files changed, 43 insertions(+), 45 deletions(-) diff --git a/tools/perf/perf.c b/tools/perf/perf.c index 15982ce..058d5dc 100644 --- a/tools/perf/perf.c +++ b/tools/perf/perf.c @@ -391,6 +391,7 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv) perf_env__set_cmdline(&perf_env, argc, argv); status = p->fn(argc, argv, prefix); + perf_config_set__delete(&config_set); exit_browser(status); perf_env__exit(&perf_env); bpf__clear(); diff --git a/tools/perf/util/cache.h b/tools/perf/util/cache.h index 0d814bb..54bbd55 100644 --- a/tools/perf/util/cache.h +++ b/tools/perf/util/cache.h @@ -7,6 +7,7 @@ #include #include "../perf.h" #include "../ui/ui.h" +#include "config.h" #include diff --git a/tools/perf/util/config.c b/tools/perf/util/config.c index 2cae413..8c517cb 100644 --- a/tools/perf/util/config.c +++ b/tools/perf/util/config.c @@ -479,51 +479,6 @@ static int perf_config_global(void) return !perf_env_bool("PERF_CONFIG_NOGLOBAL", 0); } -int perf_config(config_fn_t fn, void *data) -{ - int ret = -1; - const char *home = NULL; - - /* Setting $PERF_CONFIG makes perf read _only_ the given config file. */ - if (config_exclusive_filename) - return perf_config_from_file(fn, config_exclusive_filename, data); - if (perf_config_system() && !access(perf_etc_perfconfig(), R_OK)) { - if (perf_config_from_file(fn, perf_etc_perfconfig(), data) < 0) - goto out; - } - - home = getenv("HOME"); - if (perf_config_global() && home) { - char *user_config = strdup(mkpath("%s/.perfconfig", home)); - struct stat st; - - if (user_config == NULL) { - warning("Not enough memory to process %s/.perfconfig, " - "ignoring it.", home); - goto out; - } - - if (stat(user_config, &st) < 0) - goto out_free; - - if (st.st_uid && (st.st_uid != geteuid())) { - warning("File %s not owned by current user or root, " - "ignoring it.", user_config); - goto out_free; - } - - if (!st.st_size) - goto out_free; - - ret = perf_config_from_file(fn, user_config, data); - -out_free: - free(user_config); - } -out: - return ret; -} - static struct perf_config_section *find_section(struct list_head *sections, const char *section_name) { @@ -707,6 +662,47 @@ struct perf_config_set *perf_config_set__new(void) return set; } +static int perf_config_set__iter(struct perf_config_set *set, config_fn_t fn, void *data) +{ + struct perf_config_section *section; + struct perf_config_item *item; + struct list_head *sections; + char key[BUFSIZ]; + + if (set == NULL) + return -1; + + sections = &set->sections; + if (list_empty(sections)) + return -1; + + list_for_each_entry(section, sections, node) { + list_for_each_entry(item, §ion->items, node) { + char *value = item->value; + + if (value) { + scnprintf(key, sizeof(key), "%s.%s", + section->name, item->name); + if (fn(
[PATCH v6 8/9] perf config: Reset the config set at only 'config' sub-command
When first calling perf_config(), the config set is initialized collecting both user and system config files (i.e. user config ~/.perfconfig and system config $(sysconfdir)/perfconfig) so config set contains not only user config but also system config key-value pairs. (User config has higher priority than system config.) But 'config' sub-command individually use the config set so free the existing config set (i.e. a global variable config_set) before reinstantiating it. And 'config' sub-command have '--user' or '--system' options. To reinitialize with the options, the config set should be reset at the very beginning at cmd_config() Cc: Namhyung Kim Cc: Jiri Olsa Cc: Masami Hiramatsu Cc: Alexander Shishkin Signed-off-by: Taeung Song --- tools/perf/builtin-config.c | 5 + 1 file changed, 5 insertions(+) diff --git a/tools/perf/builtin-config.c b/tools/perf/builtin-config.c index f23fe52..4dab41e 100644 --- a/tools/perf/builtin-config.c +++ b/tools/perf/builtin-config.c @@ -79,6 +79,11 @@ int cmd_config(int argc, const char **argv, const char *prefix __maybe_unused) else if (use_user_config) config_exclusive_filename = user_config; + /* +* Reset the config set at only 'config' sub-command +* because of reinitializing with options config file location. +*/ + perf_config_set__delete(&config_set); config_set = perf_config_set__new(); if (!config_set) { ret = -1; -- 2.5.0
[PATCH v6 9/9] perf config: Reimplement show_config() using perf_config()
Old show_config() directly use config set so there are many duplicated code with perf_config_set__iter(). So reimplement show_config() using perf_config() that use perf_config_set__iter() with config set that already contains all configs. Cc: Namhyung Kim Cc: Jiri Olsa Cc: Masami Hiramatsu Cc: Alexander Shishkin Signed-off-by: Taeung Song --- tools/perf/builtin-config.c | 29 +++-- 1 file changed, 7 insertions(+), 22 deletions(-) diff --git a/tools/perf/builtin-config.c b/tools/perf/builtin-config.c index 4dab41e..b6ae8ea 100644 --- a/tools/perf/builtin-config.c +++ b/tools/perf/builtin-config.c @@ -33,28 +33,13 @@ static struct option config_options[] = { OPT_END() }; -static int show_config(struct perf_config_set *set) +static int show_config(const char *key, const char *value, + void *cb __maybe_unused) { - struct perf_config_section *section; - struct perf_config_item *item; - struct list_head *sections; - - if (set == NULL) - return -1; - - sections = &set->sections; - if (list_empty(sections)) - return -1; - - list_for_each_entry(section, sections, node) { - list_for_each_entry(item, §ion->items, node) { - char *value = item->value; - - if (value) - printf("%s.%s=%s\n", section->name, - item->name, value); - } - } + if (value) + printf("%s=%s\n", key, value); + else + printf("%s\n", key); return 0; } @@ -96,7 +81,7 @@ int cmd_config(int argc, const char **argv, const char *prefix __maybe_unused) pr_err("Error: takes no arguments\n"); parse_options_usage(config_usage, config_options, "l", 1); } else { - ret = show_config(config_set); + ret = perf_config(show_config, NULL); if (ret < 0) { const char * config_filename = config_exclusive_filename; if (!config_exclusive_filename) -- 2.5.0
[BUGFIX][PATCH v6 3/9] perf config: Handle the error when config set is NULL at collect_config()
collect_config() collect all config key-value pairs from config files and put each config info in config set. But if config set (i.e. 'set' variable at collect_config()) is NULL, this is wrong so handle it. Cc: Namhyung Kim Cc: Jiri Olsa Cc: Masami Hiramatsu Cc: Alexander Shishkin Signed-off-by: Taeung Song --- tools/perf/util/config.c | 6 +- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/tools/perf/util/config.c b/tools/perf/util/config.c index d013f90..062eeb8 100644 --- a/tools/perf/util/config.c +++ b/tools/perf/util/config.c @@ -607,8 +607,12 @@ static int collect_config(const char *var, const char *value, struct perf_config_section *section = NULL; struct perf_config_item *item = NULL; struct perf_config_set *set = perf_config_set; - struct list_head *sections = &set->sections; + struct list_head *sections; + if (set == NULL) + return -1; + + sections = &set->sections; key = ptr = strdup(var); if (!key) { pr_debug("%s: strdup failed\n", __func__); -- 2.5.0
[BUGFIX][PATCH v6 1/9] perf config: Fix the problem of abnormal termination at perf_parse_file()
If a config file has wrong key-value pairs, perf process will be forcibly terminated by die() at perf_parse_file() called by perf_config() so terminal setting can be crushed because of unusual termination. For example, If user config file has a wrong value 'red;default' instead of a normal value like 'red, default' for a key 'colors.top', # cat ~/.perfconfig [colors] medium = red;default # wrong value and if running sub-command 'top', # perf top perf process is dead by force and terminal setting is broken with a messge like below. Fatal: bad config file line 2 in /root/.perfconfig So fix it. If perf_config() can return on failure without calling die() at perf_parse_file(), this problem can be solved. And if a config file has wrong values, show the error message and then use default config values instead of wrong config values. Cc: Namhyung Kim Cc: Jiri Olsa Cc: Masami Hiramatsu Cc: Alexander Shishkin Signed-off-by: Taeung Song --- tools/perf/util/config.c | 16 +++- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/tools/perf/util/config.c b/tools/perf/util/config.c index dad7d82..b500737 100644 --- a/tools/perf/util/config.c +++ b/tools/perf/util/config.c @@ -275,7 +275,8 @@ static int perf_parse_file(config_fn_t fn, void *data) break; } } - die("bad config file line %d in %s", config_linenr, config_file_name); + pr_err("bad config file line %d in %s\n", config_linenr, config_file_name); + return -1; } static int parse_unit_factor(const char *end, unsigned long *val) @@ -479,16 +480,15 @@ static int perf_config_global(void) int perf_config(config_fn_t fn, void *data) { - int ret = 0, found = 0; + int ret = -1; const char *home = NULL; /* Setting $PERF_CONFIG makes perf read _only_ the given config file. */ if (config_exclusive_filename) return perf_config_from_file(fn, config_exclusive_filename, data); if (perf_config_system() && !access(perf_etc_perfconfig(), R_OK)) { - ret += perf_config_from_file(fn, perf_etc_perfconfig(), - data); - found += 1; + if (perf_config_from_file(fn, perf_etc_perfconfig(), data) < 0) + goto out; } home = getenv("HOME"); @@ -514,14 +514,12 @@ int perf_config(config_fn_t fn, void *data) if (!st.st_size) goto out_free; - ret += perf_config_from_file(fn, user_config, data); - found += 1; + ret = perf_config_from_file(fn, user_config, data); + out_free: free(user_config); } out: - if (found == 0) - return -1; return ret; } -- 2.5.0
[PATCH v6 4/9] perf config: Use new perf_config_set__init() to initialize config set
Instead of perf_config(), This function initialize config set collecting all configs from config files (i.e. user config ~/.perfconfig and system config $(sysconfdir)/perfconfig). If there are the same config variable both user and system config file, user config has higher priority than system config. Cc: Namhyung Kim Cc: Jiri Olsa Cc: Masami Hiramatsu Cc: Alexander Shishkin Signed-off-by: Taeung Song --- tools/perf/util/config.c | 49 +++- 1 file changed, 48 insertions(+), 1 deletion(-) diff --git a/tools/perf/util/config.c b/tools/perf/util/config.c index 062eeb8..6bfa112 100644 --- a/tools/perf/util/config.c +++ b/tools/perf/util/config.c @@ -646,14 +646,61 @@ out_free: return -1; } +static int perf_config_set__init(struct perf_config_set *set) +{ + int ret = -1; + const char *home = NULL; + + /* Setting $PERF_CONFIG makes perf read _only_ the given config file. */ + if (config_exclusive_filename) + return perf_config_from_file(collect_config, config_exclusive_filename, set); + if (perf_config_system() && !access(perf_etc_perfconfig(), R_OK)) { + if (perf_config_from_file(collect_config, perf_etc_perfconfig(), set) < 0) + goto out; + } + + home = getenv("HOME"); + if (perf_config_global() && home) { + char *user_config = strdup(mkpath("%s/.perfconfig", home)); + struct stat st; + + if (user_config == NULL) { + warning("Not enough memory to process %s/.perfconfig, " + "ignoring it.", home); + goto out; + } + + if (stat(user_config, &st) < 0) + goto out_free; + + if (st.st_uid && (st.st_uid != geteuid())) { + warning("File %s not owned by current user or root, " + "ignoring it.", user_config); + goto out_free; + } + + if (!st.st_size) + goto out_free; + + ret = perf_config_from_file(collect_config, user_config, set); + +out_free: + free(user_config); + } +out: + return ret; +} + struct perf_config_set *perf_config_set__new(void) { struct perf_config_set *set = zalloc(sizeof(*set)); if (set) { INIT_LIST_HEAD(&set->sections); - if (perf_config(collect_config, set) < 0) + if (perf_config_set__init(set) < 0) { perf_config_set__delete(set); + return NULL; + } } return set; -- 2.5.0
[RFC][PATCH v6 0/9] perf config: Reimplement perf_config() using perf_config_set__inter()
Hello, :) This patchset is to reimplement perf_config() for efficient config management. Everytime perf_config() is called, perf_config() always read config files. (i.e. user config '~/.perfconfig' and system config '$(sysconfdir)/perfconfig') But we need to use 'struct perf_config_set config_set' variable that already contains all config key-value pairs to avoid this repetitive work in perf_config(). In other words, if new perf_config() is called, only first time 'config_set' is initialized collecting all configs from config files and it work with perf_config_set__iter(). If we do, 'config_set' can be reused wherever using perf_config() and a feature of old perf_config() is the same as new perf_config() work without the repetitive work that read the config files. IMHO, I think this patchset is needed because not only the repetitive work should be avoided but also in near future, it would be smooth to manage perf configs. Most important patch of this patchset is "[PATCH v5 7/9] perf config: Reimplement perf_config() using perf_config_set__iter()" and PATCH 1/9 ~ 6/9 are preparation for it. If you give me any feedback, I'd apprecicated it. :) Thanks, Taeung v6: - add printing error message when perf_config_set__iter() is failed - modify commit messages for bugfix 1~3 (PATCH 1/9 ~ 3/9) to help reviewers easily understand why them is needed v5: - solve the leak when perf_config_set__init() failed (Arnaldo) (to clear the problem it is needed to apply the bottom bugfix 1~3 patches) - bugfix 1) fix the problem of abnormal terminaltion at perf_parse_file() called by perf_config() - bugfix 2) if failed at collect_config(), finally free a config set after it is done instead of freeing the config set in the function - bugfix 3) handle NULL pointer exception of 'set' at collect_config() v4: - Keep perf_config_set__delete() as it is (Arnaldo) - Remove perf_config_set__check() (Arnaldo) - Keep the existing code about the config set at cmd_config() (Arnaldo) v3: - add freeing config set after sub-command work at run_builtin() (Namhyung) - remove needless code about the config set at cmd_config() - add a patch about a global variable 'config_set' v2: - split a patch into several patches - reimplement show_config() using new perf_config() - modify perf_config_set__delete using global variable 'config_set' - reset config set when only 'config' sub-commaned work because of options for config file location Taeung Song (9): perf config: Fix the problem of abnormal termination at perf_parse_file() perf config: If collect_config() is failed, finally free a config set after it is done perf config: Handle the error when config set is NULL at collect_config() perf config: Use new perf_config_set__init() to initialize config set perf config: Add global variable 'config_set' perf config: Use zfree() instead of free() at perf_config_set__delete() perf config: Reimplement perf_config() using perf_config_set__iter() perf config: Reset the config set at only 'config' sub-command perf config: Reimplement show_config() using perf_config() tools/perf/builtin-config.c | 41 +--- tools/perf/perf.c | 1 + tools/perf/util/cache.h | 1 + tools/perf/util/config.c| 159 +--- tools/perf/util/config.h| 4 +- 5 files changed, 124 insertions(+), 82 deletions(-) -- 2.5.0
[PATCH v6 6/9] perf config: Use zfree() instead of free() at perf_config_set__delete()
perf_config_set__delete() delete allocated the config set but the global variable 'config_set' is used all around. So purge and zfree by an address of the global variable , i.e. 'struct perf_config_set **' type instead of using local variable 'set' of which type is 'struct perf_config_set *'. Cc: Namhyung Kim Cc: Jiri Olsa Cc: Masami Hiramatsu Cc: Alexander Shishkin Signed-off-by: Taeung Song --- tools/perf/builtin-config.c | 2 +- tools/perf/util/config.c| 11 +++ tools/perf/util/config.h| 2 +- 3 files changed, 9 insertions(+), 6 deletions(-) diff --git a/tools/perf/builtin-config.c b/tools/perf/builtin-config.c index b3bc01a..f23fe52 100644 --- a/tools/perf/builtin-config.c +++ b/tools/perf/builtin-config.c @@ -105,7 +105,7 @@ int cmd_config(int argc, const char **argv, const char *prefix __maybe_unused) usage_with_options(config_usage, config_options); } - perf_config_set__delete(config_set); + perf_config_set__delete(&config_set); out_err: return ret; } diff --git a/tools/perf/util/config.c b/tools/perf/util/config.c index 21b7ca8..2cae413 100644 --- a/tools/perf/util/config.c +++ b/tools/perf/util/config.c @@ -699,7 +699,7 @@ struct perf_config_set *perf_config_set__new(void) if (set) { INIT_LIST_HEAD(&set->sections); if (perf_config_set__init(set) < 0) { - perf_config_set__delete(set); + perf_config_set__delete(&set); return NULL; } } @@ -741,10 +741,13 @@ static void perf_config_set__purge(struct perf_config_set *set) } } -void perf_config_set__delete(struct perf_config_set *set) +void perf_config_set__delete(struct perf_config_set **set) { - perf_config_set__purge(set); - free(set); + if (*set == NULL) + return; + + perf_config_set__purge(*set); + zfree(set); } /* diff --git a/tools/perf/util/config.h b/tools/perf/util/config.h index ea157a4..271b429 100644 --- a/tools/perf/util/config.h +++ b/tools/perf/util/config.h @@ -23,6 +23,6 @@ struct perf_config_set { extern struct perf_config_set *config_set; struct perf_config_set *perf_config_set__new(void); -void perf_config_set__delete(struct perf_config_set *set); +void perf_config_set__delete(struct perf_config_set **set); #endif /* __PERF_CONFIG_H */ -- 2.5.0
[tip:perf/core] perf tools: Add arch/*/include/generated/ to .gitignore
Commit-ID: dcd1e2a7ba63710843d559f1570628321e62223e Gitweb: http://git.kernel.org/tip/dcd1e2a7ba63710843d559f1570628321e62223e Author: Taeung Song AuthorDate: Fri, 27 May 2016 19:01:14 +0900 Committer: Arnaldo Carvalho de Melo CommitDate: Mon, 30 May 2016 12:41:46 -0300 perf tools: Add arch/*/include/generated/ to .gitignore Commit 1b700c997500 ("perf tools: Build syscall table .c header from kernel's syscall_64.tbl") automatically generates per-arch syscall table arrays, e.g.: arch/x86/include/generated/asm/syscalls_64.c So add this directory to .gitignore Signed-off-by: Taeung Song Cc: Adrian Hunter Cc: Alexander Shishkin Cc: David Ahern Cc: Jiri Olsa Cc: Masami Hiramatsu Cc: Namhyung Kim Cc: Peter Zijlstra Cc: Wang Nan Fixes: 1b700c997500 ("perf tools: Build syscall table .c header from kernel's syscall_64.tbl") Link: http://lkml.kernel.org/r/1464343274-19403-1-git-send-email-treeze.tae...@gmail.com Signed-off-by: Arnaldo Carvalho de Melo --- tools/perf/.gitignore | 1 + 1 file changed, 1 insertion(+) diff --git a/tools/perf/.gitignore b/tools/perf/.gitignore index 3d1bb80..3db3db9 100644 --- a/tools/perf/.gitignore +++ b/tools/perf/.gitignore @@ -30,3 +30,4 @@ config.mak.autogen *.pyo .config-detected util/intel-pt-decoder/inat-tables.c +arch/*/include/generated/
[RFC][PATCH v5 0/9] perf config: Reimplement perf_config() using perf_config_set__inter()
Hello, :) This patchset is to reimplement perf_config() for efficient config management. Everytime perf_config() is called, perf_config() always read config files. (i.e. user config '~/.perfconfig' and system config '$(sysconfdir)/perfconfig') But we need to use 'struct perf_config_set config_set' variable that already contains all config key-value pairs to avoid this repetitive work in perf_config(). In other words, if new perf_config() is called, only first time 'config_set' is initialized collecting all configs from config files and it work with perf_config_set__iter(). If we do, 'config_set' can be reused wherever using perf_config() and a feature of old perf_config() is the same as new perf_config() work without the repetitive work that read the config files. IMHO, I think this patchset is needed because not only the repetitive work should be avoided but also in near future, it would be smooth to manage perf configs. Most important patch of this patchset is "[PATCH v5 7/9] perf config: Reimplement perf_config() using perf_config_set__iter()" and PATCH 1/9 ~ 6/9 are preparation for it. If you give me any feedback, I'd apprecicated it. :) Thanks, Taeung v5: - solve a leak when perf_config_set__init() failed (Arnaldo) - bugfix) let perf_config() return in case of failure - bugfix) finally free the config set after collect_config() worked - bugfix) handle NULL pointer exception of 'set' at collect_config() v4: - Keep perf_config_set__delete() as it is (Arnaldo) - Remove perf_config_set__check() (Arnaldo) - Keep the existing code about the config set at cmd_config() (Arnaldo) v3: - add freeing config set after sub-command work at run_builtin() (Namhyung) - remove needless code about the config set at cmd_config() - add a patch about a global variable 'config_set' v2: - split a patch into several patches - reimplement show_config() using new perf_config() - modify perf_config_set__delete using global variable 'config_set' - reset config set when only 'config' sub-commaned work because of options for config file location Taeung Song (9): perf config: Let perf_config() return in case of failure perf config: Finally free a config set after collect_config() worked perf config: Handle an error when config set is NULL at collect_config() perf config: Use new perf_config_set__init() to initialize config set perf config: Add global variable 'config_set' perf config: Use zfree() instead of free() at perf_config_set__delete() perf config: Reimplement perf_config() using perf_config_set__iter() perf config: Reset the config set at only 'config' sub-command perf config: Reimplement show_config() using perf_config() tools/perf/builtin-config.c | 41 +--- tools/perf/perf.c | 1 + tools/perf/util/cache.h | 1 + tools/perf/util/config.c| 156 tools/perf/util/config.h| 4 +- 5 files changed, 121 insertions(+), 82 deletions(-) -- 2.5.0
[PATCH v5 7/9] perf config: Reimplement perf_config() using perf_config_set__iter()
Everytime perf_config() is called, perf_config() always read config files. (i.e. user config '~/.perfconfig' and system config '$(sysconfdir)/perfconfig') But we need to use the config set that already contains all config key-value pairs to avoid this repetitive work reading the config files in perf_config(). (the config set mean a global variable 'config_set') In other words, if new perf_config() is called, only first time 'config_set' is initialized collecting all configs from the config files and it work with perf_config_set__iter(). And free the config set after sub-command work at run_builtin(). If we do, 'config_set' can be reused wherever using perf_config() and a feature of old perf_config() is the same as new perf_config() work without the repetitive work that read the config files. Cc: Namhyung Kim Cc: Jiri Olsa Cc: Wang Nan Cc: Peter Zijlstra Cc: Ingo Molnar Cc: Masami Hiramatsu Cc: Alexander Shishkin Signed-off-by: Taeung Song --- tools/perf/perf.c| 1 + tools/perf/util/cache.h | 1 + tools/perf/util/config.c | 83 ++-- 3 files changed, 40 insertions(+), 45 deletions(-) diff --git a/tools/perf/perf.c b/tools/perf/perf.c index 15982ce..058d5dc 100644 --- a/tools/perf/perf.c +++ b/tools/perf/perf.c @@ -391,6 +391,7 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv) perf_env__set_cmdline(&perf_env, argc, argv); status = p->fn(argc, argv, prefix); + perf_config_set__delete(&config_set); exit_browser(status); perf_env__exit(&perf_env); bpf__clear(); diff --git a/tools/perf/util/cache.h b/tools/perf/util/cache.h index 0d814bb..54bbd55 100644 --- a/tools/perf/util/cache.h +++ b/tools/perf/util/cache.h @@ -7,6 +7,7 @@ #include #include "../perf.h" #include "../ui/ui.h" +#include "config.h" #include diff --git a/tools/perf/util/config.c b/tools/perf/util/config.c index 2cae413..d4d204a 100644 --- a/tools/perf/util/config.c +++ b/tools/perf/util/config.c @@ -479,51 +479,6 @@ static int perf_config_global(void) return !perf_env_bool("PERF_CONFIG_NOGLOBAL", 0); } -int perf_config(config_fn_t fn, void *data) -{ - int ret = -1; - const char *home = NULL; - - /* Setting $PERF_CONFIG makes perf read _only_ the given config file. */ - if (config_exclusive_filename) - return perf_config_from_file(fn, config_exclusive_filename, data); - if (perf_config_system() && !access(perf_etc_perfconfig(), R_OK)) { - if (perf_config_from_file(fn, perf_etc_perfconfig(), data) < 0) - goto out; - } - - home = getenv("HOME"); - if (perf_config_global() && home) { - char *user_config = strdup(mkpath("%s/.perfconfig", home)); - struct stat st; - - if (user_config == NULL) { - warning("Not enough memory to process %s/.perfconfig, " - "ignoring it.", home); - goto out; - } - - if (stat(user_config, &st) < 0) - goto out_free; - - if (st.st_uid && (st.st_uid != geteuid())) { - warning("File %s not owned by current user or root, " - "ignoring it.", user_config); - goto out_free; - } - - if (!st.st_size) - goto out_free; - - ret = perf_config_from_file(fn, user_config, data); - -out_free: - free(user_config); - } -out: - return ret; -} - static struct perf_config_section *find_section(struct list_head *sections, const char *section_name) { @@ -707,6 +662,44 @@ struct perf_config_set *perf_config_set__new(void) return set; } +static int perf_config_set__iter(struct perf_config_set *set, config_fn_t fn, void *data) +{ + struct perf_config_section *section; + struct perf_config_item *item; + struct list_head *sections; + char key[BUFSIZ]; + + if (set == NULL) + return -1; + + sections = &set->sections; + if (list_empty(sections)) + return -1; + + list_for_each_entry(section, sections, node) { + list_for_each_entry(item, §ion->items, node) { + char *value = item->value; + + if (value) { + scnprintf(key, sizeof(key), "%s.%s", + section->name, item->name); + if (fn(key, value, data) < 0) +
[BUGFIX][PATCH v5 3/9] perf config: Handle an error when config set is NULL at collect_config()
collect_config() collect all config key-value pairs from a config files and put each config in config set. But if config set (i.e. 'set' variable at collect_config()) is NULL, this is an error so return -1. Cc: Namhyung Kim Cc: Jiri Olsa Cc: Masami Hiramatsu Cc: Alexander Shishkin Signed-off-by: Taeung Song --- tools/perf/util/config.c | 6 +- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/tools/perf/util/config.c b/tools/perf/util/config.c index d013f90..062eeb8 100644 --- a/tools/perf/util/config.c +++ b/tools/perf/util/config.c @@ -607,8 +607,12 @@ static int collect_config(const char *var, const char *value, struct perf_config_section *section = NULL; struct perf_config_item *item = NULL; struct perf_config_set *set = perf_config_set; - struct list_head *sections = &set->sections; + struct list_head *sections; + if (set == NULL) + return -1; + + sections = &set->sections; key = ptr = strdup(var); if (!key) { pr_debug("%s: strdup failed\n", __func__); -- 2.5.0
[BUGFIX][PATCH v5 2/9] perf config: Finally free a config set after collect_config() worked
Current colllect_config() free a config set on failure. But it is needed to finally free it at perf_config_set__new() after collect_config() worked. Cc: Namhyung Kim Cc: Jiri Olsa Cc: Masami Hiramatsu Cc: Alexander Shishkin Signed-off-by: Taeung Song --- tools/perf/util/config.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tools/perf/util/config.c b/tools/perf/util/config.c index b500737..d013f90 100644 --- a/tools/perf/util/config.c +++ b/tools/perf/util/config.c @@ -639,7 +639,6 @@ static int collect_config(const char *var, const char *value, out_free: free(key); - perf_config_set__delete(set); return -1; } @@ -649,7 +648,8 @@ struct perf_config_set *perf_config_set__new(void) if (set) { INIT_LIST_HEAD(&set->sections); - perf_config(collect_config, set); + if (perf_config(collect_config, set) < 0) + perf_config_set__delete(set); } return set; -- 2.5.0
[PATCH v5 6/9] perf config: Use zfree() instead of free() at perf_config_set__delete()
perf_config_set__delete() delete allocated the config set but the global variable 'config_set' is used all around. So purge and zfree by an address of the global variable , i.e. 'struct perf_config_set **' type instead of using local variable 'set' of which type is 'struct perf_config_set *'. Cc: Namhyung Kim Cc: Jiri Olsa Cc: Masami Hiramatsu Cc: Alexander Shishkin Signed-off-by: Taeung Song --- tools/perf/builtin-config.c | 2 +- tools/perf/util/config.c| 11 +++ tools/perf/util/config.h| 2 +- 3 files changed, 9 insertions(+), 6 deletions(-) diff --git a/tools/perf/builtin-config.c b/tools/perf/builtin-config.c index b3bc01a..f23fe52 100644 --- a/tools/perf/builtin-config.c +++ b/tools/perf/builtin-config.c @@ -105,7 +105,7 @@ int cmd_config(int argc, const char **argv, const char *prefix __maybe_unused) usage_with_options(config_usage, config_options); } - perf_config_set__delete(config_set); + perf_config_set__delete(&config_set); out_err: return ret; } diff --git a/tools/perf/util/config.c b/tools/perf/util/config.c index 21b7ca8..2cae413 100644 --- a/tools/perf/util/config.c +++ b/tools/perf/util/config.c @@ -699,7 +699,7 @@ struct perf_config_set *perf_config_set__new(void) if (set) { INIT_LIST_HEAD(&set->sections); if (perf_config_set__init(set) < 0) { - perf_config_set__delete(set); + perf_config_set__delete(&set); return NULL; } } @@ -741,10 +741,13 @@ static void perf_config_set__purge(struct perf_config_set *set) } } -void perf_config_set__delete(struct perf_config_set *set) +void perf_config_set__delete(struct perf_config_set **set) { - perf_config_set__purge(set); - free(set); + if (*set == NULL) + return; + + perf_config_set__purge(*set); + zfree(set); } /* diff --git a/tools/perf/util/config.h b/tools/perf/util/config.h index ea157a4..271b429 100644 --- a/tools/perf/util/config.h +++ b/tools/perf/util/config.h @@ -23,6 +23,6 @@ struct perf_config_set { extern struct perf_config_set *config_set; struct perf_config_set *perf_config_set__new(void); -void perf_config_set__delete(struct perf_config_set *set); +void perf_config_set__delete(struct perf_config_set **set); #endif /* __PERF_CONFIG_H */ -- 2.5.0
[PATCH v5 5/9] perf config: Add global variable 'config_set'
The config set is prepared by collecting all configs from config files (i.e. user config ~/.perfconfig and system config $(sysconfdir)/perfconfig) so the config set contains all config key-value pairs. We need to use it as global variable to share it. And in near future, the variable will be handled in perf_config() and other functions at util/config.c Cc: Namhyung Kim Cc: Jiri Olsa Cc: Masami Hiramatsu Cc: Alexander Shishkin Signed-off-by: Taeung Song --- tools/perf/builtin-config.c | 9 - tools/perf/util/config.c| 1 + tools/perf/util/config.h| 2 ++ 3 files changed, 7 insertions(+), 5 deletions(-) diff --git a/tools/perf/builtin-config.c b/tools/perf/builtin-config.c index fe1b77f..b3bc01a 100644 --- a/tools/perf/builtin-config.c +++ b/tools/perf/builtin-config.c @@ -62,7 +62,6 @@ static int show_config(struct perf_config_set *set) int cmd_config(int argc, const char **argv, const char *prefix __maybe_unused) { int ret = 0; - struct perf_config_set *set; char *user_config = mkpath("%s/.perfconfig", getenv("HOME")); argc = parse_options(argc, argv, config_options, config_usage, @@ -80,8 +79,8 @@ int cmd_config(int argc, const char **argv, const char *prefix __maybe_unused) else if (use_user_config) config_exclusive_filename = user_config; - set = perf_config_set__new(); - if (!set) { + config_set = perf_config_set__new(); + if (!config_set) { ret = -1; goto out_err; } @@ -92,7 +91,7 @@ int cmd_config(int argc, const char **argv, const char *prefix __maybe_unused) pr_err("Error: takes no arguments\n"); parse_options_usage(config_usage, config_options, "l", 1); } else { - ret = show_config(set); + ret = show_config(config_set); if (ret < 0) { const char * config_filename = config_exclusive_filename; if (!config_exclusive_filename) @@ -106,7 +105,7 @@ int cmd_config(int argc, const char **argv, const char *prefix __maybe_unused) usage_with_options(config_usage, config_options); } - perf_config_set__delete(set); + perf_config_set__delete(config_set); out_err: return ret; } diff --git a/tools/perf/util/config.c b/tools/perf/util/config.c index 6bfa112..21b7ca8 100644 --- a/tools/perf/util/config.c +++ b/tools/perf/util/config.c @@ -28,6 +28,7 @@ static int config_linenr; static int config_file_eof; const char *config_exclusive_filename; +struct perf_config_set *config_set; static int get_next_char(void) { diff --git a/tools/perf/util/config.h b/tools/perf/util/config.h index 22ec626..ea157a4 100644 --- a/tools/perf/util/config.h +++ b/tools/perf/util/config.h @@ -20,6 +20,8 @@ struct perf_config_set { struct list_head sections; }; +extern struct perf_config_set *config_set; + struct perf_config_set *perf_config_set__new(void); void perf_config_set__delete(struct perf_config_set *set); -- 2.5.0
[BUGFIX][PATCH v5 1/9] perf config: Let perf_config() return in case of failure
If a config file has wrong key-value pairs, perf will be forcibly terminated by die() at perf_parse_file() so terminal setting can be crushed because of abnormal termination. For example, If user config file has a wrong value 'red;default' instead of a normal value like 'red, default' for a key 'colors.top', # cat ~/.perfconfig [colors] medium = red;default # wrong value and if running sub-command 'top'. # perf top perf process is terminated by force and terminal setting is broken with a messge like below. Fatal: bad config file line 2 in /root/.perfconfig So fix it. If perf_config() can return on failure without calling die() at perf_parse_file(), this problem is solved. Cc: Namhyung Kim Cc: Jiri Olsa Cc: Masami Hiramatsu Cc: Alexander Shishkin Signed-off-by: Taeung Song --- tools/perf/util/config.c | 16 +++- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/tools/perf/util/config.c b/tools/perf/util/config.c index dad7d82..b500737 100644 --- a/tools/perf/util/config.c +++ b/tools/perf/util/config.c @@ -275,7 +275,8 @@ static int perf_parse_file(config_fn_t fn, void *data) break; } } - die("bad config file line %d in %s", config_linenr, config_file_name); + pr_err("bad config file line %d in %s\n", config_linenr, config_file_name); + return -1; } static int parse_unit_factor(const char *end, unsigned long *val) @@ -479,16 +480,15 @@ static int perf_config_global(void) int perf_config(config_fn_t fn, void *data) { - int ret = 0, found = 0; + int ret = -1; const char *home = NULL; /* Setting $PERF_CONFIG makes perf read _only_ the given config file. */ if (config_exclusive_filename) return perf_config_from_file(fn, config_exclusive_filename, data); if (perf_config_system() && !access(perf_etc_perfconfig(), R_OK)) { - ret += perf_config_from_file(fn, perf_etc_perfconfig(), - data); - found += 1; + if (perf_config_from_file(fn, perf_etc_perfconfig(), data) < 0) + goto out; } home = getenv("HOME"); @@ -514,14 +514,12 @@ int perf_config(config_fn_t fn, void *data) if (!st.st_size) goto out_free; - ret += perf_config_from_file(fn, user_config, data); - found += 1; + ret = perf_config_from_file(fn, user_config, data); + out_free: free(user_config); } out: - if (found == 0) - return -1; return ret; } -- 2.5.0
[PATCH v5 9/9] perf config: Reimplement show_config() using perf_config()
Old show_config() directly use config set so there are many duplicated code with perf_config_set__iter(). So reimplement show_config() using perf_config() that use perf_config_set__iter() with config set that already contains all configs. Cc: Namhyung Kim Cc: Jiri Olsa Cc: Masami Hiramatsu Cc: Alexander Shishkin Signed-off-by: Taeung Song --- tools/perf/builtin-config.c | 29 +++-- 1 file changed, 7 insertions(+), 22 deletions(-) diff --git a/tools/perf/builtin-config.c b/tools/perf/builtin-config.c index 4dab41e..b6ae8ea 100644 --- a/tools/perf/builtin-config.c +++ b/tools/perf/builtin-config.c @@ -33,28 +33,13 @@ static struct option config_options[] = { OPT_END() }; -static int show_config(struct perf_config_set *set) +static int show_config(const char *key, const char *value, + void *cb __maybe_unused) { - struct perf_config_section *section; - struct perf_config_item *item; - struct list_head *sections; - - if (set == NULL) - return -1; - - sections = &set->sections; - if (list_empty(sections)) - return -1; - - list_for_each_entry(section, sections, node) { - list_for_each_entry(item, §ion->items, node) { - char *value = item->value; - - if (value) - printf("%s.%s=%s\n", section->name, - item->name, value); - } - } + if (value) + printf("%s=%s\n", key, value); + else + printf("%s\n", key); return 0; } @@ -96,7 +81,7 @@ int cmd_config(int argc, const char **argv, const char *prefix __maybe_unused) pr_err("Error: takes no arguments\n"); parse_options_usage(config_usage, config_options, "l", 1); } else { - ret = show_config(config_set); + ret = perf_config(show_config, NULL); if (ret < 0) { const char * config_filename = config_exclusive_filename; if (!config_exclusive_filename) -- 2.5.0
[PATCH v5 8/9] perf config: Reset the config set at only 'config' sub-command
When first calling perf_config(), the config set is initialized collecting both user and system config files (i.e. user config ~/.perfconfig and system config $(sysconfdir)/perfconfig) so config set contains not only user config but also system config key-value pairs. (User config has higher priority than system config.) But 'config' sub-command individually use the config set so free the existing config set (i.e. a global variable config_set) before reinstantiating it. And 'config' sub-command have '--user' or '--system' options. To reinitialize with the options, the config set should be reset at the very beginning at cmd_config() Cc: Namhyung Kim Cc: Jiri Olsa Cc: Masami Hiramatsu Cc: Alexander Shishkin Signed-off-by: Taeung Song --- tools/perf/builtin-config.c | 5 + 1 file changed, 5 insertions(+) diff --git a/tools/perf/builtin-config.c b/tools/perf/builtin-config.c index f23fe52..4dab41e 100644 --- a/tools/perf/builtin-config.c +++ b/tools/perf/builtin-config.c @@ -79,6 +79,11 @@ int cmd_config(int argc, const char **argv, const char *prefix __maybe_unused) else if (use_user_config) config_exclusive_filename = user_config; + /* +* Reset the config set at only 'config' sub-command +* because of reinitializing with options config file location. +*/ + perf_config_set__delete(&config_set); config_set = perf_config_set__new(); if (!config_set) { ret = -1; -- 2.5.0
[PATCH v5 4/9] perf config: Use new perf_config_set__init() to initialize config set
Instead of perf_config(), This function initialize config set collecting all configs from config files (i.e. user config ~/.perfconfig and system config $(sysconfdir)/perfconfig). If there are the same config variable both user and system config file, user config has higher priority than system config. Cc: Namhyung Kim Cc: Jiri Olsa Cc: Masami Hiramatsu Cc: Alexander Shishkin Signed-off-by: Taeung Song --- tools/perf/util/config.c | 49 +++- 1 file changed, 48 insertions(+), 1 deletion(-) diff --git a/tools/perf/util/config.c b/tools/perf/util/config.c index 062eeb8..6bfa112 100644 --- a/tools/perf/util/config.c +++ b/tools/perf/util/config.c @@ -646,14 +646,61 @@ out_free: return -1; } +static int perf_config_set__init(struct perf_config_set *set) +{ + int ret = -1; + const char *home = NULL; + + /* Setting $PERF_CONFIG makes perf read _only_ the given config file. */ + if (config_exclusive_filename) + return perf_config_from_file(collect_config, config_exclusive_filename, set); + if (perf_config_system() && !access(perf_etc_perfconfig(), R_OK)) { + if (perf_config_from_file(collect_config, perf_etc_perfconfig(), set) < 0) + goto out; + } + + home = getenv("HOME"); + if (perf_config_global() && home) { + char *user_config = strdup(mkpath("%s/.perfconfig", home)); + struct stat st; + + if (user_config == NULL) { + warning("Not enough memory to process %s/.perfconfig, " + "ignoring it.", home); + goto out; + } + + if (stat(user_config, &st) < 0) + goto out_free; + + if (st.st_uid && (st.st_uid != geteuid())) { + warning("File %s not owned by current user or root, " + "ignoring it.", user_config); + goto out_free; + } + + if (!st.st_size) + goto out_free; + + ret = perf_config_from_file(collect_config, user_config, set); + +out_free: + free(user_config); + } +out: + return ret; +} + struct perf_config_set *perf_config_set__new(void) { struct perf_config_set *set = zalloc(sizeof(*set)); if (set) { INIT_LIST_HEAD(&set->sections); - if (perf_config(collect_config, set) < 0) + if (perf_config_set__init(set) < 0) { perf_config_set__delete(set); + return NULL; + } } return set; -- 2.5.0
Re: [PATCH v4 1/6] perf config: Use new perf_config_set__init() to initialize config set
On 05/31/2016 10:43 PM, Arnaldo Carvalho de Melo wrote: Em Tue, May 31, 2016 at 10:13:43AM +0900, Taeung Song escreveu: Instead of perf_config(), This function initialize config set collecting all configs from config files (i.e. user config ~/.perfconfig and system config $(sysconfdir)/perfconfig). If there are the same config variable both user and system config file, user config has higher priority than system config. Cc: Namhyung Kim Cc: Jiri Olsa Cc: Masami Hiramatsu Cc: Alexander Shishkin Signed-off-by: Taeung Song --- tools/perf/util/config.c | 50 +++- 1 file changed, 49 insertions(+), 1 deletion(-) diff --git a/tools/perf/util/config.c b/tools/perf/util/config.c index dad7d82..5d01899 100644 --- a/tools/perf/util/config.c +++ b/tools/perf/util/config.c @@ -645,13 +645,61 @@ out_free: return -1; } +static int perf_config_set__init(struct perf_config_set *set) +{ + int ret = 0, found = 0; + const char *home = NULL; + + /* Setting $PERF_CONFIG makes perf read _only_ the given config file. */ + if (config_exclusive_filename) + return perf_config_from_file(collect_config, config_exclusive_filename, set); + if (perf_config_system() && !access(perf_etc_perfconfig(), R_OK)) { + ret += perf_config_from_file(collect_config, perf_etc_perfconfig(), set); + found += 1; + } + + home = getenv("HOME"); + if (perf_config_global() && home) { + char *user_config = strdup(mkpath("%s/.perfconfig", home)); + struct stat st; + + if (user_config == NULL) { + warning("Not enough memory to process %s/.perfconfig, " + "ignoring it.", home); + goto out; + } + + if (stat(user_config, &st) < 0) + goto out_free; + + if (st.st_uid && (st.st_uid != geteuid())) { + warning("File %s not owned by current user or root, " + "ignoring it.", user_config); + goto out_free; + } + + if (!st.st_size) + goto out_free; + + ret += perf_config_from_file(collect_config, user_config, set); + found += 1; +out_free: + free(user_config); + } +out: + if (found == 0) + return -1; + return ret; +} + struct perf_config_set *perf_config_set__new(void) { struct perf_config_set *set = zalloc(sizeof(*set)); if (set) { INIT_LIST_HEAD(&set->sections); - perf_config(collect_config, set); + if (perf_config_set__init(set) < 0) + return NULL; So, the usual pattern is: alloc, init, fail? free, return NULL. I thought you could've been deviating from that pattern and went to look at perf_config_set__init() to see if that was doing the freeing in case of failure, which it shouldn't, it isn't, so I guess this is a leak on failure, no? You are right. And I found additional problems. First of all, as you said, if it is failed in perf_config_set__init(), the config set wouldn't be freed so this is a leak on failure. Secondly, if it is failed in perf_parse_file(), perf_parse_file() cannot return because of die() so perf_config_from_file() and perf_config() don't also return. I guess this is abnormal termination without the freeing. (The important point of this problem is die() at perf_parse_file()) Thirdly, there are problems that are related to collect_config(). If perf_config_from_file(collect_config,..) is failed the config set will be freed at collect_config() like below. static int collect_config(const char *var, const char *value, void *perf_config_set) { ... out_free: free(key); perf_config_set__delete(set); return -1; } And then if calling perf_config_from_file(collect_config,..) at perf_config_set__init() again, an error will happen because the config set is NULL at collect_config(). (the error mean NULL pointer exception.) To conclude, First of all, I'll send preparatory PATCH set for this patch to solve the problems i.e. 1) A problem that perf_config() can't return becuase of die() at perf_parse_file() 2) A problem about the freeing config set at collect_config() 3) NULL pointer exception at collect_config() And then I will send changed this patch following above patchset. (to solve a leak when perf_config_set__init() failed) Thanks, Taeung
[PATCH v4 3/6] perf config: Use zfree() instead of free() at perf_config_set__delete()
perf_config_set__delete() delete allocated the config set but the global variable 'config_set' is used all around. So purge and zfree by an address of the global variable , i.e. 'struct perf_config_set **' type instead of using local variable 'set' of which type is 'struct perf_config_set *'. Cc: Namhyung Kim Cc: Jiri Olsa Cc: Masami Hiramatsu Cc: Alexander Shishkin Signed-off-by: Taeung Song --- tools/perf/builtin-config.c | 2 +- tools/perf/util/config.c| 11 +++ tools/perf/util/config.h| 2 +- 3 files changed, 9 insertions(+), 6 deletions(-) diff --git a/tools/perf/builtin-config.c b/tools/perf/builtin-config.c index b3bc01a..f23fe52 100644 --- a/tools/perf/builtin-config.c +++ b/tools/perf/builtin-config.c @@ -105,7 +105,7 @@ int cmd_config(int argc, const char **argv, const char *prefix __maybe_unused) usage_with_options(config_usage, config_options); } - perf_config_set__delete(config_set); + perf_config_set__delete(&config_set); out_err: return ret; } diff --git a/tools/perf/util/config.c b/tools/perf/util/config.c index adf2bad..79ded23 100644 --- a/tools/perf/util/config.c +++ b/tools/perf/util/config.c @@ -642,7 +642,7 @@ static int collect_config(const char *var, const char *value, out_free: free(key); - perf_config_set__delete(set); + perf_config_set__delete(&set); return -1; } @@ -740,10 +740,13 @@ static void perf_config_set__purge(struct perf_config_set *set) } } -void perf_config_set__delete(struct perf_config_set *set) +void perf_config_set__delete(struct perf_config_set **set) { - perf_config_set__purge(set); - free(set); + if (*set == NULL) + return; + + perf_config_set__purge(*set); + zfree(set); } /* diff --git a/tools/perf/util/config.h b/tools/perf/util/config.h index ea157a4..271b429 100644 --- a/tools/perf/util/config.h +++ b/tools/perf/util/config.h @@ -23,6 +23,6 @@ struct perf_config_set { extern struct perf_config_set *config_set; struct perf_config_set *perf_config_set__new(void); -void perf_config_set__delete(struct perf_config_set *set); +void perf_config_set__delete(struct perf_config_set **set); #endif /* __PERF_CONFIG_H */ -- 2.5.0
[PATCH v4 5/6] perf config: Reset the config set at only 'config' sub-command
When first calling perf_config(), the config set is initialized collecting both user and system config files (i.e. user config ~/.perfconfig and system config $(sysconfdir)/perfconfig) so config set contains not only user config but also system config key-value pairs. (User config has higher priority than system config.) But 'config' sub-command individually use the config set so free the existing config set (i.e. a global variable config_set) before reinstantiating it. And 'config' sub-command have '--user' or '--system' options. To reinitialize with the options, the config set should be reset at the very beginning at cmd_config() Cc: Namhyung Kim Cc: Jiri Olsa Cc: Masami Hiramatsu Cc: Alexander Shishkin Signed-off-by: Taeung Song --- tools/perf/builtin-config.c | 5 + 1 file changed, 5 insertions(+) diff --git a/tools/perf/builtin-config.c b/tools/perf/builtin-config.c index f23fe52..4dab41e 100644 --- a/tools/perf/builtin-config.c +++ b/tools/perf/builtin-config.c @@ -79,6 +79,11 @@ int cmd_config(int argc, const char **argv, const char *prefix __maybe_unused) else if (use_user_config) config_exclusive_filename = user_config; + /* +* Reset the config set at only 'config' sub-command +* because of reinitializing with options config file location. +*/ + perf_config_set__delete(&config_set); config_set = perf_config_set__new(); if (!config_set) { ret = -1; -- 2.5.0
[PATCH v4 6/6] perf config: Reimplement show_config() using perf_config()
Old show_config() directly use config set so there are many duplicated code with perf_config_set__iter(). So reimplement show_config() using perf_config() that use perf_config_set__iter() with config set that already contains all configs. Cc: Namhyung Kim Cc: Jiri Olsa Cc: Masami Hiramatsu Cc: Alexander Shishkin Signed-off-by: Taeung Song --- tools/perf/builtin-config.c | 29 +++-- 1 file changed, 7 insertions(+), 22 deletions(-) diff --git a/tools/perf/builtin-config.c b/tools/perf/builtin-config.c index 4dab41e..b6ae8ea 100644 --- a/tools/perf/builtin-config.c +++ b/tools/perf/builtin-config.c @@ -33,28 +33,13 @@ static struct option config_options[] = { OPT_END() }; -static int show_config(struct perf_config_set *set) +static int show_config(const char *key, const char *value, + void *cb __maybe_unused) { - struct perf_config_section *section; - struct perf_config_item *item; - struct list_head *sections; - - if (set == NULL) - return -1; - - sections = &set->sections; - if (list_empty(sections)) - return -1; - - list_for_each_entry(section, sections, node) { - list_for_each_entry(item, §ion->items, node) { - char *value = item->value; - - if (value) - printf("%s.%s=%s\n", section->name, - item->name, value); - } - } + if (value) + printf("%s=%s\n", key, value); + else + printf("%s\n", key); return 0; } @@ -96,7 +81,7 @@ int cmd_config(int argc, const char **argv, const char *prefix __maybe_unused) pr_err("Error: takes no arguments\n"); parse_options_usage(config_usage, config_options, "l", 1); } else { - ret = show_config(config_set); + ret = perf_config(show_config, NULL); if (ret < 0) { const char * config_filename = config_exclusive_filename; if (!config_exclusive_filename) -- 2.5.0
[PATCH v4 2/6] perf config: Add global variable 'config_set'
The config set is prepared by collecting all configs from config files (i.e. user config ~/.perfconfig and system config $(sysconfdir)/perfconfig) so the config set contains all config key-value pairs. We need to use it as global variable to share it. And in near future, the variable will be handled in perf_config() and other functions at util/config.c Cc: Namhyung Kim Cc: Jiri Olsa Cc: Masami Hiramatsu Cc: Alexander Shishkin Signed-off-by: Taeung Song --- tools/perf/builtin-config.c | 9 - tools/perf/util/config.c| 1 + tools/perf/util/config.h| 2 ++ 3 files changed, 7 insertions(+), 5 deletions(-) diff --git a/tools/perf/builtin-config.c b/tools/perf/builtin-config.c index fe1b77f..b3bc01a 100644 --- a/tools/perf/builtin-config.c +++ b/tools/perf/builtin-config.c @@ -62,7 +62,6 @@ static int show_config(struct perf_config_set *set) int cmd_config(int argc, const char **argv, const char *prefix __maybe_unused) { int ret = 0; - struct perf_config_set *set; char *user_config = mkpath("%s/.perfconfig", getenv("HOME")); argc = parse_options(argc, argv, config_options, config_usage, @@ -80,8 +79,8 @@ int cmd_config(int argc, const char **argv, const char *prefix __maybe_unused) else if (use_user_config) config_exclusive_filename = user_config; - set = perf_config_set__new(); - if (!set) { + config_set = perf_config_set__new(); + if (!config_set) { ret = -1; goto out_err; } @@ -92,7 +91,7 @@ int cmd_config(int argc, const char **argv, const char *prefix __maybe_unused) pr_err("Error: takes no arguments\n"); parse_options_usage(config_usage, config_options, "l", 1); } else { - ret = show_config(set); + ret = show_config(config_set); if (ret < 0) { const char * config_filename = config_exclusive_filename; if (!config_exclusive_filename) @@ -106,7 +105,7 @@ int cmd_config(int argc, const char **argv, const char *prefix __maybe_unused) usage_with_options(config_usage, config_options); } - perf_config_set__delete(set); + perf_config_set__delete(config_set); out_err: return ret; } diff --git a/tools/perf/util/config.c b/tools/perf/util/config.c index 5d01899..adf2bad 100644 --- a/tools/perf/util/config.c +++ b/tools/perf/util/config.c @@ -28,6 +28,7 @@ static int config_linenr; static int config_file_eof; const char *config_exclusive_filename; +struct perf_config_set *config_set; static int get_next_char(void) { diff --git a/tools/perf/util/config.h b/tools/perf/util/config.h index 22ec626..ea157a4 100644 --- a/tools/perf/util/config.h +++ b/tools/perf/util/config.h @@ -20,6 +20,8 @@ struct perf_config_set { struct list_head sections; }; +extern struct perf_config_set *config_set; + struct perf_config_set *perf_config_set__new(void); void perf_config_set__delete(struct perf_config_set *set); -- 2.5.0
[PATCH v4 4/6] perf config: Reimplement perf_config() using perf_config_set__iter()
Everytime perf_config() is called, perf_config() always read config files. (i.e. user config '~/.perfconfig' and system config '$(sysconfdir)/perfconfig') But we need to use the config set that already contains all config key-value pairs to avoid this repetitive work reading the config files in perf_config(). (the config set mean a global variable 'config_set') In other words, if new perf_config() is called, only first time 'config_set' is initialized collecting all configs from the config files and it work with perf_config_set__iter(). And free the config set after sub-command work at run_builtin(). If we do, 'config_set' can be reused wherever using perf_config() and a feature of old perf_config() is the same as new perf_config() work without the repetitive work that read the config files. Cc: Namhyung Kim Cc: Jiri Olsa Cc: Wang Nan Cc: Peter Zijlstra Cc: Ingo Molnar Cc: Masami Hiramatsu Cc: Alexander Shishkin Signed-off-by: Taeung Song --- tools/perf/perf.c| 1 + tools/perf/util/cache.h | 1 + tools/perf/util/config.c | 86 +--- 3 files changed, 40 insertions(+), 48 deletions(-) diff --git a/tools/perf/perf.c b/tools/perf/perf.c index 15982ce..058d5dc 100644 --- a/tools/perf/perf.c +++ b/tools/perf/perf.c @@ -391,6 +391,7 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv) perf_env__set_cmdline(&perf_env, argc, argv); status = p->fn(argc, argv, prefix); + perf_config_set__delete(&config_set); exit_browser(status); perf_env__exit(&perf_env); bpf__clear(); diff --git a/tools/perf/util/cache.h b/tools/perf/util/cache.h index 0d814bb..54bbd55 100644 --- a/tools/perf/util/cache.h +++ b/tools/perf/util/cache.h @@ -7,6 +7,7 @@ #include #include "../perf.h" #include "../ui/ui.h" +#include "config.h" #include diff --git a/tools/perf/util/config.c b/tools/perf/util/config.c index 79ded23..bf385ca 100644 --- a/tools/perf/util/config.c +++ b/tools/perf/util/config.c @@ -478,54 +478,6 @@ static int perf_config_global(void) return !perf_env_bool("PERF_CONFIG_NOGLOBAL", 0); } -int perf_config(config_fn_t fn, void *data) -{ - int ret = 0, found = 0; - const char *home = NULL; - - /* Setting $PERF_CONFIG makes perf read _only_ the given config file. */ - if (config_exclusive_filename) - return perf_config_from_file(fn, config_exclusive_filename, data); - if (perf_config_system() && !access(perf_etc_perfconfig(), R_OK)) { - ret += perf_config_from_file(fn, perf_etc_perfconfig(), - data); - found += 1; - } - - home = getenv("HOME"); - if (perf_config_global() && home) { - char *user_config = strdup(mkpath("%s/.perfconfig", home)); - struct stat st; - - if (user_config == NULL) { - warning("Not enough memory to process %s/.perfconfig, " - "ignoring it.", home); - goto out; - } - - if (stat(user_config, &st) < 0) - goto out_free; - - if (st.st_uid && (st.st_uid != geteuid())) { - warning("File %s not owned by current user or root, " - "ignoring it.", user_config); - goto out_free; - } - - if (!st.st_size) - goto out_free; - - ret += perf_config_from_file(fn, user_config, data); - found += 1; -out_free: - free(user_config); - } -out: - if (found == 0) - return -1; - return ret; -} - static struct perf_config_section *find_section(struct list_head *sections, const char *section_name) { @@ -706,6 +658,44 @@ struct perf_config_set *perf_config_set__new(void) return set; } +static int perf_config_set__iter(struct perf_config_set *set, config_fn_t fn, void *data) +{ + struct perf_config_section *section; + struct perf_config_item *item; + struct list_head *sections; + char key[BUFSIZ]; + + if (set == NULL) + return -1; + + sections = &set->sections; + if (list_empty(sections)) + return -1; + + list_for_each_entry(section, sections, node) { + list_for_each_entry(item, §ion->items, node) { + char *value = item->value; + + if (value) { + scnprintf(key, sizeof(key), "%s.%s", + section->name,
[RFC][PATCH v4 0/6] perf config: Reimplement perf_config() using perf_config_set__inter()
Everytime perf_config() is called, perf_config() always read config files. (i.e. user config '~/.perfconfig' and system config '$(sysconfdir)/perfconfig') But we need to use 'struct perf_config_set config_set' variable that already contains all config key-value pairs to avoid this repetitive work in perf_config(). In other words, if new perf_config() is called, only first time 'config_set' is initialized collecting all configs from config files and it work with perf_config_set__iter(). If we do, 'config_set' can be reused wherever using perf_config() and a feature of old perf_config() is the same as new perf_config() work without the repetitive work that read the config files. IMHO, I think this patchset is needed because not only the repetitive work should be avoided but also in near future, it would be smooth to manage perf configs. Most important patch of this patchset is "[PATCH v4 4/6] perf config: Reimplement perf_config() using perf_config_set__iter()" and PATCH 1/6 ~ 3/6 are preparation for it. If you give me any feedback, I'd apprecicated it. :) Thanks, Taeung v4: - Keep perf_config_set__delete() as it is (Arnaldo) - Remove perf_config_set__check() (Arnaldo) - Keep the existing code about the config set at cmd_config() (Arnaldo) v3: - add freeing config set after sub-command work at run_builtin() (Namhyung) - remove needless code about the config set at cmd_config() - add a patch about a global variable 'config_set' v2: - split a patch into several patches - reimplement show_config() using new perf_config() - modify perf_config_set__delete using global variable 'config_set' - reset config set when only 'config' sub-commaned work because of options for config file location Taeung Song (6): perf config: Use new perf_config_set__init() to initialize config set perf config: Add global variable 'config_set' perf config: Use zfree() instead of free() at perf_config_set__delete() perf config: Reimplement perf_config() using perf_config_set__iter() perf config: Reset the config set at only 'config' sub-command perf config: Reimplement show_config() using perf_config() tools/perf/builtin-config.c | 41 +--- tools/perf/perf.c | 1 + tools/perf/util/cache.h | 1 + tools/perf/util/config.c| 148 tools/perf/util/config.h| 4 +- 5 files changed, 115 insertions(+), 80 deletions(-) -- 2.5.0
[PATCH v4 1/6] perf config: Use new perf_config_set__init() to initialize config set
Instead of perf_config(), This function initialize config set collecting all configs from config files (i.e. user config ~/.perfconfig and system config $(sysconfdir)/perfconfig). If there are the same config variable both user and system config file, user config has higher priority than system config. Cc: Namhyung Kim Cc: Jiri Olsa Cc: Masami Hiramatsu Cc: Alexander Shishkin Signed-off-by: Taeung Song --- tools/perf/util/config.c | 50 +++- 1 file changed, 49 insertions(+), 1 deletion(-) diff --git a/tools/perf/util/config.c b/tools/perf/util/config.c index dad7d82..5d01899 100644 --- a/tools/perf/util/config.c +++ b/tools/perf/util/config.c @@ -645,13 +645,61 @@ out_free: return -1; } +static int perf_config_set__init(struct perf_config_set *set) +{ + int ret = 0, found = 0; + const char *home = NULL; + + /* Setting $PERF_CONFIG makes perf read _only_ the given config file. */ + if (config_exclusive_filename) + return perf_config_from_file(collect_config, config_exclusive_filename, set); + if (perf_config_system() && !access(perf_etc_perfconfig(), R_OK)) { + ret += perf_config_from_file(collect_config, perf_etc_perfconfig(), set); + found += 1; + } + + home = getenv("HOME"); + if (perf_config_global() && home) { + char *user_config = strdup(mkpath("%s/.perfconfig", home)); + struct stat st; + + if (user_config == NULL) { + warning("Not enough memory to process %s/.perfconfig, " + "ignoring it.", home); + goto out; + } + + if (stat(user_config, &st) < 0) + goto out_free; + + if (st.st_uid && (st.st_uid != geteuid())) { + warning("File %s not owned by current user or root, " + "ignoring it.", user_config); + goto out_free; + } + + if (!st.st_size) + goto out_free; + + ret += perf_config_from_file(collect_config, user_config, set); + found += 1; +out_free: + free(user_config); + } +out: + if (found == 0) + return -1; + return ret; +} + struct perf_config_set *perf_config_set__new(void) { struct perf_config_set *set = zalloc(sizeof(*set)); if (set) { INIT_LIST_HEAD(&set->sections); - perf_config(collect_config, set); + if (perf_config_set__init(set) < 0) + return NULL; } return set; -- 2.5.0
Re: [PATCH v3 4/7] perf config: Reimplement perf_config() using perf_config_set__iter()
On 05/31/2016 04:32 AM, Arnaldo Carvalho de Melo wrote: Em Tue, May 31, 2016 at 01:44:08AM +0900, Taeung Song escreveu: +static int perf_config_set__iter(struct perf_config_set *set, config_fn_t fn, void *data) +{ + struct perf_config_section *section; + struct perf_config_item *item; + struct list_head *sections; + char key[BUFSIZ]; + + if (set == NULL) + return -1; + return 0; +} +int perf_config(config_fn_t fn, void *data) +{ + if (perf_config_set__check() < 0) + return -1; + return perf_config_set__iter(config_set, fn, data); +} "check" looks too vague, this is equivalent, no? int perf_config(config_fn_t, void *data) { if (config_set == NULL) config_set = perf_config_set__new(); return perf_config_set__iter(config_set, fn, data); } I understood it! I thought __check() function is needed for readability. But I'll remove __check() because it would seem that the function isn't needed as you said. Thanks, Taeung
Re: [PATCH v3 6/7] perf config: Remove needless code about config set at cmd_config()
On 05/31/2016 04:35 AM, Arnaldo Carvalho de Melo wrote: Em Tue, May 31, 2016 at 01:44:10AM +0900, Taeung Song escreveu: show_config() was reimplemented using perf_config() so it isn't needed to use perf_config_set__new() at cmd_config(). And perf_config_set__delete() isn't needed at cmd_config() because of calling the function at run_builtin() when a sub-command finished. And it isn't also needed to declare 'config_set' as extern variable because the variable is only handled at util/config.c from now on. But the existing code looks more natural, i.e. before dealing with the config_set, we try instantiating it, handling errors at constructor calling time, etc. Then, finally, calling the destructor when we don't need it anymore. I understood it. The important thing is when it is instantiated or isn't needed, not where it is handled (i.e. at util/config.c). right ? So we need to use the config set as extern variable and it would be better to keep the existing code at cmd_config() as you said. I'll send v4 with modified code soon ! :-) Thanks, Taeung Cc: Namhyung Kim Cc: Jiri Olsa Cc: Masami Hiramatsu Cc: Alexander Shishkin Signed-off-by: Taeung Song --- tools/perf/builtin-config.c | 8 tools/perf/util/config.h| 2 -- 2 files changed, 10 deletions(-) diff --git a/tools/perf/builtin-config.c b/tools/perf/builtin-config.c index cf6c396..412c725 100644 --- a/tools/perf/builtin-config.c +++ b/tools/perf/builtin-config.c @@ -64,12 +64,6 @@ int cmd_config(int argc, const char **argv, const char *prefix __maybe_unused) else if (use_user_config) config_exclusive_filename = user_config; - config_set = perf_config_set__new(); - if (!config_set) { - ret = -1; - goto out_err; - } - switch (actions) { case ACTION_LIST: if (argc) { @@ -90,7 +84,5 @@ int cmd_config(int argc, const char **argv, const char *prefix __maybe_unused) usage_with_options(config_usage, config_options); } - perf_config_set__delete(); -out_err: return ret; } diff --git a/tools/perf/util/config.h b/tools/perf/util/config.h index e9b47b3..be4e603 100644 --- a/tools/perf/util/config.h +++ b/tools/perf/util/config.h @@ -20,8 +20,6 @@ struct perf_config_set { struct list_head sections; }; -extern struct perf_config_set *config_set; - struct perf_config_set *perf_config_set__new(void); void perf_config_set__delete(void); -- 2.5.0
Re: [PATCH v3 3/7] perf config: Modify perf_config_set__delete() using global variable 'config_set'
Hi, Arnaldo :-D On 05/31/2016 04:29 AM, Arnaldo Carvalho de Melo wrote: Em Tue, May 31, 2016 at 01:44:07AM +0900, Taeung Song escreveu: This function deleted allocated config set but the global variable 'config_set' is used all around so this directly remove 'config_set' instead of using local variable 'set'. Keep it like it is, i.e. foo__delete() acts on something returned by foo__new(), for consistency. We may want to, for instance, have two instances returned by foo__new() and then call the destructor for each. I understood it. I'll just keep the function as it is i.e. void perf_config_set__delete(struct perf_config_set *set) , for consistency. And at [PATCH v3 4/7] perf config: Reimplement perf_config() using perf_config_set__iter() when calling the function at run_builtin(), I'll modify it like below. @@ -391,6 +391,7 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv) perf_env__set_cmdline(&perf_env, argc, argv); status = p->fn(argc, argv, prefix); + perf_config_set__delete(config_set); exit_browser(status); perf_env__exit(&perf_env); bpf__clear(); Thanks, Taeung Cc: Namhyung Kim Cc: Jiri Olsa Cc: Masami Hiramatsu Cc: Alexander Shishkin Signed-off-by: Taeung Song --- tools/perf/builtin-config.c | 2 +- tools/perf/util/config.c| 8 tools/perf/util/config.h| 2 +- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/tools/perf/builtin-config.c b/tools/perf/builtin-config.c index b3bc01a..255015e 100644 --- a/tools/perf/builtin-config.c +++ b/tools/perf/builtin-config.c @@ -105,7 +105,7 @@ int cmd_config(int argc, const char **argv, const char *prefix __maybe_unused) usage_with_options(config_usage, config_options); } - perf_config_set__delete(config_set); + perf_config_set__delete(); out_err: return ret; } diff --git a/tools/perf/util/config.c b/tools/perf/util/config.c index adf2bad..68def9a 100644 --- a/tools/perf/util/config.c +++ b/tools/perf/util/config.c @@ -642,7 +642,7 @@ static int collect_config(const char *var, const char *value, out_free: free(key); - perf_config_set__delete(set); + perf_config_set__delete(); return -1; } @@ -740,10 +740,10 @@ static void perf_config_set__purge(struct perf_config_set *set) } } -void perf_config_set__delete(struct perf_config_set *set) +void perf_config_set__delete(void) { - perf_config_set__purge(set); - free(set); + perf_config_set__purge(config_set); + zfree(&config_set); } /* diff --git a/tools/perf/util/config.h b/tools/perf/util/config.h index ea157a4..e9b47b3 100644 --- a/tools/perf/util/config.h +++ b/tools/perf/util/config.h @@ -23,6 +23,6 @@ struct perf_config_set { extern struct perf_config_set *config_set; struct perf_config_set *perf_config_set__new(void); -void perf_config_set__delete(struct perf_config_set *set); +void perf_config_set__delete(void); #endif /* __PERF_CONFIG_H */ -- 2.5.0
[PATCH v3 3/7] perf config: Modify perf_config_set__delete() using global variable 'config_set'
This function deleted allocated config set but the global variable 'config_set' is used all around so this directly remove 'config_set' instead of using local variable 'set'. Cc: Namhyung Kim Cc: Jiri Olsa Cc: Masami Hiramatsu Cc: Alexander Shishkin Signed-off-by: Taeung Song --- tools/perf/builtin-config.c | 2 +- tools/perf/util/config.c| 8 tools/perf/util/config.h| 2 +- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/tools/perf/builtin-config.c b/tools/perf/builtin-config.c index b3bc01a..255015e 100644 --- a/tools/perf/builtin-config.c +++ b/tools/perf/builtin-config.c @@ -105,7 +105,7 @@ int cmd_config(int argc, const char **argv, const char *prefix __maybe_unused) usage_with_options(config_usage, config_options); } - perf_config_set__delete(config_set); + perf_config_set__delete(); out_err: return ret; } diff --git a/tools/perf/util/config.c b/tools/perf/util/config.c index adf2bad..68def9a 100644 --- a/tools/perf/util/config.c +++ b/tools/perf/util/config.c @@ -642,7 +642,7 @@ static int collect_config(const char *var, const char *value, out_free: free(key); - perf_config_set__delete(set); + perf_config_set__delete(); return -1; } @@ -740,10 +740,10 @@ static void perf_config_set__purge(struct perf_config_set *set) } } -void perf_config_set__delete(struct perf_config_set *set) +void perf_config_set__delete(void) { - perf_config_set__purge(set); - free(set); + perf_config_set__purge(config_set); + zfree(&config_set); } /* diff --git a/tools/perf/util/config.h b/tools/perf/util/config.h index ea157a4..e9b47b3 100644 --- a/tools/perf/util/config.h +++ b/tools/perf/util/config.h @@ -23,6 +23,6 @@ struct perf_config_set { extern struct perf_config_set *config_set; struct perf_config_set *perf_config_set__new(void); -void perf_config_set__delete(struct perf_config_set *set); +void perf_config_set__delete(void); #endif /* __PERF_CONFIG_H */ -- 2.5.0
[PATCH v3 5/7] perf config: Reimplement show_config() using perf_config()
Old show_config() directly use config set so there are many duplicated code with perf_config_set__iter(). So reimplement show_config() using perf_config() that use perf_config_set__iter() with config set that already contains all configs. Cc: Namhyung Kim Cc: Jiri Olsa Cc: Masami Hiramatsu Cc: Alexander Shishkin Signed-off-by: Taeung Song --- tools/perf/builtin-config.c | 29 +++-- 1 file changed, 7 insertions(+), 22 deletions(-) diff --git a/tools/perf/builtin-config.c b/tools/perf/builtin-config.c index 255015e..cf6c396 100644 --- a/tools/perf/builtin-config.c +++ b/tools/perf/builtin-config.c @@ -33,28 +33,13 @@ static struct option config_options[] = { OPT_END() }; -static int show_config(struct perf_config_set *set) +static int show_config(const char *key, const char *value, + void *cb __maybe_unused) { - struct perf_config_section *section; - struct perf_config_item *item; - struct list_head *sections; - - if (set == NULL) - return -1; - - sections = &set->sections; - if (list_empty(sections)) - return -1; - - list_for_each_entry(section, sections, node) { - list_for_each_entry(item, §ion->items, node) { - char *value = item->value; - - if (value) - printf("%s.%s=%s\n", section->name, - item->name, value); - } - } + if (value) + printf("%s=%s\n", key, value); + else + printf("%s\n", key); return 0; } @@ -91,7 +76,7 @@ int cmd_config(int argc, const char **argv, const char *prefix __maybe_unused) pr_err("Error: takes no arguments\n"); parse_options_usage(config_usage, config_options, "l", 1); } else { - ret = show_config(config_set); + ret = perf_config(show_config, NULL); if (ret < 0) { const char * config_filename = config_exclusive_filename; if (!config_exclusive_filename) -- 2.5.0
[PATCH v3 4/7] perf config: Reimplement perf_config() using perf_config_set__iter()
Everytime perf_config() is called, perf_config() always read config files. (i.e. user config '~/.perfconfig' and system config '$(sysconfdir)/perfconfig') But we need to use the config set that already contains all config key-value pairs to avoid this repetitive work reading the config files in perf_config(). (the config set mean a global variable 'config_set') In other words, if new perf_config() is called, only first time 'config_set' is initialized collecting all configs from the config files and it work with perf_config_set__iter(). And free the config set after sub-command work at run_builtin(). If we do, 'config_set' can be reused wherever using perf_config() and a feature of old perf_config() is the same as new perf_config() work without the repetitive work that read the config files. Cc: Namhyung Kim Cc: Jiri Olsa Cc: Wang Nan Cc: Peter Zijlstra Cc: Ingo Molnar Cc: Masami Hiramatsu Cc: Alexander Shishkin Signed-off-by: Taeung Song --- tools/perf/perf.c| 1 + tools/perf/util/cache.h | 1 + tools/perf/util/config.c | 97 3 files changed, 51 insertions(+), 48 deletions(-) diff --git a/tools/perf/perf.c b/tools/perf/perf.c index 15982ce..32e54d9 100644 --- a/tools/perf/perf.c +++ b/tools/perf/perf.c @@ -391,6 +391,7 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv) perf_env__set_cmdline(&perf_env, argc, argv); status = p->fn(argc, argv, prefix); + perf_config_set__delete(); exit_browser(status); perf_env__exit(&perf_env); bpf__clear(); diff --git a/tools/perf/util/cache.h b/tools/perf/util/cache.h index 0d814bb..54bbd55 100644 --- a/tools/perf/util/cache.h +++ b/tools/perf/util/cache.h @@ -7,6 +7,7 @@ #include #include "../perf.h" #include "../ui/ui.h" +#include "config.h" #include diff --git a/tools/perf/util/config.c b/tools/perf/util/config.c index 68def9a..abfe1b2 100644 --- a/tools/perf/util/config.c +++ b/tools/perf/util/config.c @@ -478,54 +478,6 @@ static int perf_config_global(void) return !perf_env_bool("PERF_CONFIG_NOGLOBAL", 0); } -int perf_config(config_fn_t fn, void *data) -{ - int ret = 0, found = 0; - const char *home = NULL; - - /* Setting $PERF_CONFIG makes perf read _only_ the given config file. */ - if (config_exclusive_filename) - return perf_config_from_file(fn, config_exclusive_filename, data); - if (perf_config_system() && !access(perf_etc_perfconfig(), R_OK)) { - ret += perf_config_from_file(fn, perf_etc_perfconfig(), - data); - found += 1; - } - - home = getenv("HOME"); - if (perf_config_global() && home) { - char *user_config = strdup(mkpath("%s/.perfconfig", home)); - struct stat st; - - if (user_config == NULL) { - warning("Not enough memory to process %s/.perfconfig, " - "ignoring it.", home); - goto out; - } - - if (stat(user_config, &st) < 0) - goto out_free; - - if (st.st_uid && (st.st_uid != geteuid())) { - warning("File %s not owned by current user or root, " - "ignoring it.", user_config); - goto out_free; - } - - if (!st.st_size) - goto out_free; - - ret += perf_config_from_file(fn, user_config, data); - found += 1; -out_free: - free(user_config); - } -out: - if (found == 0) - return -1; - return ret; -} - static struct perf_config_section *find_section(struct list_head *sections, const char *section_name) { @@ -706,6 +658,55 @@ struct perf_config_set *perf_config_set__new(void) return set; } +static int perf_config_set__check(void) +{ + if (config_set != NULL) + return 0; + + config_set = perf_config_set__new(); + if (!config_set) + return -1; + + return 0; +} + +static int perf_config_set__iter(struct perf_config_set *set, config_fn_t fn, void *data) +{ + struct perf_config_section *section; + struct perf_config_item *item; + struct list_head *sections; + char key[BUFSIZ]; + + if (set == NULL) + return -1; + + sections = &set->sections; + if (list_empty(sections)) + return -1; + + list_for_each_entry(section, sections, node) { + list_for_each_entry(item, §ion->items, node) { +
[PATCH v3 7/7] perf config: Reset the config set at only 'config' sub-command
When first calling perf_config(), config set is initialized collecting both user and system config files (i.e. user config ~/.perfconfig and system config $(sysconfdir)/perfconfig) so config set contains not only user config but also system config key-value pairs. (User config has higher priority than system config.) But 'config' sub-command have '--user' or '--system' options. The options is to select a particular config file location so the config set should be reset before 'config' sub-command work. User config file: # cat ~/.perfconfig [annotate] hide_src_code = false [tui] report = on System config file: # cat /usr/local/etc/perfconfig [annotate] hide_src_code = true Before: # perf config --user --list annotate.hide_src_code=false ui.report=on # perf config --system --list annotate.hide_src_code=false tui.report=on After: # perf config --user --list annotate.hide_src_code=false tui.report=on # perf config --system --list annotate.hide_src_code=true Cc: Namhyung Kim Cc: Jiri Olsa Cc: Masami Hiramatsu Cc: Alexander Shishkin Signed-off-by: Taeung Song --- tools/perf/builtin-config.c | 6 ++ 1 file changed, 6 insertions(+) diff --git a/tools/perf/builtin-config.c b/tools/perf/builtin-config.c index 412c725..5615631 100644 --- a/tools/perf/builtin-config.c +++ b/tools/perf/builtin-config.c @@ -64,6 +64,12 @@ int cmd_config(int argc, const char **argv, const char *prefix __maybe_unused) else if (use_user_config) config_exclusive_filename = user_config; + /* +* Reset the config set at only 'config' sub-command +* because of options config file location. +*/ + perf_config_set__delete(); + switch (actions) { case ACTION_LIST: if (argc) { -- 2.5.0
[PATCH v3 6/7] perf config: Remove needless code about config set at cmd_config()
show_config() was reimplemented using perf_config() so it isn't needed to use perf_config_set__new() at cmd_config(). And perf_config_set__delete() isn't needed at cmd_config() because of calling the function at run_builtin() when a sub-command finished. And it isn't also needed to declare 'config_set' as extern variable because the variable is only handled at util/config.c from now on. Cc: Namhyung Kim Cc: Jiri Olsa Cc: Masami Hiramatsu Cc: Alexander Shishkin Signed-off-by: Taeung Song --- tools/perf/builtin-config.c | 8 tools/perf/util/config.h| 2 -- 2 files changed, 10 deletions(-) diff --git a/tools/perf/builtin-config.c b/tools/perf/builtin-config.c index cf6c396..412c725 100644 --- a/tools/perf/builtin-config.c +++ b/tools/perf/builtin-config.c @@ -64,12 +64,6 @@ int cmd_config(int argc, const char **argv, const char *prefix __maybe_unused) else if (use_user_config) config_exclusive_filename = user_config; - config_set = perf_config_set__new(); - if (!config_set) { - ret = -1; - goto out_err; - } - switch (actions) { case ACTION_LIST: if (argc) { @@ -90,7 +84,5 @@ int cmd_config(int argc, const char **argv, const char *prefix __maybe_unused) usage_with_options(config_usage, config_options); } - perf_config_set__delete(); -out_err: return ret; } diff --git a/tools/perf/util/config.h b/tools/perf/util/config.h index e9b47b3..be4e603 100644 --- a/tools/perf/util/config.h +++ b/tools/perf/util/config.h @@ -20,8 +20,6 @@ struct perf_config_set { struct list_head sections; }; -extern struct perf_config_set *config_set; - struct perf_config_set *perf_config_set__new(void); void perf_config_set__delete(void); -- 2.5.0
[RFC][PATCH v3 0/7] perf config: Reimplement perf_config() using perf_config_set__inter()
Everytime perf_config() is called, perf_config() always read config files. (i.e. user config '~/.perfconfig' and system config '$(sysconfdir)/perfconfig') But we need to use 'struct perf_config_set config_set' variable that already contains all config key-value pairs to avoid this repetitive work in perf_config(). In other words, if new perf_config() is called, only first time 'config_set' is initialized collecting all configs from config files and it work with perf_config_set__iter(). If we do, 'config_set' can be reused wherever using perf_config() and a feature of old perf_config() is the same as new perf_config() work without the repetitive work that read the config files. IMHO, I think this patchset is needed because not only the repetitive work should be avoided but also in near future, it would be smooth to manage perf configs. Most important patch of this patchset is "[PATCH v3 4/7] perf config: Reimplement perf_config() using perf_config_set__iter()" and PATCH 1/7 ~ 3/7 are preparation for it. If you give me any feedback, I'd apprecicated it. :) Thanks, Taeung v3: - add freeing config set after sub-command work at run_builtin() (Namhyung) - remove needless code about config set at cmd_config() - add a patch about a global variable 'config_set' v2: - split a patch into several patches - reimplement show_config() using new perf_config() - modify perf_config_set__delete using global variable 'config_set' - reset config set when only 'config' sub-commaned work because of options for config file location Taeung Song (7): perf config: Use new perf_config_set__init() to initialize config set perf config: Add global variable 'config_set' perf config: Modify perf_config_set__delete() using global variable 'config_set' perf config: Reimplement perf_config() using perf_config_set__iter() perf config: Reimplement show_config() using perf_config() perf config: Remove needless code about config set at cmd_config() perf config: Reset the config set at only 'config' sub-command tools/perf/builtin-config.c | 42 tools/perf/perf.c | 1 + tools/perf/util/cache.h | 1 + tools/perf/util/config.c| 156 +--- tools/perf/util/config.h| 2 +- 5 files changed, 118 insertions(+), 84 deletions(-) -- 2.5.0
[PATCH v3 2/7] perf config: Add global variable 'config_set'
The config set is prepared by collecting all configs from config files (i.e. user config ~/.perfconfig and system config $(sysconfdir)/perfconfig) so the config set contains all config key-value pairs. We need to use it as global variable to share it. And in near future, the variable will be handled in perf_config() and other functions at util/config.c Cc: Namhyung Kim Cc: Jiri Olsa Cc: Masami Hiramatsu Cc: Alexander Shishkin Signed-off-by: Taeung Song --- tools/perf/builtin-config.c | 9 - tools/perf/util/config.c| 1 + tools/perf/util/config.h| 2 ++ 3 files changed, 7 insertions(+), 5 deletions(-) diff --git a/tools/perf/builtin-config.c b/tools/perf/builtin-config.c index fe1b77f..b3bc01a 100644 --- a/tools/perf/builtin-config.c +++ b/tools/perf/builtin-config.c @@ -62,7 +62,6 @@ static int show_config(struct perf_config_set *set) int cmd_config(int argc, const char **argv, const char *prefix __maybe_unused) { int ret = 0; - struct perf_config_set *set; char *user_config = mkpath("%s/.perfconfig", getenv("HOME")); argc = parse_options(argc, argv, config_options, config_usage, @@ -80,8 +79,8 @@ int cmd_config(int argc, const char **argv, const char *prefix __maybe_unused) else if (use_user_config) config_exclusive_filename = user_config; - set = perf_config_set__new(); - if (!set) { + config_set = perf_config_set__new(); + if (!config_set) { ret = -1; goto out_err; } @@ -92,7 +91,7 @@ int cmd_config(int argc, const char **argv, const char *prefix __maybe_unused) pr_err("Error: takes no arguments\n"); parse_options_usage(config_usage, config_options, "l", 1); } else { - ret = show_config(set); + ret = show_config(config_set); if (ret < 0) { const char * config_filename = config_exclusive_filename; if (!config_exclusive_filename) @@ -106,7 +105,7 @@ int cmd_config(int argc, const char **argv, const char *prefix __maybe_unused) usage_with_options(config_usage, config_options); } - perf_config_set__delete(set); + perf_config_set__delete(config_set); out_err: return ret; } diff --git a/tools/perf/util/config.c b/tools/perf/util/config.c index 5d01899..adf2bad 100644 --- a/tools/perf/util/config.c +++ b/tools/perf/util/config.c @@ -28,6 +28,7 @@ static int config_linenr; static int config_file_eof; const char *config_exclusive_filename; +struct perf_config_set *config_set; static int get_next_char(void) { diff --git a/tools/perf/util/config.h b/tools/perf/util/config.h index 22ec626..ea157a4 100644 --- a/tools/perf/util/config.h +++ b/tools/perf/util/config.h @@ -20,6 +20,8 @@ struct perf_config_set { struct list_head sections; }; +extern struct perf_config_set *config_set; + struct perf_config_set *perf_config_set__new(void); void perf_config_set__delete(struct perf_config_set *set); -- 2.5.0
[PATCH v3 1/7] perf config: Use new perf_config_set__init() to initialize config set
Instead of perf_config(), This function initialize config set collecting all configs from config files (i.e. user config ~/.perfconfig and system config $(sysconfdir)/perfconfig). If there are the same config variable both user and system config file, user config has higher priority than system config. Cc: Namhyung Kim Cc: Jiri Olsa Cc: Masami Hiramatsu Cc: Alexander Shishkin Signed-off-by: Taeung Song --- tools/perf/util/config.c | 50 +++- 1 file changed, 49 insertions(+), 1 deletion(-) diff --git a/tools/perf/util/config.c b/tools/perf/util/config.c index dad7d82..5d01899 100644 --- a/tools/perf/util/config.c +++ b/tools/perf/util/config.c @@ -645,13 +645,61 @@ out_free: return -1; } +static int perf_config_set__init(struct perf_config_set *set) +{ + int ret = 0, found = 0; + const char *home = NULL; + + /* Setting $PERF_CONFIG makes perf read _only_ the given config file. */ + if (config_exclusive_filename) + return perf_config_from_file(collect_config, config_exclusive_filename, set); + if (perf_config_system() && !access(perf_etc_perfconfig(), R_OK)) { + ret += perf_config_from_file(collect_config, perf_etc_perfconfig(), set); + found += 1; + } + + home = getenv("HOME"); + if (perf_config_global() && home) { + char *user_config = strdup(mkpath("%s/.perfconfig", home)); + struct stat st; + + if (user_config == NULL) { + warning("Not enough memory to process %s/.perfconfig, " + "ignoring it.", home); + goto out; + } + + if (stat(user_config, &st) < 0) + goto out_free; + + if (st.st_uid && (st.st_uid != geteuid())) { + warning("File %s not owned by current user or root, " + "ignoring it.", user_config); + goto out_free; + } + + if (!st.st_size) + goto out_free; + + ret += perf_config_from_file(collect_config, user_config, set); + found += 1; +out_free: + free(user_config); + } +out: + if (found == 0) + return -1; + return ret; +} + struct perf_config_set *perf_config_set__new(void) { struct perf_config_set *set = zalloc(sizeof(*set)); if (set) { INIT_LIST_HEAD(&set->sections); - perf_config(collect_config, set); + if (perf_config_set__init(set) < 0) + return NULL; } return set; -- 2.5.0
Re: [PATCH v2] perf tools: Add arch/*/include/generated/ to .gitignore
On 05/27/2016 11:44 PM, Arnaldo Carvalho de Melo wrote: Em Fri, May 27, 2016 at 07:01:14PM +0900, Taeung Song escreveu: Commit 1b700c9975008615ad470cf79acc8455ce60a695 ("perf tools: Build syscall table .c header from kernel's syscall_64.tbl") that automatically generate per-arch syscall table arrays e.g. arch/x86/include/generated/asm/syscalls_64.c So add this directory to .gitignore Cc: Namhyung Kim Cc: Jiri Olsa Cc: Adrian Hunter Cc: David Ahern Cc: Wang Nan Cc: Alexander Shishkin Thanks, applied, after adding a Fixes: tag: Fixes: 1b700c997500 ("perf tools: Build syscall table .c header from kernel's syscall_64.tbl") - Arnaldo Thank you !! :) Taeung
[PATCH v2] perf tools: Add arch/*/include/generated/ to .gitignore
Commit 1b700c9975008615ad470cf79acc8455ce60a695 ("perf tools: Build syscall table .c header from kernel's syscall_64.tbl") that automatically generate per-arch syscall table arrays e.g. arch/x86/include/generated/asm/syscalls_64.c So add this directory to .gitignore Cc: Namhyung Kim Cc: Jiri Olsa Cc: Adrian Hunter Cc: David Ahern Cc: Wang Nan Cc: Alexander Shishkin Signed-off-by: Taeung Song --- tools/perf/.gitignore | 1 + 1 file changed, 1 insertion(+) diff --git a/tools/perf/.gitignore b/tools/perf/.gitignore index 3d1bb80..3db3db9 100644 --- a/tools/perf/.gitignore +++ b/tools/perf/.gitignore @@ -30,3 +30,4 @@ config.mak.autogen *.pyo .config-detected util/intel-pt-decoder/inat-tables.c +arch/*/include/generated/ -- 2.5.0
Re: [RESEND PATCH] perf tools: Add arch/*/include/generated/ to .gitignore
Hi, Wangnan Thank you !! I'll resend modified patch, now Thanks, Taeung On 05/27/2016 06:53 PM, Wangnan (F) wrote: On 2016/5/27 17:15, Taeung Song wrote: Hi, Arnaldo If you have a little time, could you check this simple patch ? This patch is minor but everytime I build tools/perf code, untracked file(arch/*/include/generated/) is always generated.. like below taeung ~/git/linux-perf/tools/perf :> make -j4 BUILD: Doing 'make -j4' parallel build ... INSTALL python-scripts INSTALL perf_completion-script INSTALL perf-tip taeung ~/git/linux-perf/tools/perf :> git status On branch master Your branch is up-to-date with 'origin/master'. Untracked files: (use "git add ..." to include in what will be committed) arch/x86/include/generated/ nothing added to commit but untracked files present (use "git add" to track) Thanks, Taeung On 05/24/2016 05:13 PM, Taeung Song wrote: Commit 1b700c9975008615ad470cf79acc8455ce60a695 ("perf tools: Build syscall table .c header from kernel's syscall_64.tbl") that automatically generate per-arch syscall table arrays e.g. arch/x86/include/generated/asm/syscalls_64.c So add this directory to .gitignore Cc: Namhyung Kim Cc: Jiri Olsa Cc: Masami Hiramatsu Cc: Adrian Hunter Cc: David Ahern Cc: Wang Nan Cc: Alexander Shishkin Signed-off-by: Taeung Song --- tools/perf/.gitignore | 1 + 1 file changed, 1 insertion(+) diff --git a/tools/perf/.gitignore b/tools/perf/.gitignore index 3d1bb80..4bef135 100644 --- a/tools/perf/.gitignore +++ b/tools/perf/.gitignore @@ -30,3 +30,4 @@ config.mak.autogen *.pyo .config-detected util/intel-pt-decoder/inat-tables.c +arch/*/include/generated/ \ No newline at end of file Please add a newline here. Thank you.
Re: [RESEND PATCH] perf tools: Add arch/*/include/generated/ to .gitignore
Hi, Arnaldo If you have a little time, could you check this simple patch ? This patch is minor but everytime I build tools/perf code, untracked file(arch/*/include/generated/) is always generated.. like below taeung ~/git/linux-perf/tools/perf :> make -j4 BUILD: Doing 'make -j4' parallel build ... INSTALL python-scripts INSTALL perf_completion-script INSTALL perf-tip taeung ~/git/linux-perf/tools/perf :> git status On branch master Your branch is up-to-date with 'origin/master'. Untracked files: (use "git add ..." to include in what will be committed) arch/x86/include/generated/ nothing added to commit but untracked files present (use "git add" to track) Thanks, Taeung On 05/24/2016 05:13 PM, Taeung Song wrote: Commit 1b700c9975008615ad470cf79acc8455ce60a695 ("perf tools: Build syscall table .c header from kernel's syscall_64.tbl") that automatically generate per-arch syscall table arrays e.g. arch/x86/include/generated/asm/syscalls_64.c So add this directory to .gitignore Cc: Namhyung Kim Cc: Jiri Olsa Cc: Masami Hiramatsu Cc: Adrian Hunter Cc: David Ahern Cc: Wang Nan Cc: Alexander Shishkin Signed-off-by: Taeung Song --- tools/perf/.gitignore | 1 + 1 file changed, 1 insertion(+) diff --git a/tools/perf/.gitignore b/tools/perf/.gitignore index 3d1bb80..4bef135 100644 --- a/tools/perf/.gitignore +++ b/tools/perf/.gitignore @@ -30,3 +30,4 @@ config.mak.autogen *.pyo .config-detected util/intel-pt-decoder/inat-tables.c +arch/*/include/generated/ \ No newline at end of file
[RESEND PATCH] perf tools: Add arch/*/include/generated/ to .gitignore
Commit 1b700c9975008615ad470cf79acc8455ce60a695 ("perf tools: Build syscall table .c header from kernel's syscall_64.tbl") that automatically generate per-arch syscall table arrays e.g. arch/x86/include/generated/asm/syscalls_64.c So add this directory to .gitignore Cc: Namhyung Kim Cc: Jiri Olsa Cc: Masami Hiramatsu Cc: Adrian Hunter Cc: David Ahern Cc: Wang Nan Cc: Alexander Shishkin Signed-off-by: Taeung Song --- tools/perf/.gitignore | 1 + 1 file changed, 1 insertion(+) diff --git a/tools/perf/.gitignore b/tools/perf/.gitignore index 3d1bb80..4bef135 100644 --- a/tools/perf/.gitignore +++ b/tools/perf/.gitignore @@ -30,3 +30,4 @@ config.mak.autogen *.pyo .config-detected util/intel-pt-decoder/inat-tables.c +arch/*/include/generated/ \ No newline at end of file -- 2.5.0
[PATCH v2 4/5] perf config: Reimplement show_config() using perf_config()
Old show_config() directly use config set so there are many duplicated code with perf_config_set__iter(). So reimplement show_config() using perf_config() that use perf_config_set__iter() with config set that already contains all configs. Cc: Namhyung Kim Cc: Jiri Olsa Cc: Masami Hiramatsu Cc: Alexander Shishkin Signed-off-by: Taeung Song --- tools/perf/builtin-config.c | 29 +++-- 1 file changed, 7 insertions(+), 22 deletions(-) diff --git a/tools/perf/builtin-config.c b/tools/perf/builtin-config.c index 8eef3fb..4a61411 100644 --- a/tools/perf/builtin-config.c +++ b/tools/perf/builtin-config.c @@ -33,28 +33,13 @@ static struct option config_options[] = { OPT_END() }; -static int show_config(struct perf_config_set *set) +static int show_config(const char *key, const char *value, + void *cb __maybe_unused) { - struct perf_config_section *section; - struct perf_config_item *item; - struct list_head *sections; - - if (set == NULL) - return -1; - - sections = &set->sections; - if (list_empty(sections)) - return -1; - - list_for_each_entry(section, sections, node) { - list_for_each_entry(item, §ion->items, node) { - char *value = item->value; - - if (value) - printf("%s.%s=%s\n", section->name, - item->name, value); - } - } + if (value) + printf("%s=%s\n", key, value); + else + printf("%s\n", key); return 0; } @@ -92,7 +77,7 @@ int cmd_config(int argc, const char **argv, const char *prefix __maybe_unused) pr_err("Error: takes no arguments\n"); parse_options_usage(config_usage, config_options, "l", 1); } else { - ret = show_config(set); + ret = perf_config(show_config, NULL); if (ret < 0) { const char * config_filename = config_exclusive_filename; if (!config_exclusive_filename) -- 2.5.0
[PATCH v2 1/5] perf config: Use new perf_config_set__init() to initialize config set
Instead of perf_config(), This function initialize config set collecting all configs from config files (i.e. user config ~/.perfconfig and system config $(sysconfdir)/perfconfig). If there are the same config variable both user and system config file, user config has higher priority than system config. Cc: Namhyung Kim Cc: Jiri Olsa Cc: Masami Hiramatsu Cc: Alexander Shishkin Signed-off-by: Taeung Song --- tools/perf/util/config.c | 50 +++- 1 file changed, 49 insertions(+), 1 deletion(-) diff --git a/tools/perf/util/config.c b/tools/perf/util/config.c index dad7d82..5d01899 100644 --- a/tools/perf/util/config.c +++ b/tools/perf/util/config.c @@ -645,13 +645,61 @@ out_free: return -1; } +static int perf_config_set__init(struct perf_config_set *set) +{ + int ret = 0, found = 0; + const char *home = NULL; + + /* Setting $PERF_CONFIG makes perf read _only_ the given config file. */ + if (config_exclusive_filename) + return perf_config_from_file(collect_config, config_exclusive_filename, set); + if (perf_config_system() && !access(perf_etc_perfconfig(), R_OK)) { + ret += perf_config_from_file(collect_config, perf_etc_perfconfig(), set); + found += 1; + } + + home = getenv("HOME"); + if (perf_config_global() && home) { + char *user_config = strdup(mkpath("%s/.perfconfig", home)); + struct stat st; + + if (user_config == NULL) { + warning("Not enough memory to process %s/.perfconfig, " + "ignoring it.", home); + goto out; + } + + if (stat(user_config, &st) < 0) + goto out_free; + + if (st.st_uid && (st.st_uid != geteuid())) { + warning("File %s not owned by current user or root, " + "ignoring it.", user_config); + goto out_free; + } + + if (!st.st_size) + goto out_free; + + ret += perf_config_from_file(collect_config, user_config, set); + found += 1; +out_free: + free(user_config); + } +out: + if (found == 0) + return -1; + return ret; +} + struct perf_config_set *perf_config_set__new(void) { struct perf_config_set *set = zalloc(sizeof(*set)); if (set) { INIT_LIST_HEAD(&set->sections); - perf_config(collect_config, set); + if (perf_config_set__init(set) < 0) + return NULL; } return set; -- 2.5.0
[PATCH v2 5/5] perf config: Reset config set at only 'config' sub-command
When first calling perf_config(), config set is initialized but 'config' sub-command need to reset config set because of '--user' or '--system' options. The options of 'config' sub-command is to select a particular config file location so the config set should be reinitialized collecting configs from selected exclusive config file. Cc: Namhyung Kim Cc: Jiri Olsa Cc: Masami Hiramatsu Cc: Alexander Shishkin Signed-off-by: Taeung Song --- tools/perf/builtin-config.c | 12 +--- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/tools/perf/builtin-config.c b/tools/perf/builtin-config.c index 4a61411..dc5b52f 100644 --- a/tools/perf/builtin-config.c +++ b/tools/perf/builtin-config.c @@ -47,7 +47,6 @@ static int show_config(const char *key, const char *value, int cmd_config(int argc, const char **argv, const char *prefix __maybe_unused) { int ret = 0; - struct perf_config_set *set; char *user_config = mkpath("%s/.perfconfig", getenv("HOME")); argc = parse_options(argc, argv, config_options, config_usage, @@ -65,11 +64,11 @@ int cmd_config(int argc, const char **argv, const char *prefix __maybe_unused) else if (use_user_config) config_exclusive_filename = user_config; - set = perf_config_set__new(); - if (!set) { - ret = -1; - goto out_err; - } + /* +* Reset config set at only 'config' sub-command +* because of options config file location. +*/ + perf_config_set__delete(); switch (actions) { case ACTION_LIST: @@ -92,6 +91,5 @@ int cmd_config(int argc, const char **argv, const char *prefix __maybe_unused) } perf_config_set__delete(); -out_err: return ret; } -- 2.5.0
[RFC][PATCH v2 0/5] perf config: Reimplement perf_config() using perf_config_set__inter()
Everytime perf_config() is called, perf_config() always read config files. (i.e. user config '~/.perfconfig' and system config '$(sysconfdir)/perfconfig') But we need to use 'struct perf_config_set config_set' variable that already contains all config key-value pairs to avoid this repetitive work in perf_config(). In other words, if new perf_config() is called, only first time 'config_set' is initialized collecting all configs from config files and it work with perf_config_set__iter(). If we do, what old perf_config() handle is the same as new perf_config() work without the repetitive work that read config files. IMHO, I think this patchset is needed because not only the repetitive work should be avoided but also in near future, it would be smooth to manage perf configs. If you give me any feedback, I'd apprecicated it. :) Thanks, Taeung v2: - split a patch into several patches - reimplement show_config() using new perf_config() - modify perf_config_set__delete using global variable 'config_set' - reset config set when only 'config' sub-commaned work because of options for config file location Taeung Song (5): perf config: Use new perf_config_set__init() to initialize config set perf config: Reimplement perf_config() using perf_config_set__iter() perf config: Modify perf_config_set__delete() using global variable 'config_set' perf config: Reimplement show_config() using perf_config() perf config: Reset config set at only 'config' sub-command tools/perf/builtin-config.c | 43 tools/perf/util/config.c| 156 +--- tools/perf/util/config.h| 2 +- 3 files changed, 117 insertions(+), 84 deletions(-) -- 2.5.0
[PATCH v2 2/5] perf config: Reimplement perf_config() using perf_config_set__iter()
Everytime perf_config() is called, perf_config() always read config files. (i.e. user config '~/.perfconfig' and system config '$(sysconfdir)/perfconfig') But we need to use config set that already contains all config key-value pairs to avoid this repetitive work reading the config files in perf_config(). In other words, if new perf_config() is called, only first time 'config_set' is initialized collecting all configs from the config files and it work with perf_config_set__iter(). If we do, what old perf_config() handle is the same as new perf_config() work without the repetitive work that read the config files. Cc: Namhyung Kim Cc: Jiri Olsa Cc: Wang Nan Cc: Peter Zijlstra Cc: Ingo Molnar Cc: Masami Hiramatsu Cc: Alexander Shishkin Signed-off-by: Taeung Song --- tools/perf/util/config.c | 98 1 file changed, 50 insertions(+), 48 deletions(-) diff --git a/tools/perf/util/config.c b/tools/perf/util/config.c index 5d01899..487d390 100644 --- a/tools/perf/util/config.c +++ b/tools/perf/util/config.c @@ -28,6 +28,7 @@ static int config_linenr; static int config_file_eof; const char *config_exclusive_filename; +struct perf_config_set *config_set; static int get_next_char(void) { @@ -477,54 +478,6 @@ static int perf_config_global(void) return !perf_env_bool("PERF_CONFIG_NOGLOBAL", 0); } -int perf_config(config_fn_t fn, void *data) -{ - int ret = 0, found = 0; - const char *home = NULL; - - /* Setting $PERF_CONFIG makes perf read _only_ the given config file. */ - if (config_exclusive_filename) - return perf_config_from_file(fn, config_exclusive_filename, data); - if (perf_config_system() && !access(perf_etc_perfconfig(), R_OK)) { - ret += perf_config_from_file(fn, perf_etc_perfconfig(), - data); - found += 1; - } - - home = getenv("HOME"); - if (perf_config_global() && home) { - char *user_config = strdup(mkpath("%s/.perfconfig", home)); - struct stat st; - - if (user_config == NULL) { - warning("Not enough memory to process %s/.perfconfig, " - "ignoring it.", home); - goto out; - } - - if (stat(user_config, &st) < 0) - goto out_free; - - if (st.st_uid && (st.st_uid != geteuid())) { - warning("File %s not owned by current user or root, " - "ignoring it.", user_config); - goto out_free; - } - - if (!st.st_size) - goto out_free; - - ret += perf_config_from_file(fn, user_config, data); - found += 1; -out_free: - free(user_config); - } -out: - if (found == 0) - return -1; - return ret; -} - static struct perf_config_section *find_section(struct list_head *sections, const char *section_name) { @@ -705,6 +658,55 @@ struct perf_config_set *perf_config_set__new(void) return set; } +static int perf_config_set__check(void) +{ + if (config_set != NULL) + return 0; + + config_set = perf_config_set__new(); + if (!config_set) + return -1; + + return 0; +} + +static int perf_config_set__iter(struct perf_config_set *set, config_fn_t fn, void *data) +{ + struct perf_config_section *section; + struct perf_config_item *item; + struct list_head *sections; + char key[BUFSIZ]; + + if (set == NULL) + return -1; + + sections = &set->sections; + if (list_empty(sections)) + return -1; + + list_for_each_entry(section, sections, node) { + list_for_each_entry(item, §ion->items, node) { + char *value = item->value; + + if (value) { + scnprintf(key, sizeof(key), "%s.%s", + section->name, item->name); + if (fn(key, value, data) < 0) + return -1; + } + } + } + + return 0; +} + +int perf_config(config_fn_t fn, void *data) +{ + if (perf_config_set__check() < 0) + return -1; + return perf_config_set__iter(config_set, fn, data); +} + static void perf_config_item__delete(struct perf_config_item *item) { zfree(&item->name); -- 2.5.0
[PATCH v2 3/5] perf config: Modify perf_config_set__delete() using global variable 'config_set'
This function deleted allocated config set but the global variable 'config_set' is used all around so this directly remove 'config_set' instead of using local variable 'set'. Cc: Namhyung Kim Cc: Jiri Olsa Cc: Masami Hiramatsu Cc: Alexander Shishkin Signed-off-by: Taeung Song --- tools/perf/builtin-config.c | 2 +- tools/perf/util/config.c| 8 tools/perf/util/config.h| 2 +- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/tools/perf/builtin-config.c b/tools/perf/builtin-config.c index fe1b77f..8eef3fb 100644 --- a/tools/perf/builtin-config.c +++ b/tools/perf/builtin-config.c @@ -106,7 +106,7 @@ int cmd_config(int argc, const char **argv, const char *prefix __maybe_unused) usage_with_options(config_usage, config_options); } - perf_config_set__delete(set); + perf_config_set__delete(); out_err: return ret; } diff --git a/tools/perf/util/config.c b/tools/perf/util/config.c index 487d390..abfe1b2 100644 --- a/tools/perf/util/config.c +++ b/tools/perf/util/config.c @@ -594,7 +594,7 @@ static int collect_config(const char *var, const char *value, out_free: free(key); - perf_config_set__delete(set); + perf_config_set__delete(); return -1; } @@ -741,10 +741,10 @@ static void perf_config_set__purge(struct perf_config_set *set) } } -void perf_config_set__delete(struct perf_config_set *set) +void perf_config_set__delete(void) { - perf_config_set__purge(set); - free(set); + perf_config_set__purge(config_set); + zfree(&config_set); } /* diff --git a/tools/perf/util/config.h b/tools/perf/util/config.h index 22ec626..be4e603 100644 --- a/tools/perf/util/config.h +++ b/tools/perf/util/config.h @@ -21,6 +21,6 @@ struct perf_config_set { }; struct perf_config_set *perf_config_set__new(void); -void perf_config_set__delete(struct perf_config_set *set); +void perf_config_set__delete(void); #endif /* __PERF_CONFIG_H */ -- 2.5.0
[PATCH v3 6/7] perf config: Add 'annotate' section default configs arrrays
Actual variable for configs of 'annotate' section is like below. (at ui/browsers/annoate.c) static struct annotate_browser_opt { bool hide_src_code, use_offset, jump_arrows, show_linenr, show_nr_jumps, show_total_period; } annotate_browser__opts = { .use_offset = true, .jump_arrows = true, }; But I suggest using 'annoate' default config array that have all default config key-value pairs for 'annotate' section In near future, this arrays will be used on ui/browsers/annoate.c because of setting default actual variable for 'annotate' config. Cc: Namhyung Kim Cc: Jiri Olsa Cc: Masami Hiramatsu Cc: Alexander Shishkin Signed-off-by: Taeung Song --- tools/perf/util/config.c | 11 +++ tools/perf/util/config.h | 11 +++ 2 files changed, 22 insertions(+) diff --git a/tools/perf/util/config.c b/tools/perf/util/config.c index e38d187..9a06cb0 100644 --- a/tools/perf/util/config.c +++ b/tools/perf/util/config.c @@ -31,6 +31,7 @@ const char *config_exclusive_filename; const struct perf_config_section default_sections[] = { { .name = "colors" }, + { .name = "annotate" }, }; const struct default_config_item colors_config_items[] = { @@ -44,6 +45,16 @@ const struct default_config_item colors_config_items[] = { CONF_END() }; +const struct default_config_item annotate_config_items[] = { + CONF_BOOL_VAR("hide_src_code", false), + CONF_BOOL_VAR("use_offset", true), + CONF_BOOL_VAR("jump_arrows", true), + CONF_BOOL_VAR("show_nr_jumps", false), + CONF_BOOL_VAR("show_linenr", false), + CONF_BOOL_VAR("show_total_period", false), + CONF_END() +}; + static int get_next_char(void) { int c; diff --git a/tools/perf/util/config.h b/tools/perf/util/config.h index f2220a8..696e5bc 100644 --- a/tools/perf/util/config.h +++ b/tools/perf/util/config.h @@ -46,6 +46,7 @@ struct perf_config_set { enum config_section_idx { CONFIG_COLORS, + CONFIG_ANNOTATE, }; enum colors_config_items_idx { @@ -58,6 +59,15 @@ enum colors_config_items_idx { CONFIG_COLORS_ROOT, }; +enum annotate_config_items_idx { + CONFIG_ANNOTATE_HIDE_SRC_CODE, + CONFIG_ANNOTATE_USE_OFFSET, + CONFIG_ANNOTATE_JUMP_ARROWS, + CONFIG_ANNOTATE_SHOW_NR_JUMPS, + CONFIG_ANNOTATE_SHOW_LINENR, + CONFIG_ANNOTATE_SHOW_TOTAL_PERIOD, +}; + #define CONF_VAR(_name, _field, _val, _type) \ { .name = _name, .value._field = _val, .type = _type } @@ -79,6 +89,7 @@ enum colors_config_items_idx { { .name = NULL } extern const struct default_config_item colors_config_items[]; +extern const struct default_config_item annotate_config_items[]; struct perf_config_set *perf_config_set__new(void); void perf_config_set__delete(struct perf_config_set *set); -- 2.5.0
[PATCH v3 1/7] perf config: Introduce default_config_item for default config key-value pairs
When initializing default perf config values, we currently use values of actual type(int, bool, char *, etc.). For example, If there isn't user config value at ~/.perfconfig for 'annotate.use_offset' config variable, default value for it is 'true' bool type value in perf like below. At ui/browsers/annoate.c static struct annotate_browser_opt { bool hide_src_code, use_offset, jump_arrows, show_linenr, show_nr_jumps, show_total_period; } annotate_browser__opts = { .use_offset = true, .jump_arrows = true, }; By the way, I suggest using 'struct default_config_item' that have default config key-value pairs and then initializing default config values with them, in near future. Because if we do, we can manage default perf config values at one spot (like util/config.c) with default config arrays and It can be easy and simple to modify default config values or add new configs. Cc: Namhyung Kim Cc: Jiri Olsa Cc: Wang Nan Cc: Peter Zijlstra Cc: Ingo Molnar Cc: Masami Hiramatsu Cc: Alexander Shishkin Signed-off-by: Taeung Song --- tools/perf/util/config.h | 44 1 file changed, 44 insertions(+) diff --git a/tools/perf/util/config.h b/tools/perf/util/config.h index 22ec626..5a11ca6 100644 --- a/tools/perf/util/config.h +++ b/tools/perf/util/config.h @@ -4,6 +4,30 @@ #include #include +enum perf_config_type { + CONFIG_TYPE_BOOL, + CONFIG_TYPE_INT, + CONFIG_TYPE_LONG, + CONFIG_TYPE_U64, + CONFIG_TYPE_FLOAT, + CONFIG_TYPE_DOUBLE, + CONFIG_TYPE_STRING +}; + +struct default_config_item { + const char *name; + union { + bool b; + int i; + u32 l; + u64 ll; + float f; + double d; + const char *s; + } value; + enum perf_config_type type; +}; + struct perf_config_item { char *name; char *value; @@ -20,6 +44,26 @@ struct perf_config_set { struct list_head sections; }; +#define CONF_VAR(_name, _field, _val, _type) \ + { .name = _name, .value._field = _val, .type = _type } + +#define CONF_BOOL_VAR(_name, _val) \ + CONF_VAR(_name, b, _val, CONFIG_TYPE_BOOL) +#define CONF_INT_VAR(_name, _val) \ + CONF_VAR(_name, i, _val, CONFIG_TYPE_INT) +#define CONF_LONG_VAR(_name, _val) \ + CONF_VAR(_name, l, _val, CONFIG_TYPE_LONG) +#define CONF_U64_VAR(_name, _val) \ + CONF_VAR(_name, ll, _val, CONFIG_TYPE_U64) +#define CONF_FLOAT_VAR(_name, _val)\ + CONF_VAR(_name, f, _val, CONFIG_TYPE_FLOAT) +#define CONF_DOUBLE_VAR(_name, _val) \ + CONF_VAR(_name, d, _val, CONFIG_TYPE_DOUBLE) +#define CONF_STR_VAR(_name, _val) \ + CONF_VAR(_name, s, _val, CONFIG_TYPE_STRING) +#define CONF_END() \ + { .name = NULL } + struct perf_config_set *perf_config_set__new(void); void perf_config_set__delete(struct perf_config_set *set); -- 2.5.0
[PATCH v3 5/7] perf config: Introduce perf_default_config_init()
default_*_config_init() initialize actual variables with each default config value. (e.g. default_colors_config_init() for 'colors' section) But I suggest using perf_default_config_init() that call all default_*_config_init() and this function would be called at the very beginning of main() on perf.c. In order to set all default config values before running a particular sub-command. And if we do, we can manage all default_*_config_init() functions in perf_default_config_init(). Cc: Namhyung Kim Cc: Jiri Olsa Cc: Wang Nan Cc: Peter Zijlstra Cc: Ingo Molnar Cc: Masami Hiramatsu Cc: Alexander Shishkin Signed-off-by: Taeung Song --- tools/perf/perf.c | 6 ++ tools/perf/ui/browser.c | 1 - tools/perf/util/cache.h | 1 + 3 files changed, 7 insertions(+), 1 deletion(-) diff --git a/tools/perf/perf.c b/tools/perf/perf.c index 15982ce..e140b551 100644 --- a/tools/perf/perf.c +++ b/tools/perf/perf.c @@ -77,6 +77,11 @@ struct pager_config { int val; }; +static void perf_default_config_init(void) +{ + default_colors_config_init(); +} + static int pager_command_config(const char *var, const char *value, void *data) { struct pager_config *c = data; @@ -558,6 +563,7 @@ int main(int argc, const char **argv) srandom(time(NULL)); + perf_default_config_init(); perf_config(perf_default_config, NULL); set_buildid_dir(NULL); diff --git a/tools/perf/ui/browser.c b/tools/perf/ui/browser.c index 00a91e0..c07ca70 100644 --- a/tools/perf/ui/browser.c +++ b/tools/perf/ui/browser.c @@ -738,7 +738,6 @@ void ui_browser__init(void) { int i = 0; - default_colors_config_init(); perf_config(ui_browser__color_config, NULL); while (ui_browser__colorsets[i].name) { diff --git a/tools/perf/util/cache.h b/tools/perf/util/cache.h index 54bbd55..19e1e00 100644 --- a/tools/perf/util/cache.h +++ b/tools/perf/util/cache.h @@ -7,6 +7,7 @@ #include #include "../perf.h" #include "../ui/ui.h" +#include "../ui/browser.h" #include "config.h" #include -- 2.5.0
[PATCH v3 2/7] perf config: Add 'colors' section default configs arrrays
Actual variable for configs of 'colors' section is like below. (at ui/browser.c) static struct ui_browser_colorset { const char *name, *fg, *bg; int colorset; } ui_browser__colorsets[] = { { .colorset = HE_COLORSET_TOP, .name = "top", .fg = "red", .bg = "default", }, ... But I suggest using 'colors' default config array that have all default config key-value pairs for 'colors' section In near future, this array will be used on ui/browser.c because of setting default actual variable for 'colors' config. Cc: Namhyung Kim Cc: Jiri Olsa Cc: Masami Hiramatsu Cc: Alexander Shishkin Signed-off-by: Taeung Song --- tools/perf/util/cache.h | 1 + tools/perf/util/config.c | 17 - tools/perf/util/config.h | 18 +- 3 files changed, 34 insertions(+), 2 deletions(-) diff --git a/tools/perf/util/cache.h b/tools/perf/util/cache.h index 0d814bb..54bbd55 100644 --- a/tools/perf/util/cache.h +++ b/tools/perf/util/cache.h @@ -7,6 +7,7 @@ #include #include "../perf.h" #include "../ui/ui.h" +#include "config.h" #include diff --git a/tools/perf/util/config.c b/tools/perf/util/config.c index dad7d82..e38d187 100644 --- a/tools/perf/util/config.c +++ b/tools/perf/util/config.c @@ -29,6 +29,21 @@ static int config_file_eof; const char *config_exclusive_filename; +const struct perf_config_section default_sections[] = { + { .name = "colors" }, +}; + +const struct default_config_item colors_config_items[] = { + CONF_STR_VAR("top", "red, default"), + CONF_STR_VAR("medium", "green, default"), + CONF_STR_VAR("normal", "default, default"), + CONF_STR_VAR("selected", "black, yellow"), + CONF_STR_VAR("jump_arrows", "blue, default"), + CONF_STR_VAR("addr", "magenta, default"), + CONF_STR_VAR("root", "white, blue"), + CONF_END() +}; + static int get_next_char(void) { int c; @@ -677,7 +692,7 @@ static void perf_config_section__purge(struct perf_config_section *section) static void perf_config_section__delete(struct perf_config_section *section) { perf_config_section__purge(section); - zfree(§ion->name); + zfree((char **)§ion->name); free(section); } diff --git a/tools/perf/util/config.h b/tools/perf/util/config.h index 5a11ca6..f2220a8 100644 --- a/tools/perf/util/config.h +++ b/tools/perf/util/config.h @@ -35,7 +35,7 @@ struct perf_config_item { }; struct perf_config_section { - char *name; + const char *name; struct list_head items; struct list_head node; }; @@ -44,6 +44,20 @@ struct perf_config_set { struct list_head sections; }; +enum config_section_idx { + CONFIG_COLORS, +}; + +enum colors_config_items_idx { + CONFIG_COLORS_TOP, + CONFIG_COLORS_MEDIUM, + CONFIG_COLORS_NORMAL, + CONFIG_COLORS_SELECTED, + CONFIG_COLORS_JUMP_ARROWS, + CONFIG_COLORS_ADDR, + CONFIG_COLORS_ROOT, +}; + #define CONF_VAR(_name, _field, _val, _type) \ { .name = _name, .value._field = _val, .type = _type } @@ -64,6 +78,8 @@ struct perf_config_set { #define CONF_END() \ { .name = NULL } +extern const struct default_config_item colors_config_items[]; + struct perf_config_set *perf_config_set__new(void); void perf_config_set__delete(struct perf_config_set *set); -- 2.5.0
[PATCH v3 7/7] perf config: Initialize annotate_browser__opts with default config items
Set default config values for 'annotate' section with 'annotate_config_items[]' instead of actual bool type values. (e.g. using annotate_config_items[CONFIG_ANNOTATE_USE_OFFSET].value instead of 'true' bool type value for 'annotate.use_offset'.) Cc: Namhyung Kim Cc: Jiri Olsa Cc: Masami Hiramatsu Cc: Alexander Shishkin Signed-off-by: Taeung Song --- tools/perf/perf.c | 1 + tools/perf/ui/browser.h | 1 + tools/perf/ui/browsers/annotate.c | 15 +++ tools/perf/util/config.h | 6 ++ 4 files changed, 19 insertions(+), 4 deletions(-) diff --git a/tools/perf/perf.c b/tools/perf/perf.c index e140b551..d5541fa 100644 --- a/tools/perf/perf.c +++ b/tools/perf/perf.c @@ -80,6 +80,7 @@ struct pager_config { static void perf_default_config_init(void) { default_colors_config_init(); + default_annotate_config_init(); } static int pager_command_config(const char *var, const char *value, void *data) diff --git a/tools/perf/ui/browser.h b/tools/perf/ui/browser.h index 46921e5..b7dc178 100644 --- a/tools/perf/ui/browser.h +++ b/tools/perf/ui/browser.h @@ -77,4 +77,5 @@ void ui_browser__init(void); void annotate_browser__init(void); void default_colors_config_init(void); +void default_annotate_config_init(void); #endif /* _PERF_UI_BROWSER_H_ */ diff --git a/tools/perf/ui/browsers/annotate.c b/tools/perf/ui/browsers/annotate.c index 4fc208e..35c2547 100644 --- a/tools/perf/ui/browsers/annotate.c +++ b/tools/perf/ui/browsers/annotate.c @@ -37,10 +37,7 @@ static struct annotate_browser_opt { show_linenr, show_nr_jumps, show_total_period; -} annotate_browser__opts = { - .use_offset = true, - .jump_arrows= true, -}; +} annotate_browser__opts; struct annotate_browser { struct ui_browser b; @@ -1158,6 +1155,16 @@ static int annotate__config(const char *var, const char *value, return 0; } +void default_annotate_config_init(void) +{ + annotate_browser__opts.hide_src_code = CONF_ANNOTATE_DEFAULT_VAL(HIDE_SRC_CODE, b); + annotate_browser__opts.use_offset = CONF_ANNOTATE_DEFAULT_VAL(USE_OFFSET, b); + annotate_browser__opts.jump_arrows = CONF_ANNOTATE_DEFAULT_VAL(JUMP_ARROWS, b); + annotate_browser__opts.show_linenr = CONF_ANNOTATE_DEFAULT_VAL(SHOW_LINENR, b); + annotate_browser__opts.show_nr_jumps = CONF_ANNOTATE_DEFAULT_VAL(SHOW_NR_JUMPS, b); + annotate_browser__opts.show_total_period = CONF_ANNOTATE_DEFAULT_VAL(SHOW_TOTAL_PERIOD, b); +} + void annotate_browser__init(void) { perf_config(annotate__config, NULL); diff --git a/tools/perf/util/config.h b/tools/perf/util/config.h index 696e5bc..7b70971 100644 --- a/tools/perf/util/config.h +++ b/tools/perf/util/config.h @@ -88,6 +88,12 @@ enum annotate_config_items_idx { #define CONF_END() \ { .name = NULL } +#define CONF_DEFAULT_VAL(section, name, field) \ + section##_config_items[CONFIG_##name].value.field + +#define CONF_ANNOTATE_DEFAULT_VAL(name, field) \ + CONF_DEFAULT_VAL(annotate, ANNOTATE_##name, field) + extern const struct default_config_item colors_config_items[]; extern const struct default_config_item annotate_config_items[]; -- 2.5.0
[PATCH v3 3/7] perf config: Use combined {fore,back}ground colors value instead of each two color
To manage all default config values at one spot (at util/config.c), it would be better that actual variables for each 'colors' config also have only one value like 'default_config_item' type. If we do, it smoothly work to initialize 'colors' default config values by 'colors_config_items' array that have default values at util/config.c because both actual variable and config item of 'colors_config_items' are equal in the number of values (as just one). Cc: Namhyung Kim Cc: Jiri Olsa Cc: Masami Hiramatsu Cc: Alexander Shishkin Signed-off-by: Taeung Song --- tools/perf/ui/browser.c | 81 - 1 file changed, 39 insertions(+), 42 deletions(-) diff --git a/tools/perf/ui/browser.c b/tools/perf/ui/browser.c index af68a9d..8e06b2e 100644 --- a/tools/perf/ui/browser.c +++ b/tools/perf/ui/browser.c @@ -503,61 +503,53 @@ unsigned int ui_browser__list_head_refresh(struct ui_browser *browser) } static struct ui_browser_colorset { - const char *name, *fg, *bg; + const char *name, *fb_ground; int colorset; } ui_browser__colorsets[] = { { - .colorset = HE_COLORSET_TOP, - .name = "top", - .fg = "red", - .bg = "default", + .colorset = HE_COLORSET_TOP, + .name = "top", + .fb_ground = "red, default", }, { - .colorset = HE_COLORSET_MEDIUM, - .name = "medium", - .fg = "green", - .bg = "default", + .colorset = HE_COLORSET_MEDIUM, + .name = "medium", + .fb_ground = "green, default", }, { - .colorset = HE_COLORSET_NORMAL, - .name = "normal", - .fg = "default", - .bg = "default", + .colorset = HE_COLORSET_NORMAL, + .name = "normal", + .fb_ground = "default, default", }, { - .colorset = HE_COLORSET_SELECTED, - .name = "selected", - .fg = "black", - .bg = "yellow", + .colorset = HE_COLORSET_SELECTED, + .name = "selected", + .fb_ground = "black, yellow", }, { - .colorset = HE_COLORSET_JUMP_ARROWS, - .name = "jump_arrows", - .fg = "blue", - .bg = "default", + .colorset = HE_COLORSET_JUMP_ARROWS, + .name = "jump_arrows", + .fb_ground = "blue, default", }, { - .colorset = HE_COLORSET_ADDR, - .name = "addr", - .fg = "magenta", - .bg = "default", + .colorset = HE_COLORSET_ADDR, + .name = "addr", + .fb_ground = "magenta, default", }, { - .colorset = HE_COLORSET_ROOT, - .name = "root", - .fg = "white", - .bg = "blue", + .colorset = HE_COLORSET_ROOT, + .name = "root", + .fb_ground = "white, blue", }, { .name = NULL, } }; - static int ui_browser__color_config(const char *var, const char *value, void *data __maybe_unused) { - char *fg = NULL, *bg; + char *fb_ground; int i; /* same dir for all commands */ @@ -570,22 +562,18 @@ static int ui_browser__color_config(const char *var, const char *value, if (strcmp(ui_browser__colorsets[i].name, name) != 0) continue; - fg = strdup(value); - if (fg == NULL) - break; + if (strstr(value, ",") == NULL) + return -1; - bg = strchr(fg, ','); - if (bg == NULL) + fb_ground = strdup(value); + if (fb_ground == NULL) break; + ui_browser__colorsets[i].fb_ground = fb_ground; - *bg = '\0'; - while (isspace(*++bg)); - ui_browser__colorsets[i].bg = bg; - ui_browser__colorsets[i].fg = fg; return 0; } - free(fg); + free(fb_ground); return -