Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932297AbcCKONF (ORCPT ); Fri, 11 Mar 2016 09:13:05 -0500 Received: from mail-pa0-f47.google.com ([209.85.220.47]:36283 "EHLO mail-pa0-f47.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932067AbcCKOM4 (ORCPT ); Fri, 11 Mar 2016 09:12:56 -0500 Date: Fri, 11 Mar 2016 23:11:31 +0900 From: Namhyung Kim To: Taeung Song Cc: Arnaldo Carvalho de Melo , linux-kernel@vger.kernel.org, Jiri Olsa , Ingo Molnar , Peter Zijlstra Subject: Re: [RFC][PATCH] perf config: Introduce perf_config_set class Message-ID: <20160311141131.GB25533@danjae.kornet> References: <1457618667-19915-1-git-send-email-treeze.taeung@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <1457618667-19915-1-git-send-email-treeze.taeung@gmail.com> User-Agent: Mutt/1.5.24 (2015-08-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 20586 Lines: 662 Hi Taeung, On Thu, Mar 10, 2016 at 11:04:27PM +0900, Taeung Song wrote: > In order to variously handle config key-value pairs, > prepare all default configs and collect > config key-value pairs from user and > system config files (i.e user wide ~/.perfconfig > and system wide $(sysconfdir)/perfconfig) > > The various purposes are like > showing current configs, > in the near future, showing all configs with > default value, getting current configs or > setting configs that user type, etc. > > There aren't additional functionalities by appearances > excluding handling the error by file status. > (e.g checking permission, whether it is empty or etc) > and when the two config files have each other's > values of same key i.e. > > # cat ~/.perfconfig > [top] > children = false > > when $(sysconfdir) is /usr/local/etc > # cat /usr/local/etc/perfconfig > [top] > children = true > > Before: > # perf config --user --list > top.children=false > > # perf config --system --list > top.children=true > > # perf config --list > top.children=true > top.children=false > > After: > # perf config --user --list > top.children=false > > # perf config --system --list > top.children=true > > # perf config --list > top.children=false > > But this patch is designed with considering > not only current functionality but also upcoming features. > > And send [TEST REPORT] email follwing this patch > to save time for review. (kinds of test: BASIC, EXCEP HANDLING) > > Cc: Namhyung Kim > Cc: Jiri Olsa > Signed-off-by: Taeung Song > --- > tools/perf/builtin-config.c | 61 +++++++--- > tools/perf/util/cache.h | 2 - > tools/perf/util/config.c | 268 +++++++++++++++++++++++++++++++++++++++++--- > tools/perf/util/config.h | 138 +++++++++++++++++++++++ > 4 files changed, 433 insertions(+), 36 deletions(-) > create mode 100644 tools/perf/util/config.h This is a pretty big change. I recommend you to split the patch into pieces. I guess it can consist of 3 patches at least. 1. implement basic perf_config_item/set operartion 2. add actual config items using the above ops 3. handle system/user (or both) case Also I think it'd be better just keeping a single config value instead of 3 kinds. Maybe you can read system-wide config first and overwrite them with user config (for the 'both' case). Thanks, Namhyung > > diff --git a/tools/perf/builtin-config.c b/tools/perf/builtin-config.c > index c42448e..d79f4d9 100644 > --- a/tools/perf/builtin-config.c > +++ b/tools/perf/builtin-config.c > @@ -12,6 +12,7 @@ > #include > #include "util/util.h" > #include "util/debug.h" > +#include "util/config.h" > > static bool use_system_config, use_user_config; > > @@ -32,21 +33,40 @@ static struct option config_options[] = { > OPT_END() > }; > > -static int show_config(const char *key, const char *value, > - void *cb __maybe_unused) > +static int show_config(struct perf_config_set *perf_configs) > { > - if (value) > - printf("%s=%s\n", key, value); > - else > - printf("%s\n", key); > + struct perf_config_item *config; > + enum perf_config_kind pos = perf_configs->pos; > + bool *file_usable = perf_configs->file_usable; > + struct list_head *config_list = &perf_configs->config_list; > + > + if (pos == BOTH) { > + if (!file_usable[USER] && !file_usable[SYSTEM]) > + return -1; > + } else if (!file_usable[pos]) > + return -1; > + > + list_for_each_entry(config, config_list, list) { > + char *value = config->value[pos]; > + > + if (value) > + printf("%s.%s=%s\n", config->section, > + config->name, value); > + } > > return 0; > } > > int cmd_config(int argc, const char **argv, const char *prefix __maybe_unused) > { > + /* > + * The perf_configs that contains not only default configs > + * but also key-value pairs from config files. > + * (i.e user wide ~/.perfconfig and system wide $(sysconfdir)/perfconfig) > + * This is designed to be used by several functions that handle config set. > + */ > + struct perf_config_set *perf_configs; > int ret = 0; > - char *user_config = mkpath("%s/.perfconfig", getenv("HOME")); > > argc = parse_options(argc, argv, config_options, config_usage, > PARSE_OPT_STOP_AT_NON_OPTION); > @@ -58,10 +78,17 @@ int cmd_config(int argc, const char **argv, const char *prefix __maybe_unused) > return -1; > } > > - if (use_system_config) > - config_exclusive_filename = perf_etc_perfconfig(); > - else if (use_user_config) > - config_exclusive_filename = user_config; > + perf_configs = perf_config_set__new(); > + if (!perf_configs) > + return -1; > + > + if (use_user_config) > + perf_configs->pos = USER; > + else if (use_system_config) > + perf_configs->pos = SYSTEM; > + else > + perf_configs->pos = BOTH; > + > > switch (actions) { > case ACTION_LIST: > @@ -69,19 +96,17 @@ 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 = perf_config(show_config, NULL); > - if (ret < 0) { > - const char * config_filename = config_exclusive_filename; > - if (!config_exclusive_filename) > - config_filename = user_config; > + ret = show_config(perf_configs); > + if (ret < 0) > pr_err("Nothing configured, " > - "please check your %s \n", config_filename); > - } > + "please check your %s \n", > + perf_configs->file_path[perf_configs->pos]); > } > break; > default: > usage_with_options(config_usage, config_options); > } > > + perf_config_set__delete(perf_configs); > return ret; > } > diff --git a/tools/perf/util/cache.h b/tools/perf/util/cache.h > index 3ca453f..ba657ad 100644 > --- a/tools/perf/util/cache.h > +++ b/tools/perf/util/cache.h > @@ -23,8 +23,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 *); > extern int perf_default_config(const char *, const char *, void *); > extern int perf_config(config_fn_t fn, void *); > diff --git a/tools/perf/util/config.c b/tools/perf/util/config.c > index 4e72763..f827932 100644 > --- a/tools/perf/util/config.c > +++ b/tools/perf/util/config.c > @@ -13,6 +13,7 @@ > #include > #include "util/hist.h" /* perf_hist_config */ > #include "util/llvm-utils.h" /* perf_llvm_config */ > +#include "config.h" > > #define MAXNAME (256) > > @@ -26,7 +27,53 @@ static const char *config_file_name; > static int config_linenr; > static int config_file_eof; > > -const char *config_exclusive_filename; > +static const char *config_exclusive_filename; > + > +struct perf_config_item default_configs[] = { > + CONF_STR_VAR(COLORS_TOP, "colors", "top", "red, default"), > + CONF_STR_VAR(COLORS_MEDIUM, "colors", "medium", "green, default"), > + CONF_STR_VAR(COLORS_NORMAL, "colors", "normal", "lightgray, default"), > + CONF_STR_VAR(COLORS_SELECTED, "colors", "selected", "white, lightgray"), > + CONF_STR_VAR(COLORS_JUMP_ARROWS, "colors", "jump_arrows", "blue, default"), > + CONF_STR_VAR(COLORS_ADDR, "colors", "addr", "magenta, default"), > + CONF_STR_VAR(COLORS_ROOT, "colors", "root", "white, blue"), > + CONF_BOOL_VAR(TUI_REPORT, "tui", "report", true), > + CONF_BOOL_VAR(TUI_ANNOTATE, "tui", "annotate", true), > + CONF_BOOL_VAR(TUI_TOP, "tui", "top", true), > + CONF_STR_VAR(BUILDID_DIR, "buildid", "dir", "~/.debug"), > + CONF_BOOL_VAR(ANNOTATE_HIDE_SRC_CODE, "annotate", "hide_src_code", false), > + CONF_BOOL_VAR(ANNOTATE_USE_OFFSET, "annotate", "use_offset", true), > + CONF_BOOL_VAR(ANNOTATE_JUMP_ARROWS, "annotate", "jump_arrows", true), > + CONF_BOOL_VAR(ANNOTATE_SHOW_NR_JUMPS, "annotate", "show_nr_jumps", false), > + CONF_BOOL_VAR(ANNOTATE_SHOW_LINENR, "annotate", "show_linenr", false), > + CONF_BOOL_VAR(ANNOTATE_SHOW_TOTAL_PERIOD, "annotate", "show_total_period", false), > + CONF_BOOL_VAR(GTK_ANNOTATE, "gtk", "annotate", false), > + CONF_BOOL_VAR(GTK_REPORT, "gtk", "report", false), > + CONF_BOOL_VAR(GTK_TOP, "gtk", "top", false), > + CONF_BOOL_VAR(PAGER_CMD, "pager", "cmd", true), > + CONF_BOOL_VAR(PAGER_REPORT, "pager", "report", true), > + CONF_BOOL_VAR(PAGER_ANNOTATE, "pager", "annotate", true), > + CONF_BOOL_VAR(PAGER_TOP, "pager", "top", true), > + CONF_BOOL_VAR(PAGER_DIFF, "pager", "diff", true), > + CONF_STR_VAR(HELP_FORMAT, "help", "format", "man"), > + CONF_INT_VAR(HELP_AUTOCORRECT, "help", "autocorrect", 0), > + CONF_STR_VAR(HIST_PERCENTAGE, "hist", "percentage", "absolute"), > + CONF_BOOL_VAR(UI_SHOW_HEADERS, "ui", "show-headers", true), > + CONF_STR_VAR(CALL_GRAPH_RECORD_MODE, "call-graph", "record-mode", "fp"), > + CONF_LONG_VAR(CALL_GRAPH_DUMP_SIZE, "call-graph", "dump-size", 8192), > + CONF_STR_VAR(CALL_GRAPH_PRINT_TYPE, "call-graph", "print-type", "graph"), > + CONF_STR_VAR(CALL_GRAPH_ORDER, "call-graph", "order", "callee"), > + CONF_STR_VAR(CALL_GRAPH_SORT_KEY, "call-graph", "sort-key", "function"), > + CONF_DOUBLE_VAR(CALL_GRAPH_THRESHOLD, "call-graph", "threshold", 0.5), > + CONF_LONG_VAR(CALL_GRAPH_PRINT_LIMIT, "call-graph", "print-limit", 0), > + CONF_BOOL_VAR(REPORT_GROUP, "report", "group", true), > + CONF_BOOL_VAR(REPORT_CHILDREN, "report", "children", true), > + CONF_FLOAT_VAR(REPORT_PERCENT_LIMIT, "report", "percent-limit", 0), > + CONF_U64_VAR(REPORT_QUEUE_SIZE, "report", "queue-size", 0), > + CONF_BOOL_VAR(TOP_CHILDREN, "top", "children", true), > + CONF_STR_VAR(MAN_VIEWER, "man", "viewer", "man"), > + CONF_STR_VAR(KMEM_DEFAULT, "kmem", "default", "slab"), > +}; > > static int get_next_char(void) > { > @@ -458,21 +505,10 @@ static int perf_config_global(void) > return !perf_env_bool("PERF_CONFIG_NOGLOBAL", 0); > } > > -int perf_config(config_fn_t fn, void *data) > +static char *perf_user_perfconfig(void) > { > - int ret = 0, found = 0; > - const char *home = NULL; > + const char *home = getenv("HOME"); > > - /* 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; > @@ -495,17 +531,217 @@ 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; > + return user_config; > + > out_free: > free(user_config); > } > out: > + return NULL; > +} > + > +int perf_config(config_fn_t fn, void *data) > +{ > + int ret = 0, found = 0; > + char *user_config; > + > + /* 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; > + } > + > + user_config = perf_user_perfconfig(); > + if (user_config) { > + ret += perf_config_from_file(fn, user_config, data); > + found += 1; > + free(user_config); > + } > + > if (found == 0) > return -1; > return ret; > } > > +static struct perf_config_item *find_config(struct list_head *config_list, > + const char *section, > + const char *name) > +{ > + struct perf_config_item *config; > + > + list_for_each_entry(config, config_list, list) { > + if (!strcmp(config->section, section) && > + !strcmp(config->name, name)) > + return config; > + } > + > + return NULL; > +} > + > +static struct perf_config_item *add_config(struct list_head *config_list, > + const char *section, > + const char *name) > +{ > + struct perf_config_item *config = zalloc(sizeof(*config)); > + > + if (!config) > + return NULL; > + > + config->is_custom = true; > + config->section = strdup(section); > + if (!section) > + goto out_err; > + > + config->name = strdup(name); > + if (!name) { > + free((char *)config->section); > + goto out_err; > + } > + > + list_add_tail(&config->list, config_list); > + return config; > + > +out_err: > + free(config); > + pr_err("%s: strdup failed\n", __func__); > + return NULL; > +} > + > +static int set_value(struct perf_config_item *config, enum perf_config_kind pos, > + const char *value) > +{ > + char *val = strdup(value); > + > + if (!val) > + return -1; > + > + config->value[BOTH] = val; > + config->value[pos] = val; > + return 0; > +} > + > +static int collect_current_config(const char *var, const char *value, > + void *perf_config_set) > +{ > + int ret = 0; > + char *ptr, *key; > + char *section, *name; > + struct perf_config_item *config; > + struct perf_config_set *perf_configs = perf_config_set; > + struct list_head *config_list = &perf_configs->config_list; > + > + key = ptr = strdup(var); > + if (!key) { > + pr_err("%s: strdup failed\n", __func__); > + return -1; > + } > + > + section = strsep(&ptr, "."); > + name = ptr; > + if (name == NULL || value == NULL) { > + ret = -1; > + goto out_err; > + } > + > + config = find_config(config_list, section, name); > + if (!config) { > + config = add_config(config_list, section, name); > + if (!config) { > + free((char *)config->section); > + free((char *)config->name); > + goto out_err; > + } > + } > + ret = set_value(config, perf_configs->pos, value); > + > +out_err: > + free(key); > + return ret; > +} > + > +static struct perf_config_set *perf_config_set__init(struct perf_config_set *perf_configs) > +{ > + int i; > + struct list_head *head = &perf_configs->config_list; > + > + INIT_LIST_HEAD(&perf_configs->config_list); > + > + for (i = 0; i != CONFIG_END; i++) { > + struct perf_config_item *config = &default_configs[i]; > + > + list_add_tail(&config->list, head); > + } > + return perf_configs; > +} > + > +struct perf_config_set *perf_config_set__new(void) > +{ > + int ret; > + struct perf_config_set *perf_configs = zalloc(sizeof(*perf_configs)); > + char *user_config = perf_user_perfconfig(); > + char *system_config = strdup(perf_etc_perfconfig()); > + > + if (!system_config) > + goto out_err; > + > + perf_config_set__init(perf_configs); > + > + if (!access(system_config, R_OK)) { > + perf_configs->pos = SYSTEM; > + ret = perf_config_from_file(collect_current_config, system_config, > + perf_configs); > + if (ret == 0) > + perf_configs->file_usable[SYSTEM] = true; > + } > + > + if (user_config) { > + perf_configs->pos = USER; > + ret = perf_config_from_file(collect_current_config, user_config, > + perf_configs); > + if (ret == 0) > + perf_configs->file_usable[USER] = true; > + } else { > + user_config = strdup(mkpath("%s/.perfconfig", getenv("HOME"))); > + if (!user_config) > + goto out_err; > + } > + > + /* user config file has order of priority */ > + perf_configs->file_path[BOTH] = user_config; > + perf_configs->file_path[USER] = user_config; > + perf_configs->file_path[SYSTEM] = system_config; > + > + return perf_configs; > + > +out_err: > + pr_err("%s: strdup failed\n", __func__); > + free(perf_configs); > + return NULL; > +} > + > +void perf_config_set__delete(struct perf_config_set *perf_configs) > +{ > + struct perf_config_item *pos, *item; > + > + free(perf_configs->file_path[USER]); > + free(perf_configs->file_path[SYSTEM]); > + > + list_for_each_entry_safe(pos, item, &perf_configs->config_list, list) { > + list_del(&pos->list); > + if (pos->is_custom) { > + free((char *)pos->section); > + free((char *)pos->name); > + } > + free(pos->value[USER]); > + free(pos->value[SYSTEM]); > + } > + > + free(perf_configs); > +} > + > /* > * Call this to report error for your variable that should not > * get a boolean value (i.e. "[my] var" means "true"). > diff --git a/tools/perf/util/config.h b/tools/perf/util/config.h > new file mode 100644 > index 0000000..2523221 > --- /dev/null > +++ b/tools/perf/util/config.h > @@ -0,0 +1,138 @@ > +#ifndef __PERF_CONFIG_H > +#define __PERF_CONFIG_H > + > +#include > +#include > + > +/** > + * enum perf_config_kind - three kinds of config information > + * > + * @USER: user wide ~/.perfconfig > + * @SYSTEM: systeom wide $(sysconfdir)/perfconfig > + * @BOTH: both the config files > + * but user config file has order of priority. > + */ > +#define CONFIG_KIND 3 > +enum perf_config_kind { > + BOTH, > + USER, > + SYSTEM, > +}; > + > +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 perf_config_item - element of perf's configs > + * > + * @value: array that has values for each kind (USER/SYSTEM/BOTH) > + * @is_custom: if the user or system config files contain this > + */ > +struct perf_config_item { > + const char *section; > + const char *name; > + char *value[CONFIG_KIND]; > + union { > + bool b; > + int i; > + u32 l; > + u64 ll; > + float f; > + double d; > + const char *s; > + } default_value; > + enum perf_config_type type; > + bool is_custom; > + struct list_head list; > +}; > + > +/** > + * struct perf_config_set - perf's config set from the config files > + * > + * @file_path: array that has paths of config files > + * @pos: current major config file > + * @config_list: perf_config_item list head > + */ > +struct perf_config_set { > + enum perf_config_kind pos; > + char *file_path[CONFIG_KIND]; > + bool file_usable[CONFIG_KIND]; > + struct list_head config_list; > +}; > + > +enum perf_config_idx { > + CONFIG_COLORS_TOP, > + CONFIG_COLORS_MEDIUM, > + CONFIG_COLORS_NORMAL, > + CONFIG_COLORS_SELECTED, > + CONFIG_COLORS_JUMP_ARROWS, > + CONFIG_COLORS_ADDR, > + CONFIG_COLORS_ROOT, > + CONFIG_TUI_REPORT, > + CONFIG_TUI_ANNOTATE, > + CONFIG_TUI_TOP, > + CONFIG_BUILDID_DIR, > + 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, > + CONFIG_GTK_ANNOTATE, > + CONFIG_GTK_REPORT, > + CONFIG_GTK_TOP, > + CONFIG_PAGER_CMD, > + CONFIG_PAGER_REPORT, > + CONFIG_PAGER_ANNOTATE, > + CONFIG_PAGER_TOP, > + CONFIG_PAGER_DIFF, > + CONFIG_HELP_FORMAT, > + CONFIG_HELP_AUTOCORRECT, > + CONFIG_HIST_PERCENTAGE, > + CONFIG_UI_SHOW_HEADERS, > + CONFIG_CALL_GRAPH_RECORD_MODE, > + CONFIG_CALL_GRAPH_DUMP_SIZE, > + CONFIG_CALL_GRAPH_PRINT_TYPE, > + CONFIG_CALL_GRAPH_ORDER, > + CONFIG_CALL_GRAPH_SORT_KEY, > + CONFIG_CALL_GRAPH_THRESHOLD, > + CONFIG_CALL_GRAPH_PRINT_LIMIT, > + CONFIG_REPORT_GROUP, > + CONFIG_REPORT_CHILDREN, > + CONFIG_REPORT_PERCENT_LIMIT, > + CONFIG_REPORT_QUEUE_SIZE, > + CONFIG_TOP_CHILDREN, > + CONFIG_MAN_VIEWER, > + CONFIG_KMEM_DEFAULT, > + CONFIG_END > +}; > + > +#define CONF_VAR(_sec, _name, _field, _val, _type) \ > + { .section = _sec, .name = _name, .default_value._field = _val, .type = _type } > + > +#define CONF_BOOL_VAR(_idx, _sec, _name, _val) \ > + [CONFIG_##_idx] = CONF_VAR(_sec, _name, b, _val, CONFIG_TYPE_BOOL) > +#define CONF_INT_VAR(_idx, _sec, _name, _val) \ > + [CONFIG_##_idx] = CONF_VAR(_sec, _name, i, _val, CONFIG_TYPE_INT) > +#define CONF_LONG_VAR(_idx, _sec, _name, _val) \ > + [CONFIG_##_idx] = CONF_VAR(_sec, _name, l, _val, CONFIG_TYPE_LONG) > +#define CONF_U64_VAR(_idx, _sec, _name, _val) \ > + [CONFIG_##_idx] = CONF_VAR(_sec, _name, ll, _val, CONFIG_TYPE_U64) > +#define CONF_FLOAT_VAR(_idx, _sec, _name, _val) \ > + [CONFIG_##_idx] = CONF_VAR(_sec, _name, f, _val, CONFIG_TYPE_FLOAT) > +#define CONF_DOUBLE_VAR(_idx, _sec, _name, _val) \ > + [CONFIG_##_idx] = CONF_VAR(_sec, _name, d, _val, CONFIG_TYPE_DOUBLE) > +#define CONF_STR_VAR(_idx, _sec, _name, _val) \ > + [CONFIG_##_idx] = CONF_VAR(_sec, _name, s, _val, CONFIG_TYPE_STRING) > + > +struct perf_config_set *perf_config_set__new(void); > +void perf_config_set__delete(struct perf_config_set *perf_configs); > + > +#endif /* __PERF_CONFIG_H */ > -- > 2.5.0 >