Re: [PATCH v2 1/5] perf config: Introduce perf_config_set class
From: Taeung Song
Date: Thu Mar 17 2016 - 20:52:26 EST
On 03/18/2016 08:27 AM, Namhyung Kim wrote:
On Thu, Mar 17, 2016 at 11:10:12PM +0900, Taeung Song wrote:
Hi, Namhyung
On 03/17/2016 09:31 PM, Namhyung Kim wrote:
Hi Taeung,
On Mon, Mar 14, 2016 at 09:16:05PM +0900, Taeung Song wrote:
This infrastructure code was designed for
upcoming features of perf-config.
That collect config key-value pairs from user and
system config files (i.e. user wide ~/.perfconfig
and system wide $(sysconfdir)/perfconfig)
to manage perf's configs.
Cc: Namhyung Kim <namhyung@xxxxxxxxxx>
Cc: Jiri Olsa <jolsa@xxxxxxxxxx>
Signed-off-by: Taeung Song <treeze.taeung@xxxxxxxxx>
---
tools/perf/builtin-config.c | 1 +
tools/perf/util/config.c | 123 ++++++++++++++++++++++++++++++++++++++++++++
tools/perf/util/config.h | 21 ++++++++
3 files changed, 145 insertions(+)
create mode 100644 tools/perf/util/config.h
diff --git a/tools/perf/builtin-config.c b/tools/perf/builtin-config.c
index c42448e..412c725 100644
--- a/tools/perf/builtin-config.c
+++ b/tools/perf/builtin-config.c
@@ -12,6 +12,7 @@
#include <subcmd/parse-options.h>
#include "util/util.h"
#include "util/debug.h"
+#include "util/config.h"
static bool use_system_config, use_user_config;
diff --git a/tools/perf/util/config.c b/tools/perf/util/config.c
index 4e72763..b9660e4 100644
--- a/tools/perf/util/config.c
+++ b/tools/perf/util/config.c
@@ -13,6 +13,7 @@
#include <subcmd/exec-cmd.h>
#include "util/hist.h" /* perf_hist_config */
#include "util/llvm-utils.h" /* perf_llvm_config */
+#include "config.h"
#define MAXNAME (256)
@@ -506,6 +507,128 @@ out:
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;
+ }
Hmm.. why do you remove the section list?
IMHO, there are several reasons
1) To use only one list (default config, custom config(user/system))
1-1) I used two list that were 'list_head sections'
and 'config_item default_configs[]'. So if checking
type of config variable, two for-loop must be needed
for each list. Because two structure was different i.e.
'sections' list mean config_section list
that each section contain config_element list.
(there wasn't telling about correct type of 'value' instead of string(char
*))
struct config_element {
char *name;
char *value;
struct list_head list;
};
struct config_section {
char *name;
struct list_head element_head;
struct list_head list;
};
'struct config_item default_configs[]' mean all default configs.
struct config_item {
const char *section;
const char *name;
union {
bool b;
int i;
u32 l;
u64 ll;
float f;
double d;
const char *s;
} value;
enum config_type type;
const char *desc;
};
IMHO, I think this is a bit complex
and I want to simplify the perf's config list on perf-config.
2) A small number of perf's configs
I think perf's configs aren't too many so I think
two structure for section and element aren't needed.
OK.
3) A object for a config variable need to have enough info for itself
This is a bit similar to 1) reason.
If using only 'struct config_item' for the config list,
it can contain section name, name, values(default, user config,
system config, both config), correct type, etc.
If we do, we needn't to find detail for a config variable at other objects
e.g.
When we find correct type of a config variable,
we needn't to do for-loop for default_configs[] in order to know the
type.
I'm not sure I understand you correctly, but I think this is not
related to the two-level structure.
What you said is right.
I just thought using two lists (list_head sections, default_configs[])
was complex..
At this patchset, I only focused on simplifying the config list on
perf-config..
As you said, it hasn't problems to use the two-level structure i.e.
struct config_element
struct config_section (that has multiple elements)
So, what about this structures ?
(you may already think about it, but..)
struct config_element {
char *name;
char *value; /*from the two config file (user/system)*/
union {
bool b;
int i;
u32 l;
u64 ll;
float f;
double d;
const char *s;
} default_value; /* perf's default config value */
enum config_type type;
struct list_head list;
};
struct config_section {
char *name;
struct list_head element_head;
struct list_head list;
};
Because I want to use only one list (default_configs + sections)
for more concise code than old code.
I think this is better than old two structure.
+
+ 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->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, const char *value)
+{
+ char *val = strdup(value);
+
+ if (!val)
+ return -1;
+ config->value = val;
It seems to overwrite old value..
Yes, I know it.
If don't using '--user' or '--system',
there isn't exclusive config file path
then have to read both config files.
But because user config file has a high order of priority,
if two config file has same variable, old value(for system config)
must be overwrote by new value(for user config).
But shouldn't it free the old value before overwriting?
Sorry, I missed free() out.
I'll fix it.
Thanks,
Taeung
+
+ return 0;
+}