linux-kernel - RE: [RFC/PATCH 3/3] perf probe: Move print logic into cmd

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, 4 Sep 2015 02:17:31 +0000
From:	平松雅巳 / HIRAMATU，MASAMI 
	<masami.hiramatsu.pt@...achi.com>
To:	"'Namhyung Kim'" <namhyung@...nel.org>,
	Arnaldo Carvalho de Melo <acme@...nel.org>
CC:	Ingo Molnar <mingo@...nel.org>,
	Peter Zijlstra <a.p.zijlstra@...llo.nl>,
	Jiri Olsa <jolsa@...hat.com>,
	LKML <linux-kernel@...r.kernel.org>,
	Wang Nan <wangnan0@...wei.com>
Subject: RE: [RFC/PATCH 3/3] perf probe: Move print logic into cmd_probe()

> From: Namhyung Kim [mailto:namhyung@...il.com] On Behalf Of Namhyung Kim
> 
> Showing actual trace event when adding perf events is only needed in
> perf probe command.  But the add functionality itself can be used by
> other places.  So move the printing code into the cmd_probe().
> 
> Also it combines the output if more than one event is added.
> 
> Before:
>   $ sudo perf probe -a do_fork -a do_exit
>   Added new event:
>   probe:do_fork        (on do_fork)
> 
>   You can now use it in all perf tools, such as:
> 
>       perf record -e probe:do_fork -aR sleep 1
> 
>   Added new events:
>   probe:do_exit        (on do_exit)
>   probe:do_exit_1      (on do_exit)
> 
>   You can now use it in all perf tools, such as:
> 
>       perf record -e probe:do_exit_1 -aR sleep 1
> 
> After:
>   $ sudo perf probe -a do_fork -a do_exit
>   Added new events:
>   probe:do_fork        (on do_fork)
>   probe:do_exit        (on do_exit)
>   probe:do_exit_1      (on do_exit)
> 
>   You can now use it in all perf tools, such as:
> 
>       perf record -e probe:do_exit_1 -aR sleep 1
> 

This change is good for me :)
And have a comment below,

[...]
> @@ -496,7 +499,41 @@ __cmd_probe(int argc, const char **argv, const char *prefix __maybe_unused)
>  			usage_with_options(probe_usage, options);
>  		}
> 
> -		ret = add_perf_probe_events(params.events, params.nevents);
> +		ret = __add_perf_probe_events(params.events, params.nevents,
> +					      &pkgs);
> +		if (ret < 0)
> +			goto out_cleanup;
> +
> +		for (i = k = 0; i < params.nevents; i++)
> +			k += pkgs[i].ntevs;
> +
> +		pr_info("Added new event%s\n", (k > 1) ? "s:" : ":");
> +		for (i = 0; i < params.nevents; i++) {
> +			struct perf_probe_event *pev = pkgs[i].pev;
> +
> +			for (k = 0; k < pkgs[i].ntevs; k++) {
> +				struct probe_trace_event *tev = &pkgs[i].tevs[k];
> +
> +				/* We use tev's name for showing new events */
> +				show_perf_probe_event(tev->group, tev->event, pev,
> +						      tev->point.module, false);
> +
> +				/* Save the last valid name */
> +				event = tev->event;
> +				group = tev->group;
> +			}
> +		}
> +
> +		/* Note that it is possible to skip all events because of blacklist */
> +		if (event) {
> +			/* Show how to use the event. */
> +			pr_info("\nYou can now use it in all perf tools, such as:\n\n");
> +			pr_info("\tperf record -e %s:%s -aR sleep 1\n\n", group, event);
> +		}
> +
> +out_cleanup:
> +		cleanup_perf_probe_events(pkgs, params.nevents);

I think this should be a separated function to avoid increasing the size of __cmd_probe()

Thanks,