RE: [RFC/PATCH 3/3] perf probe: Move print logic into cmd_probe()

[åæéå / HIRAMATUïMASAMI]

> From: Namhyung Kim [mailto:namhyung@xxxxxxxxx] 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,