| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20150413061626.GF23913@sejong>
Date: Mon, 13 Apr 2015 15:16:26 +0900
From: Namhyung Kim <namhyung@...nel.org>
To: Taeung Song <treeze.taeung@...il.com>
Cc: Arnaldo Carvalho de Melo <acme@...nel.org>,
linux-kernel@...r.kernel.org, jolsa@...hat.com
Subject: Re: [PATCH 1/2] perf tools: Add 'perf-config' command
Hi Taeung,
On Sun, Apr 12, 2015 at 11:44:48PM +0900, Taeung Song wrote:
> The perf configuration file contains many variables which can make
> the perf command's action more effective and more skilful.
> But looking through state of configuration is difficult and
> there's no knowing what kind of other variables except variables in perfconfig.example exist.
> So This patch adds 'perf-config' command with '--all' option and a document for it.
>
> Signed-off-by: Taeung Song <treeze.taeung@...il.com>
> ---
Thanks for your work!
I think the documentation updates need to be shared with tools
manpages, or at least they can point to this document for details.
This can be further work though. ;-)
[SNIP]
> diff --git a/tools/perf/builtin-config.c b/tools/perf/builtin-config.c
> new file mode 100644
> index 0000000..7cf6d03
> --- /dev/null
> +++ b/tools/perf/builtin-config.c
> @@ -0,0 +1,71 @@
> +/*
> + * builtin-config.c
> + *
> + * Copyright (C) 2015, Taeung Song <treeze.taeung@...il.com>
> + *
> + */
> +#include "builtin.h"
> +
> +#include "perf.h"
> +
> +#include "util/cache.h"
> +#include "util/parse-options.h"
> +#include "util/util.h"
> +#include "util/debug.h"
> +
> +static struct {
> + bool all_action;
> +} params;
> +
> +static const char * const config_usage[] = {
> + "perf config [options]",
> + NULL
> +};
> +static const struct option config_options[] = {
> + OPT_GROUP("Action"),
> + OPT_BOOLEAN('a', "all", ¶ms.all_action, "print all configurations"),
> + OPT_END()
> +};
> +
> +static void check_argc(int argc, int limit)
> +{
> + if (argc >= limit && argc <= limit)
> + return;
> + error("wrong number of arguments");
> + usage_with_options(config_usage, config_options);
> +}
I don't know why this is needed. The -a option won't need this and we
can support to get/set any number of config items IMHO.
> +
> +static int show_config(const char *key, const char *value,
> + void *cb __maybe_unused)
> +{
> + if (value)
> + printf("%s=%s\n", key, value);
> + else
> + printf("%s\n", key);
> +
> + return 0;
> +}
> +
> +int cmd_config(int argc, const char **argv, const char *prefix __maybe_unused)
> +{
> + int ret = 0;
> +
> + argc = parse_options(argc, argv, config_options, config_usage,
> + PARSE_OPT_STOP_AT_NON_OPTION);
> + if (argc > 0) {
> + if (strcmp(argv[0], "-") == 0) {
> + pr_warning(" Error: '-' is not supported.\n");
I don't understand this. Why is the "-" so special?
And I think current behavior don't allow any argument. So what about this?
argc = parse_options(...);
if (argc)
usage_with_options(...);
if (params.all_action)
...
> + usage_with_options(config_usage, config_options);
> + }
> + }
> +
> + if (argc == 0 || params.all_action) {
> + check_argc(argc, 0);
> + ret = perf_config(show_config, NULL);
But doesn't it just print currently set items in a config file? I
guess -a/--all option should show this *AND* all possible config items
with default values. Current behavior can be done by -l/--list option
like git-config..
Thanks,
Namhyung
> + } else {
> + pr_warning("Error: Unknown argument.\n");
> + usage_with_options(config_usage, config_options);
> + }
> +
> + return ret;
> +}
> diff --git a/tools/perf/builtin.h b/tools/perf/builtin.h
> index 3688ad2..3f871b5 100644
> --- a/tools/perf/builtin.h
> +++ b/tools/perf/builtin.h
> @@ -17,6 +17,7 @@ extern int cmd_annotate(int argc, const char **argv, const char *prefix);
> extern int cmd_bench(int argc, const char **argv, const char *prefix);
> extern int cmd_buildid_cache(int argc, const char **argv, const char *prefix);
> extern int cmd_buildid_list(int argc, const char **argv, const char *prefix);
> +extern int cmd_config(int argc, const char **argv, const char *prefix);
> extern int cmd_diff(int argc, const char **argv, const char *prefix);
> extern int cmd_evlist(int argc, const char **argv, const char *prefix);
> extern int cmd_help(int argc, const char **argv, const char *prefix);
> diff --git a/tools/perf/command-list.txt b/tools/perf/command-list.txt
> index 00fcaf8..acc3ea7 100644
> --- a/tools/perf/command-list.txt
> +++ b/tools/perf/command-list.txt
> @@ -9,6 +9,7 @@ perf-buildid-cache mainporcelain common
> perf-buildid-list mainporcelain common
> perf-data mainporcelain common
> perf-diff mainporcelain common
> +perf-config mainporcelain common
> perf-evlist mainporcelain common
> perf-inject mainporcelain common
> perf-kmem mainporcelain common
> diff --git a/tools/perf/perf.c b/tools/perf/perf.c
> index b857fcb..604fa4a 100644
> --- a/tools/perf/perf.c
> +++ b/tools/perf/perf.c
> @@ -37,6 +37,7 @@ struct cmd_struct {
> static struct cmd_struct commands[] = {
> { "buildid-cache", cmd_buildid_cache, 0 },
> { "buildid-list", cmd_buildid_list, 0 },
> + { "config", cmd_config, 0 },
> { "diff", cmd_diff, 0 },
> { "evlist", cmd_evlist, 0 },
> { "help", cmd_help, 0 },
> --
> 1.9.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@...r.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists