bpftrace / bpftrace

High-level tracing language for Linux
Apache License 2.0
8.62k stars 1.35k forks source link

Can't list probes used in script file #3058

Closed ajor closed 6 months ago

ajor commented 8 months ago

As of #2969, it's possible to list probes used in a script by combining -e and -l:

$ sudo bpftrace -l -e 't:syscalls:sys_enter_ope* {}'
tracepoint:syscalls:sys_enter_open
tracepoint:syscalls:sys_enter_open_by_handle_at
tracepoint:syscalls:sys_enter_open_tree
tracepoint:syscalls:sys_enter_openat
tracepoint:syscalls:sys_enter_openat2

However, this doesn't work if the script is contained in a file instead of written directly on the command line:

$ sudo bpftrace -l test.bt                                              
stdin:1:1-8: ERROR: Invalid probe type: test.bt
test.bt
~~~~~~~

This should just a matter of fixing bpftrace's command line argument parsing logic.

jordalgo commented 8 months ago

Good catch. If no one picks it up as a starter task, I'll take it.

ikruglov commented 7 months ago

@jordalgo I'm wondering if this is a correct fix: https://github.com/ikruglov/bpftrace/commit/65626928a74055a8aab8ad7beb4d3253634a68c0

ikruglov commented 7 months ago

I'm looking into --help and doubt that my commit will do the trick. There is a conflict, it seems:

USAGE:
    bpftrace [options] filename
    ...

OPTIONS:
    ...
    -l [search]    list kernel probes or probes in a program

Based on the snippet above, a command bpftrace -l foo is ambiguous. Because, foo can be either search pattern or a file name.

I guess we can resolve this via the following logic:

  1. assume that foo is a file.
  2. check the file existence. If yes, proceed accordingly
  3. if not, treat foo as search command.

This approach is not ideal. A missing file would be executed as a search.

Is there any disambiguity technic which I can use?

jordalgo commented 7 months ago

@ikruglov Good point about the ambiguity.

Another option would be to remove the positional search param for listing probes and just let people use grep. Or change search from being a positional param to a named one e.g. --search. @ajor Any thoughts here?

ajor commented 7 months ago

The -l search option is more powerful than a grep as it supports globbing, C++ name mangling and argument listing (with -v), so we shouldn't get rid of it.

Reworking the arguments is an option, but I'd try to avoid as much as possible so that we don't invalidate older tutorials around the web.

  1. assume that foo is a file.
  2. check the file existence. If yes, proceed accordingly
  3. if not, treat foo as search command.

It's not ideal, but doesn't seem too unreasonable to me.

Is it a big deal if a missing file gets treated as a search pattern? Confusion for the user because of strange error messages? e.g. "Invalid probe type: test.bt" instead of "file not found: test.bt". Maybe we could do something to alleviate this like also checking for colons in the pattern as a heuristic for whether its a file name or a probe identifier.

ikruglov commented 7 months ago

Is it a big deal if a missing file gets treated as a search pattern? Confusion for the user because of strange error messages? e.g. "Invalid probe type: test.bt" instead of "file not found: test.bt". Maybe we could do something to alleviate this like also checking for colons in the pattern as a heuristic for whether its a file name or a probe identifier.

We can do something similar to curl, for instance:

$ bpfilter -l @foo # file
$ bpfilter -l foo # search pattern

from man curl

       --data-binary <data>
              (HTTP) This posts data exactly as specified with no extra processing whatsoever.

              If you start the data with the letter @, the rest should be a filename.

@ajor @jordalgo WDYT?

jordalgo commented 7 months ago

I think out of all these options. I liked your first idea @ikruglov :

assume that foo is a file. check the file existence. If yes, proceed accordingly if not, treat foo as search command.

As @ajor mentioned we can also do things like check for colons or even the ".bt" extension. Maybe even add an additional name param --file that can be used to disambiguate.

ikruglov commented 7 months ago

@jordalgo @ajor I've updated https://github.com/ikruglov/bpftrace/commit/8f335d412cd1b5072bc0e477db722306bbec8deb. Can you please check if this is the logic which you would like to see. If yes, I'll polish it and prepare a proper PR.

ajor commented 7 months ago

Thanks @ikruglov! Yeah that logic looks right - I think we can get away with simplifying it a bit, but if you open it up as a PR we can discuss it over there

ikruglov commented 7 months ago

@ajor, here is PR: https://github.com/bpftrace/bpftrace/pull/3111. What do you want to change there?

ikruglov commented 6 months ago

this issue should be closed cause https://github.com/bpftrace/bpftrace/pull/3111 is merged