Re: [PATCH] perf: support running perf binaries with a dash in their name

[David Ahern]

On 9/11/17 4:14 AM, Milian Wolff wrote:
> Previously the part behind "perf-" was interpreted as an internal
> perf command. If the suffix could not be handled, the execution
> was stopped. This makes it impossible to launch perf binaries that
> got renamed to have the `perf-` prefix. This is e.g. the case for
> appimages (e.g. "perf-x86_64.AppImage"), but would also apply to
> all other scenarios where users symlink or rename perf themselves:
> 
...
> ~~~~~
> $ ./perf-custom-suffix list
> 
> List of pre-defined events (to be used in -e):
> ...
> ~~~~~
> 
> Cc: Arnaldo Carvalho de Melo <acme@xxxxxxxxxx>
> Cc: David Ahern <dsahern@xxxxxxxxx>
> Cc: Namhyung Kim <namhyung@xxxxxxxxxx>
> Cc: Peter Zijlstra <a.p.zijlstra@xxxxxxxxx>
> Cc: Yao Jin <yao.jin@xxxxxxxxxxxxxxx>
> Signed-off-by: Milian Wolff <milian.wolff@xxxxxxxx>
> ---
>  tools/perf/perf.c | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/tools/perf/perf.c b/tools/perf/perf.c
> index e0279babe0c0..7a3a39014446 100644
> --- a/tools/perf/perf.c
> +++ b/tools/perf/perf.c
> @@ -467,15 +467,19 @@ int main(int argc, const char **argv)
>  	 *  - cannot execute it externally (since it would just do
>  	 *    the same thing over again)
>  	 *
> -	 * So we just directly call the internal command handler, and
> -	 * die if that one cannot handle it.
> +	 * So we just directly call the internal command handler. If that one
> +	 * fails to handle this, then maybe we just run a renamed perf binary
> +	 * that contains a dash in its name. To handle this scenario, we just
> +	 * fall through and ignore the "xxxx" part of the command string.
>  	 */
>  	if (strstarts(cmd, "perf-")) {
>  		cmd += 5;
>  		argv[0] = cmd;
>  		handle_internal_command(argc, argv);
> -		fprintf(stderr, "cannot handle %s internally", cmd);
> -		goto out;
> +		// if the command is handled, the above function does not return
> +		// undo changes and fall through in such a case

Those should be /* */ not //

> +		cmd -= 5;
> +		argv[0] = cmd;
>  	}
>  	if (strstarts(cmd, "trace")) {
>  #ifdef HAVE_LIBAUDIT_SUPPORT
> 

other than that LGTM and long over due.

Acked-by: David Ahern <dsahern@xxxxxxxxx>