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

[Arnaldo Carvalho de Melo <acme@kernel.org>]

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@kernel.org>
> Cc: Corey Ashford <cjashfor@linux.vnet.ibm.com>
> Cc: David Ahern <dsahern@gmail.com>
> Cc: Frederic Weisbecker <fweisbec@gmail.com>
> Cc: Ingo Molnar <mingo@kernel.org>
> Cc: Jean Pihet <jean.pihet@linaro.org>
> Cc: Namhyung Kim <namhyung@kernel.org>
> Cc: Paul Mackerras <paulus@samba.org>
> Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
> Signed-off-by: Jiri Olsa <jolsa@kernel.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