Hi all, :)
This is simple patchset for perf-config
to fix small bugs and refactor code.
I'd appreciate some feedback on this patchset.
The code is also avaiable at 'config/refactoring' branch on
git://github.com/taeung/linux-perf.git
Thanks,
Taeung
Taeung Song (7):
perf config: Refactor a duplicated code for config file name
perf config: Check list empty before showing configs
perf config: Use none_err for all cases that nothing configured
perf config: Invert if statements to reduce nesting in cmd_config()
perf config: Correctly check whether it is from system config
perf config: Finally write changed configs on config file at a time
perf config: No free config set when it's initialization failed
tools/perf/builtin-config.c | 92 +++++++++++++++++++++++++--------------------
tools/perf/util/config.c | 7 +---
2 files changed, 54 insertions(+), 45 deletions(-)
--
2.7.4
Cc: Jiri Olsa <[email protected]>
Cc: Namhyung Kim <[email protected]>
Signed-off-by: Taeung Song <[email protected]>
---
tools/perf/builtin-config.c | 19 ++++++++-----------
1 file changed, 8 insertions(+), 11 deletions(-)
diff --git a/tools/perf/builtin-config.c b/tools/perf/builtin-config.c
index 55f04f8..80668fa 100644
--- a/tools/perf/builtin-config.c
+++ b/tools/perf/builtin-config.c
@@ -159,6 +159,7 @@ int cmd_config(int argc, const char **argv)
int i, ret = 0;
struct perf_config_set *set;
char *user_config = mkpath("%s/.perfconfig", getenv("HOME"));
+ const char *config_filename;
argc = parse_options(argc, argv, config_options, config_usage,
PARSE_OPT_STOP_AT_NON_OPTION);
@@ -175,6 +176,11 @@ int cmd_config(int argc, const char **argv)
else if (use_user_config)
config_exclusive_filename = user_config;
+ if (!config_exclusive_filename)
+ config_filename = user_config;
+ else
+ config_filename = config_exclusive_filename;
+
/*
* At only 'config' sub-command, individually use the config set
* because of reinitializing with options config file location.
@@ -192,13 +198,9 @@ int cmd_config(int argc, const char **argv)
parse_options_usage(config_usage, config_options, "l", 1);
} else {
ret = show_config(set);
- if (ret < 0) {
- const char * config_filename = config_exclusive_filename;
- if (!config_exclusive_filename)
- config_filename = user_config;
+ if (ret < 0)
pr_err("Nothing configured, "
"please check your %s \n", config_filename);
- }
}
break;
default:
@@ -221,13 +223,8 @@ int cmd_config(int argc, const char **argv)
if (value == NULL)
ret = show_spec_config(set, var);
- else {
- const char *config_filename = config_exclusive_filename;
-
- if (!config_exclusive_filename)
- config_filename = user_config;
+ else
ret = set_config(set, config_filename, var, value);
- }
free(arg);
}
} else
--
2.7.4
Currently there's only one error message for nothing configured.
Before:
$ perf config -l
Nothing configured, please check your /home/taeung/.perfconfig
$ perf config report.queue-size
So use none_err to handle all cases when nothing configured,
and also use it instead of out_err that skip the error message.
After:
$ perf config -l
Nothing configured, please check your /home/taeung/.perfconfig
$ perf config report.queue-size
Nothing configured, please check your /home/taeung/.perfconfig
Cc: Jiri Olsa <[email protected]>
Cc: Namhyung Kim <[email protected]>
Signed-off-by: Taeung Song <[email protected]>
---
tools/perf/builtin-config.c | 17 +++++++++++------
1 file changed, 11 insertions(+), 6 deletions(-)
diff --git a/tools/perf/builtin-config.c b/tools/perf/builtin-config.c
index 9ec8664..ad0a112 100644
--- a/tools/perf/builtin-config.c
+++ b/tools/perf/builtin-config.c
@@ -188,7 +188,7 @@ int cmd_config(int argc, const char **argv)
set = perf_config_set__new();
if (!set) {
ret = -1;
- goto out_err;
+ goto none_err;
}
switch (actions) {
@@ -199,8 +199,7 @@ int cmd_config(int argc, const char **argv)
} else {
ret = show_config(set);
if (ret < 0)
- pr_err("Nothing configured, "
- "please check your %s \n", config_filename);
+ goto none_err;
}
break;
default:
@@ -221,9 +220,11 @@ int cmd_config(int argc, const char **argv)
break;
}
- if (value == NULL)
+ if (value == NULL) {
ret = show_spec_config(set, var);
- else
+ if (ret < 0)
+ goto none_err;
+ } else
ret = set_config(set, config_filename, var, value);
free(arg);
}
@@ -231,7 +232,11 @@ int cmd_config(int argc, const char **argv)
usage_with_options(config_usage, config_options);
}
+none_err:
+ if (ret < 0 && (!set || list_empty(&set->sections)))
+ pr_err("Nothing configured, "
+ "please check your %s \n", config_filename);
+
perf_config_set__delete(set);
-out_err:
return ret;
}
--
2.7.4
If existent config files contains nothing,
the sections list in config_set can be empty.
So check not only NULL pointer of config_set but
also the list in config_set.
Cc: Jiri Olsa <[email protected]>
Cc: Namhyung Kim <[email protected]>
Signed-off-by: Taeung Song <[email protected]>
---
tools/perf/builtin-config.c | 4 ++--
tools/perf/util/config.c | 2 +-
2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/tools/perf/builtin-config.c b/tools/perf/builtin-config.c
index 80668fa..9ec8664 100644
--- a/tools/perf/builtin-config.c
+++ b/tools/perf/builtin-config.c
@@ -75,7 +75,7 @@ static int show_spec_config(struct perf_config_set *set, const char *var)
struct perf_config_section *section;
struct perf_config_item *item;
- if (set == NULL)
+ if (set == NULL || list_empty(&set->sections))
return -1;
perf_config_items__for_each_entry(&set->sections, section) {
@@ -105,7 +105,7 @@ static int show_config(struct perf_config_set *set)
struct perf_config_section *section;
struct perf_config_item *item;
- if (set == NULL)
+ if (set == NULL || list_empty(&set->sections))
return -1;
perf_config_set__for_each_entry(set, section, item) {
diff --git a/tools/perf/util/config.c b/tools/perf/util/config.c
index 8d724f0..492c862 100644
--- a/tools/perf/util/config.c
+++ b/tools/perf/util/config.c
@@ -707,7 +707,7 @@ int perf_config(config_fn_t fn, void *data)
struct perf_config_section *section;
struct perf_config_item *item;
- if (config_set == NULL)
+ if (config_set == NULL || list_empty(&config_set->sections))
return -1;
perf_config_set__for_each_entry(config_set, section, item) {
--
2.7.4
Cc: Jiri Olsa <[email protected]>
Cc: Namhyung Kim <[email protected]>
Signed-off-by: Taeung Song <[email protected]>
---
tools/perf/builtin-config.c | 50 ++++++++++++++++++++++++---------------------
1 file changed, 27 insertions(+), 23 deletions(-)
diff --git a/tools/perf/builtin-config.c b/tools/perf/builtin-config.c
index ad0a112..f4596ef 100644
--- a/tools/perf/builtin-config.c
+++ b/tools/perf/builtin-config.c
@@ -203,33 +203,37 @@ int cmd_config(int argc, const char **argv)
}
break;
default:
- if (argc) {
- for (i = 0; argv[i]; i++) {
- char *var, *value;
- char *arg = strdup(argv[i]);
-
- if (!arg) {
- pr_err("%s: strdup failed\n", __func__);
- ret = -1;
- break;
- }
+ if (!argc) {
+ usage_with_options(config_usage, config_options);
+ break;
+ }
- if (parse_config_arg(arg, &var, &value) < 0) {
- free(arg);
- ret = -1;
- break;
- }
+ for (i = 0; argv[i]; i++) {
+ char *var, *value;
+ char *arg = strdup(argv[i]);
+
+ if (!arg) {
+ pr_err("%s: strdup failed\n", __func__);
+ ret = -1;
+ break;
+ }
- if (value == NULL) {
- ret = show_spec_config(set, var);
- if (ret < 0)
- goto none_err;
- } else
- ret = set_config(set, config_filename, var, value);
+ if (parse_config_arg(arg, &var, &value) < 0) {
free(arg);
+ ret = -1;
+ break;
}
- } else
- usage_with_options(config_usage, config_options);
+
+ if (value) {
+ ret = set_config(set, config_filename, var, value);
+ continue;
+ }
+ ret = show_spec_config(set, var);
+ if (ret < 0)
+ goto none_err;
+
+ free(arg);
+ }
}
none_err:
--
2.7.4
Currently set_config() can be repeatedly called for each
input config on the below case:
$ perf config kmem.default=slab report.children=false ...
But it's a waste, so finally write changed configs at a time.
Cc: Jiri Olsa <[email protected]>
Cc: Namhyung Kim <[email protected]>
Signed-off-by: Taeung Song <[email protected]>
---
tools/perf/builtin-config.c | 14 ++++++++++----
1 file changed, 10 insertions(+), 4 deletions(-)
diff --git a/tools/perf/builtin-config.c b/tools/perf/builtin-config.c
index c76aacf..793a729 100644
--- a/tools/perf/builtin-config.c
+++ b/tools/perf/builtin-config.c
@@ -33,8 +33,7 @@ static struct option config_options[] = {
OPT_END()
};
-static int set_config(struct perf_config_set *set, const char *file_name,
- const char *var, const char *value)
+static int set_config(struct perf_config_set *set, const char *file_name)
{
struct perf_config_section *section = NULL;
struct perf_config_item *item = NULL;
@@ -48,7 +47,6 @@ static int set_config(struct perf_config_set *set, const char *file_name,
if (!fp)
return -1;
- perf_config_set__collect(set, file_name, var, value);
fprintf(fp, "%s\n", first_line);
/* overwrite configvariables */
@@ -160,6 +158,7 @@ int cmd_config(int argc, const char **argv)
struct perf_config_set *set;
char *user_config = mkpath("%s/.perfconfig", getenv("HOME"));
const char *config_filename;
+ bool changed = false;
argc = parse_options(argc, argv, config_options, config_usage,
PARSE_OPT_STOP_AT_NON_OPTION);
@@ -225,7 +224,11 @@ int cmd_config(int argc, const char **argv)
}
if (value) {
- ret = set_config(set, config_filename, var, value);
+ ret = perf_config_set__collect(set, config_filename,
+ var, value);
+ if (ret < 0)
+ break;
+ changed = true;
continue;
}
ret = show_spec_config(set, var);
@@ -234,6 +237,9 @@ int cmd_config(int argc, const char **argv)
free(arg);
}
+
+ if (changed)
+ ret = set_config(set, config_filename);
}
none_err:
--
2.7.4
Currently if perf_config_set__init() failed in perf_config_set__new(),
config_set will be freed.
However, if we do, config setting feature can't work sometimes
when user or system config files are nonexistent.
So let the config set be empty, not freed totally.
(it'll be freed at the tail end)
Before:
$ cat ~/.perfconfig
cat: /root/.perfconfig: No such file or directory
$ perf config --user report.children=false
Nothing configured, please check your /root/.perfconfig
After:
$ cat ~/.perfconfig
cat: /root/.perfconfig: No such file or directory
$ perf config --user report.children=false
$ cat ~/.perfconfig
# this file is auto-generated.
[report]
children = false
Cc: Jiri Olsa <[email protected]>
Cc: Namhyung Kim <[email protected]>
Signed-off-by: Taeung Song <[email protected]>
---
tools/perf/util/config.c | 5 +----
1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/tools/perf/util/config.c b/tools/perf/util/config.c
index 492c862..3c89d74 100644
--- a/tools/perf/util/config.c
+++ b/tools/perf/util/config.c
@@ -691,10 +691,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);
- set = NULL;
- }
+ perf_config_set__init(set);
}
return set;
--
2.7.4
Currently no bugs in the checking code.
But adjust it to correctly check item->from_system_config,
not section's from_system_config.
Cc: Jiri Olsa <[email protected]>
Cc: Namhyung Kim <[email protected]>
Signed-off-by: Taeung Song <[email protected]>
---
tools/perf/builtin-config.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tools/perf/builtin-config.c b/tools/perf/builtin-config.c
index f4596ef..c76aacf 100644
--- a/tools/perf/builtin-config.c
+++ b/tools/perf/builtin-config.c
@@ -58,7 +58,7 @@ static int set_config(struct perf_config_set *set, const char *file_name,
fprintf(fp, "[%s]\n", section->name);
perf_config_items__for_each_entry(§ion->items, item) {
- if (!use_system_config && section->from_system_config)
+ if (!use_system_config && item->from_system_config)
continue;
if (item->value)
fprintf(fp, "\t%s = %s\n",
--
2.7.4