Messages in this thread | | | Date | Wed, 8 Nov 2023 17:28:14 -0300 | From | Arnaldo Carvalho de Melo <> | Subject | Re: [PATCH 2/2] perf lock info: Enforce exactly one of --map and --thread |
| |
Em Wed, Nov 01, 2023 at 11:00:42PM -0700, Namhyung Kim escreveu: > On Wed, Nov 1, 2023 at 7:35 AM Nick Forrington <nick.forrington@arm.com> wrote: > > > > > > On 31/10/2023 15:38, Arnaldo Carvalho de Melo wrote: > > > Em Tue, Oct 31, 2023 at 12:05:25PM +0000, Nick Forrington escreveu: > > >> Improve error reporting for command line arguments. > > >> > > >> Display error/usage if neither --map or --thread are specified (rather > > >> than a non user-friendly error "Unknown type of information"). > > >> > > >> Display error/usage if both --map and --thread are specified (rather > > >> than ignoring "--map" and displaying only thread information). > > > Shouldn't one of them be the default so that we type less for the most > > > common usage? > > > > > > - Arnaldo > > > > > > > There isn't an obvious choice (to me) for which would be the default. > > > > Both options display completely different data/outputs, so I think it > > makes sense to be explicit about which data is requested. > > Maybe we can default to display both. :)
Yeah, that would be a better approach, I think.
- Arnaldo
> Thanks, > Namhyung > > > > > > > An alternative could be to use sub-commands e.g. "perf lock info > > threads" or just "perf lock threads", although changing the existing > > options would be more disruptive. > > > > > > Cheers, > > Nick > > > > >> Signed-off-by: Nick Forrington <nick.forrington@arm.com> > > >> --- > > >> tools/perf/builtin-lock.c | 25 +++++++++++++++++++++++++ > > >> 1 file changed, 25 insertions(+) > > >> > > >> diff --git a/tools/perf/builtin-lock.c b/tools/perf/builtin-lock.c > > >> index 3aa8ba5ad928..cf29f648d291 100644 > > >> --- a/tools/perf/builtin-lock.c > > >> +++ b/tools/perf/builtin-lock.c > > >> @@ -2021,6 +2021,27 @@ static int check_lock_report_options(const struct option *options, > > >> return 0; > > >> } > > >> > > >> +static int check_lock_info_options(const struct option *options, > > >> + const char * const *usage) > > >> +{ > > >> + if (!info_map && !info_threads) { > > >> + pr_err("Requires one of --map or --threads\n"); > > >> + parse_options_usage(usage, options, "map", 0); > > >> + parse_options_usage(NULL, options, "threads", 0); > > >> + return -1; > > >> + > > >> + } > > >> + > > >> + if (info_map && info_threads) { > > >> + pr_err("Cannot show map and threads together\n"); > > >> + parse_options_usage(usage, options, "map", 0); > > >> + parse_options_usage(NULL, options, "threads", 0); > > >> + return -1; > > >> + } > > >> + > > >> + return 0; > > >> +} > > >> + > > >> static int check_lock_contention_options(const struct option *options, > > >> const char * const *usage) > > >> > > >> @@ -2709,6 +2730,10 @@ int cmd_lock(int argc, const char **argv) > > >> if (argc) > > >> usage_with_options(info_usage, info_options); > > >> } > > >> + > > >> + if (check_lock_info_options(info_options, info_usage) < 0) > > >> + return -1; > > >> + > > >> /* recycling report_lock_ops */ > > >> trace_handler = &report_lock_ops; > > >> rc = __cmd_report(true); > > >> -- > > >> 2.42.0 > > >> > > >>
--
- Arnaldo
| |