Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

libbpf-tools: fix some tools error info and usage #3245

Closed
wants to merge 1 commit into from
Closed

libbpf-tools: fix some tools error info and usage #3245

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 5 additions & 5 deletions libbpf-tools/biolatency.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,20 +40,20 @@ const char *argp_program_bug_address = "<bpf@vger.kernel.org>";
const char argp_program_doc[] =
"Summarize block device I/O latency as a histogram.\n"
"\n"
"USAGE: biolatency [--help] [-T] [-m] [-Q] [-D] [-F] [-d] [interval] [count]\n"
"USAGE: biolatency [--help] [-T] [-M] [-Q] [-D] [-F] [-d DISK] [interval] [count]\n"
"\n"
"EXAMPLES:\n"
" biolatency # summarize block I/O latency as a histogram\n"
" biolatency 1 10 # print 1 second summaries, 10 times\n"
" biolatency -mT 1 # 1s summaries, milliseconds, and timestamps\n"
" biolatency -MT 1 # 1s summaries, milliseconds, and timestamps\n"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

biolatency.py is using -m, why sudden and diverging change?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are a bunch more cases of arbitrary changing the case of existing command line arguments with no justification. Please provide rationale for the changes in your commit messages, not just the "what".

Oh, my motivation is I think maybe it's easier to remember. Lowercase parameters need parameter values ​​(except -v), and uppercase ones belong to the switch class, so you don't need to add parameter values.

Also, while I have your attention. Can you please check #3224 (comment)? Would you be able to add that check? Thanks!

Oh sorry, I haven't checked the email from iovisor/bcc recently, I missed it. I will look at it later （Maybe weekend）.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't heard about such naming convention, tbh. I think the harm of changing existing arguments and, especially, diverging from .py tool, is much bigger. So let's not do such renamings unnecessarily.

And thanks for checking the other PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it, I will be consistent with .py tool. Thanks!

" biolatency -Q # include OS queued time in I/O time\n"
" biolatency -D # show each disk device separately\n"
" biolatency -F # show I/O flags separately\n"
" biolatency -d sdc # Trace sdc only\n";

static const struct argp_option opts[] = {
{ "timestamp", 'T', NULL, 0, "Include timestamp on output" },
{ "milliseconds", 'm', NULL, 0, "Millisecond histogram" },
{ "milliseconds", 'M', NULL, 0, "Millisecond histogram" },
{ "queued", 'Q', NULL, 0, "Include OS queued time in I/O time" },
{ "disk", 'D', NULL, 0, "Print a histogram per disk device" },
{ "flag", 'F', NULL, 0, "Print a histogram per set of I/O flags" },
Expand All @@ -70,7 +70,7 @@ static error_t parse_arg(int key, char *arg, struct argp_state *state)
case 'v':
env.verbose = true;
break;
case 'm':
case 'M':
env.milliseconds = true;
break;
case 'Q':
Expand Down Expand Up @@ -250,7 +250,7 @@ int main(int argc, char **argv)

obj = biolatency_bpf__open();
if (!obj) {
fprintf(stderr, "failed to open and/or load BPF ojbect\n");
fprintf(stderr, "failed to open BPF object\n");
return 1;
}

Expand Down
4 changes: 2 additions & 2 deletions libbpf-tools/biopattern.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ const char *argp_program_bug_address = "<bpf@vger.kernel.org>";
const char argp_program_doc[] =
"Show block device I/O pattern.\n"
"\n"
"USAGE: biopattern [--help] [-T] [-d] [interval] [count]\n"
"USAGE: biopattern [--help] [-T] [-d DISK] [interval] [count]\n"
"\n"
"EXAMPLES:\n"
" biopattern # show block I/O pattern\n"
Expand Down Expand Up @@ -178,7 +178,7 @@ int main(int argc, char **argv)

obj = biopattern_bpf__open();
if (!obj) {
fprintf(stderr, "failed to open and/or load BPF ojbect\n");
fprintf(stderr, "failed to open BPF object\n");
return 1;
}

Expand Down
4 changes: 2 additions & 2 deletions libbpf-tools/biosnoop.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ const char *argp_program_bug_address = "<bpf@vger.kernel.org>";
const char argp_program_doc[] =
"Trace block I/O.\n"
"\n"
"USAGE: biosnoop [--help] [-d] [-Q]\n"
"USAGE: biosnoop [--help] [-d DISK] [-Q]\n"
"\n"
"EXAMPLES:\n"
" biosnoop # trace all block I/O\n"
Expand Down Expand Up @@ -190,7 +190,7 @@ int main(int argc, char **argv)

obj = biosnoop_bpf__open();
if (!obj) {
fprintf(stderr, "failed to open and/or load BPF ojbect\n");
fprintf(stderr, "failed to open BPF object\n");
return 1;
}

Expand Down
8 changes: 4 additions & 4 deletions libbpf-tools/biostacks.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ const char *argp_program_bug_address = "<bpf@vger.kernel.org>";
const char argp_program_doc[] =
"Tracing block I/O with init stacks.\n"
"\n"
"USAGE: biostacks [--help] [-d disk] [-m] [duration]\n"
"USAGE: biostacks [--help] [-d DISK] [-M] [duration]\n"
"\n"
"EXAMPLES:\n"
" biostacks # trace block I/O with init stacks.\n"
Expand All @@ -36,7 +36,7 @@ const char argp_program_doc[] =

static const struct argp_option opts[] = {
{ "disk", 'd', "DISK", 0, "Trace this disk only" },
{ "milliseconds", 'm', NULL, 0, "Millisecond histogram" },
{ "milliseconds", 'M', NULL, 0, "Millisecond histogram" },
{ "verbose", 'v', NULL, 0, "Verbose debug output" },
{},
};
Expand All @@ -56,7 +56,7 @@ static error_t parse_arg(int key, char *arg, struct argp_state *state)
argp_usage(state);
}
break;
case 'm':
case 'M':
env.milliseconds = true;
break;
case ARGP_KEY_ARG:
Expand Down Expand Up @@ -151,7 +151,7 @@ int main(int argc, char **argv)

obj = biostacks_bpf__open();
if (!obj) {
fprintf(stderr, "failed to open and/or load BPF ojbect\n");
fprintf(stderr, "failed to open BPF object\n");
return 1;
}

Expand Down
4 changes: 2 additions & 2 deletions libbpf-tools/bitesize.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ const char *argp_program_bug_address = "<bpf@vger.kernel.org>";
const char argp_program_doc[] =
"Summarize block device I/O size as a histogram.\n"
"\n"
"USAGE: bitesize [--help] [-T] [-c] [-d] [interval] [count]\n"
"USAGE: bitesize [--help] [-T] [-c COMM] [-d DISK] [interval] [count]\n"
"\n"
"EXAMPLES:\n"
" bitesize # summarize block I/O latency as a histogram\n"
Expand Down Expand Up @@ -173,7 +173,7 @@ int main(int argc, char **argv)

obj = bitesize_bpf__open();
if (!obj) {
fprintf(stderr, "failed to open and/or load BPF ojbect\n");
fprintf(stderr, "failed to open BPF object\n");
return 1;
}

Expand Down
10 changes: 5 additions & 5 deletions libbpf-tools/cpudist.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,20 +37,20 @@ const char *argp_program_bug_address = "<bpf@vger.kernel.org>";
const char argp_program_doc[] =
"Summarize on-CPU time per task as a histogram.\n"
"\n"
"USAGE: cpudist [--help] [-O] [-T] [-m] [-P] [-L] [-p PID] [interval] [count]\n"
"USAGE: cpudist [--help] [-O] [-T] [-M] [-P] [-L] [-p PID] [interval] [count]\n"
"\n"
"EXAMPLES:\n"
" cpudist # summarize on-CPU time as a histogram"
" cpudist -O # summarize off-CPU time as a histogram"
" cpudist 1 10 # print 1 second summaries, 10 times"
" cpudist -mT 1 # 1s summaries, milliseconds, and timestamps"
" cpudist -MT 1 # 1s summaries, milliseconds, and timestamps"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

similarly. py version is sticking with -m. What's your rationale for making these incompatible changes?

" cpudist -P # show each PID separately"
" cpudist -p 185 # trace PID 185 only";

static const struct argp_option opts[] = {
{ "offcpu", 'O', NULL, 0, "Measure off-CPU time" },
{ "timestamp", 'T', NULL, 0, "Include timestamp on output" },
{ "milliseconds", 'm', NULL, 0, "Millisecond histogram" },
{ "milliseconds", 'M', NULL, 0, "Millisecond histogram" },
{ "pids", 'P', NULL, 0, "Print a histogram per process ID" },
{ "tids", 'L', NULL, 0, "Print a histogram per thread ID" },
{ "pid", 'p', "PID", 0, "Trace this PID only" },
Expand All @@ -66,7 +66,7 @@ static error_t parse_arg(int key, char *arg, struct argp_state *state)
case 'v':
env.verbose = true;
break;
case 'm':
case 'M':
env.milliseconds = true;
break;
case 'p':
Expand Down Expand Up @@ -203,7 +203,7 @@ int main(int argc, char **argv)

obj = cpudist_bpf__open();
if (!obj) {
fprintf(stderr, "failed to open and/or load BPF ojbect\n");
fprintf(stderr, "failed to open BPF object\n");
return 1;
}

Expand Down
10 changes: 5 additions & 5 deletions libbpf-tools/drsnoop.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,18 +31,18 @@ const char *argp_program_bug_address = "<bpf@vger.kernel.org>";
const char argp_program_doc[] =
"Trace direct reclaim latency.\n"
"\n"
"USAGE: drsnoop [--help] [-p PID] [-t TID] [-d DURATION] [-e]\n"
"USAGE: drsnoop [--help] [-p PID] [-t TID] [-d DURATION] [-E]\n"
"\n"
"EXAMPLES:\n"
" drsnoop # trace all direct reclaim events\n"
" drsnoop -p 123 # trace pid 123\n"
" drsnoop -t 123 # trace tid 123 (use for threads only)\n"
" drsnoop -d 10 # trace for 10 seconds only\n"
" drsnoop -e # trace all direct reclaim events with extended faileds\n";
" drsnoop -E # trace all direct reclaim events with extended faileds\n";

static const struct argp_option opts[] = {
{ "duration", 'd', "DURATION", 0, "Total duration of trace in seconds" },
{ "extended", 'e', NULL, 0, "Extended fields output" },
{ "extended", 'E', NULL, 0, "Extended fields output" },
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this seems to be libbpf-tools exclusive argument, but again, why changing it in the first place?

{ "pid", 'p', "PID", 0, "Process PID to trace" },
{ "tid", 't', "TID", 0, "Thread TID to trace" },
{ "verbose", 'v', NULL, 0, "Verbose debug output" },
Expand All @@ -69,7 +69,7 @@ static error_t parse_arg(int key, char *arg, struct argp_state *state)
}
env.duration = duration;
break;
case 'e':
case 'E':
env.extended = true;
break;
case 'p':
Expand Down Expand Up @@ -156,7 +156,7 @@ int main(int argc, char **argv)

obj = drsnoop_bpf__open();
if (!obj) {
fprintf(stderr, "failed to open and/or load BPF object\n");
fprintf(stderr, "failed to open BPF object\n");
return 1;
}

Expand Down
16 changes: 8 additions & 8 deletions libbpf-tools/execsnoop.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,26 +40,26 @@ const char *argp_program_bug_address = "<bpf@vger.kernel.org>";
const char argp_program_doc[] =
"Trace open family syscalls\n"
"\n"
"USAGE: execsnoop [-h] [-T] [-t] [-x] [-u UID] [-q] [-n NAME] [-l LINE] [-U]\n"
"USAGE: execsnoop [-h] [-T] [-t] [-X] [-u UID] [-Q] [-n NAME] [-l LINE] [-U]\n"
" [--max-args MAX_ARGS]\n"
"\n"
"EXAMPLES:\n"
" ./execsnoop # trace all exec() syscalls\n"
" ./execsnoop -x # include failed exec()s\n"
" ./execsnoop -X # include failed exec()s\n"
" ./execsnoop -T # include time (HH:MM:SS)\n"
" ./execsnoop -U # include UID\n"
" ./execsnoop -u 1000 # only trace UID 1000\n"
" ./execsnoop -t # include timestamps\n"
" ./execsnoop -q # add \"quotemarks\" around arguments\n"
" ./execsnoop -Q # add \"quotemarks\" around arguments\n"
" ./execsnoop -n main # only print command lines containing \"main\"\n"
" ./execsnoop -l tpkg # only print command where arguments contains \"tpkg\"";

static const struct argp_option opts[] = {
{ "time", 'T', NULL, 0, "include time column on output (HH:MM:SS)"},
{ "timestamp", 't', NULL, 0, "include timestamp on output"},
{ "fails", 'x', NULL, 0, "include failed exec()s"},
{ "fails", 'X', NULL, 0, "include failed exec()s"},
{ "uid", 'u', "UID", 0, "trace this UID only"},
{ "quote", 'q', NULL, 0, "Add quotemarks (\") around arguments"},
{ "quote", 'Q', NULL, 0, "Add quotemarks (\") around arguments"},
{ "name", 'n', "NAME", 0, "only print commands matching this name, any arg"},
{ "line", 'l', "LINE", 0, "only print commands where arg contains this line"},
{ "print-uid", 'U', NULL, 0, "print UID column"},
Expand All @@ -83,7 +83,7 @@ static error_t parse_arg(int key, char *arg, struct argp_state *state)
case 't':
env.timestamp = true;
break;
case 'x':
case 'X':
env.fails = true;
break;
case 'u':
Expand All @@ -95,7 +95,7 @@ static error_t parse_arg(int key, char *arg, struct argp_state *state)
}
env.uid = uid;
break;
case 'q':
case 'Q':
env.quote = true;
break;
case 'n':
Expand Down Expand Up @@ -271,7 +271,7 @@ int main(int argc, char **argv)

obj = execsnoop_bpf__open();
if (!obj) {
fprintf(stderr, "failed to open and/or load BPF object\n");
fprintf(stderr, "failed to open BPF object\n");
return 1;
}

Expand Down
2 changes: 1 addition & 1 deletion libbpf-tools/filelife.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,7 @@ int main(int argc, char **argv)

obj = filelife_bpf__open();
if (!obj) {
fprintf(stderr, "failed to open and/or load BPF object\n");
fprintf(stderr, "failed to open BPF object\n");
return 1;
}

Expand Down
10 changes: 5 additions & 5 deletions libbpf-tools/hardirqs.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,17 +36,17 @@ const char *argp_program_bug_address = "<bpf@vger.kernel.org>";
const char argp_program_doc[] =
"Summarize hard irq event time as histograms.\n"
"\n"
"USAGE: hardirqs [--help] [-T] [-N] [-d] [interval] [count]\n"
"USAGE: hardirqs [--help] [-T] [-N] [-D] [interval] [count]\n"
"\n"
"EXAMPLES:\n"
" hardirqs # sum hard irq event time\n"
" hardirqs -d # show hard irq event time as histograms\n"
" hardirqs -D # show hard irq event time as histograms\n"
" hardirqs 1 10 # print 1 second summaries, 10 times\n"
" hardirqs -NT 1 # 1s summaries, nanoseconds, and timestamps\n";

static const struct argp_option opts[] = {
{ "count", 'C', NULL, 0, "Show event counts instead of timing" },
{ "distributed", 'd', NULL, 0, "Show distributions as histograms" },
{ "distributed", 'D', NULL, 0, "Show distributions as histograms" },
{ "timestamp", 'T', NULL, 0, "Include timestamp on output" },
{ "nanoseconds", 'N', NULL, 0, "Output in nanoseconds" },
{ "verbose", 'v', NULL, 0, "Verbose debug output" },
Expand All @@ -61,7 +61,7 @@ static error_t parse_arg(int key, char *arg, struct argp_state *state)
case 'v':
env.verbose = true;
break;
case 'd':
case 'D':
env.distributed = true;
break;
case 'C':
Expand Down Expand Up @@ -191,7 +191,7 @@ int main(int argc, char **argv)

obj = hardirqs_bpf__open();
if (!obj) {
fprintf(stderr, "failed to open and/or load BPF object\n");
fprintf(stderr, "failed to open BPF object\n");
return 1;
}

Expand Down
2 changes: 1 addition & 1 deletion libbpf-tools/llcstat.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -187,7 +187,7 @@ int main(int argc, char **argv)

obj = llcstat_bpf__open();
if (!obj) {
fprintf(stderr, "failed to open and/or load BPF object\n");
fprintf(stderr, "failed to open BPF object\n");
return 1;
}

Expand Down
18 changes: 9 additions & 9 deletions libbpf-tools/opensnoop.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,24 +48,24 @@ const char *argp_program_bug_address = "<bpf@vger.kernel.org>";
const char argp_program_doc[] =
"Trace open family syscalls\n"
"\n"
"USAGE: opensnoop [-h] [-T] [-U] [-x] [-p PID] [-t TID] [-u UID] [-d DURATION]\n"
" [-n NAME] [-e]\n"
"USAGE: opensnoop [-h] [-T] [-U] [-X] [-p PID] [-t TID] [-u UID] [-d DURATION]\n"
" [-n NAME] [-E]\n"
"\n"
"EXAMPLES:\n"
" ./opensnoop # trace all open() syscalls\n"
" ./opensnoop -T # include timestamps\n"
" ./opensnoop -U # include UID\n"
" ./opensnoop -x # only show failed opens\n"
" ./opensnoop -X # only show failed opens\n"
" ./opensnoop -p 181 # only trace PID 181\n"
" ./opensnoop -t 123 # only trace TID 123\n"
" ./opensnoop -u 1000 # only trace UID 1000\n"
" ./opensnoop -d 10 # trace for 10 seconds only\n"
" ./opensnoop -n main # only print process names containing \"main\"\n"
" ./opensnoop -e # show extended fields\n";
" ./opensnoop -E # show extended fields\n";

static const struct argp_option opts[] = {
{ "duration", 'd', "DURATION", 0, "Duration to trace"},
{ "extended-fields", 'e', NULL, 0, "Print extended fields"},
{ "extended-fields", 'E', NULL, 0, "Print extended fields"},
{ NULL, 'h', NULL, OPTION_HIDDEN, "Show the full help"},
{ "name", 'n', "NAME", 0, "Trace process names containing this"},
{ "pid", 'p', "PID", 0, "Process ID to trace"},
Expand All @@ -74,7 +74,7 @@ static const struct argp_option opts[] = {
{ "uid", 'u', "UID", 0, "User ID to trace"},
{ "print-uid", 'U', NULL, 0, "Print UID"},
{ "verbose", 'v', NULL, 0, "Verbose debug output" },
{ "failed", 'x', NULL, 0, "Failed opens only"},
{ "failed", 'X', NULL, 0, "Failed opens only"},
{},
};

Expand All @@ -84,7 +84,7 @@ static error_t parse_arg(int key, char *arg, struct argp_state *state)
long int pid, uid, duration;

switch (key) {
case 'e':
case 'E':
env.extended = true;
break;
case 'h':
Expand All @@ -99,7 +99,7 @@ static error_t parse_arg(int key, char *arg, struct argp_state *state)
case 'v':
env.verbose = true;
break;
case 'x':
case 'X':
env.failed = true;
break;
case 'd':
Expand Down Expand Up @@ -231,7 +231,7 @@ int main(int argc, char **argv)

obj = opensnoop_bpf__open();
if (!obj) {
fprintf(stderr, "failed to open and/or load BPF object\n");
fprintf(stderr, "failed to open BPF object\n");
return 1;
}

Expand Down
Loading