linux-kernel - Re: [PATCH 2/2] perf tools: Add +field argument support for --sort option

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Fri, 22 Aug 2014 12:16:51 -0300
From:	Arnaldo Carvalho de Melo <acme@...nel.org>
To:	Jiri Olsa <jolsa@...nel.org>
Cc:	linux-kernel@...r.kernel.org,
	Corey Ashford <cjashfor@...ux.vnet.ibm.com>,
	David Ahern <dsahern@...il.com>,
	Frederic Weisbecker <fweisbec@...il.com>,
	Ingo Molnar <mingo@...nel.org>,
	Jean Pihet <jean.pihet@...aro.org>,
	Namhyung Kim <namhyung@...nel.org>,
	Paul Mackerras <paulus@...ba.org>,
	Peter Zijlstra <a.p.zijlstra@...llo.nl>
Subject: Re: [PATCH 2/2] perf tools: Add +field argument support for --sort
 option

Em Fri, Aug 22, 2014 at 03:58:39PM +0200, Jiri Olsa escreveu:
> Adding support to add field(s) to default sort order
> via using the '+' prefix, like for report:
> 
>   $ perf report
>   Samples: 2K of event 'cycles', Event count (approx.): 882172583
>   Overhead  Command          Shared Object                Symbol
>      7.39%  swapper          [kernel.kallsyms]            [k] intel_idle
>      1.97%  firefox          libpthread-2.17.so           [.] pthread_mutex_lock
>      1.39%  firefox          [snd_hda_intel]              [k] azx_get_position
>      1.11%  firefox          libpthread-2.17.so           [.] pthread_mutex_unlock
> 
>   $ perf report -s +cpu
>   Samples: 2K of event 'cycles', Event count (approx.): 882172583
>   Overhead  Command          Shared Object                Symbol                      CPU
>      2.89%  swapper          [kernel.kallsyms]            [k] intel_idle              000
>      2.61%  swapper          [kernel.kallsyms]            [k] intel_idle              002
>      1.20%  swapper          [kernel.kallsyms]            [k] intel_idle              001
>      0.82%  firefox          libpthread-2.17.so           [.] pthread_mutex_lock      002
> 
> Works in general for commands using --sort option.
> 
> Cc: Arnaldo Carvalho de Melo <acme@...nel.org>
> Cc: Corey Ashford <cjashfor@...ux.vnet.ibm.com>
> Cc: David Ahern <dsahern@...il.com>
> Cc: Frederic Weisbecker <fweisbec@...il.com>
> Cc: Ingo Molnar <mingo@...nel.org>
> Cc: Jean Pihet <jean.pihet@...aro.org>
> Cc: Namhyung Kim <namhyung@...nel.org>
> Cc: Paul Mackerras <paulus@...ba.org>
> Cc: Peter Zijlstra <a.p.zijlstra@...llo.nl>
> Signed-off-by: Jiri Olsa <jolsa@...nel.org>
> ---
>  tools/perf/util/sort.c | 28 +++++++++++++++++++++++++++-
>  1 file changed, 27 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/perf/util/sort.c b/tools/perf/util/sort.c
> index 1958637cf136..b3d7dc1837ec 100644
> --- a/tools/perf/util/sort.c
> +++ b/tools/perf/util/sort.c
> @@ -1446,12 +1446,38 @@ static const char *get_default_sort_order(void)
>  	return default_sort_orders[sort__mode];
>  }
>  
> +static int setup_sort_order(void)
> +{
> +#define BUF_MAX 4096

This is an arbitrary value, and you will use this just here, so you
could just have used a number and later used sizeof(buf).

Anyway, what bothers me is the use of yet another static buffer.

The thing to use here is asprintf, that will do it all for you, format
_and_ allocate a buffer the size you need.

> +	static char buf[BUF_MAX];
> +
> +	if (!sort_order || is_strict_order(sort_order))
> +		return 0;
> +
> +	if (!strlen(sort_order + 1)) {
> +		error("Invalid --fields key: `+'");
> +		return -EINVAL;
> +	}
> +
> +	scnprintf(buf, BUF_MAX, "%s,%s",
> +		  get_default_sort_order(),
> +		  sort_order + 1);
> +
> +	sort_order = buf;

I.e. it would be better to have this as:

	char *new_sort_order;

	if (sort_order[1] == '\0') {
		error("Invalid --fields key: `+'");
		return -EINVAL;
	}

	if (asprintf(&new_sort_order, "%s,%s",
		     get_default_sort_order(), sort_order + 1) < 0) {
		error("Not enough memory to set up --sort");
		return -ENOMEM;
	}
		
	sort_order = new_sort_order;

> +	return 0;


Also please fix the error message, there is a cut'n'paste error there :-)

I applied the --fields one.

- Arnaldo

> +#undef BUF_MAX
> +}
> +
>  static int __setup_sorting(void)
>  {
>  	char *tmp, *tok, *str;
> -	const char *sort_keys = sort_order;
> +	const char *sort_keys;
>  	int ret = 0;
>  
> +	if (setup_sort_order())
> +		return -EINVAL;
> +
> +	sort_keys = sort_order;
>  	if (sort_keys == NULL) {
>  		if (is_strict_order(field_order)) {
>  			/*
> -- 
> 1.8.3.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/