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.

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

record: Find the execution binary in PATH #741

Merged
merged 1 commit into from
May 13, 2019

Conversation

honggyukim
Copy link
Collaborator

@honggyukim honggyukim commented May 3, 2019

Currently uftrace explicitly requires to have a correct pathname, so it
has to be used with full pathname as follows:

$ uftrace --force /usr/bin/python -c ''

This patch allows the user can provide the command name, which is placed
one of directories in PATH environment.
So uftrace can be simply used as follows:

$ uftrace --force python -c ''

It'd be helpful especially when tracing with -P option and it will make
easier to use full dynamic tracing feature as follows:

$ uftrace -P. -a python -c 'print("Hi, Python")'

Closes: #474

Signed-off-by: Honggyu Kim [email protected]

@honggyukim
Copy link
Collaborator Author

honggyukim commented May 3, 2019

Hi @namhyung, I think we better find the executable file in PATH environment for easier usage.

$ uftrace -P. -a echo "Hello"
Hello
# DURATION     TID     FUNCTION
 321.932 us [177318] | getenv("POSIXLY_CORRECT") = "NULL";
   1.644 us [177318] | strrchr("echo", '/') = "NULL";
  62.873 us [177318] | setlocale(LC_ALL, "") = "en_US.UTF-8";
   7.377 us [177318] | bindtextdomain("coreutils", "/usr/share/locale");
   1.876 us [177318] | textdomain("coreutils");
   1.037 us [177318] | __cxa_atexit();
   1.454 us [177318] | strcmp("Hello", "--help") = 27;
   0.523 us [177318] | strcmp("Hello", "--version") = 27;
   3.773 us [177318] | fputs_unlocked();
  12.184 us [177318] | __overflow();
   1.457 us [177318] | __fpending();
   1.280 us [177318] | fileno(&_IO_2_1_stdout_) = 1;
   0.803 us [177318] | __freading();
   0.190 us [177318] | __freading();
   1.104 us [177318] | fflush(&_IO_2_1_stdout_) = 0;
   2.580 us [177318] | fclose(&_IO_2_1_stdout_) = 0;
   0.184 us [177318] | __fpending();
   0.307 us [177318] | fileno(&_IO_2_1_stderr_) = 2;
   0.163 us [177318] | __freading();
   0.146 us [177318] | __freading();
   0.376 us [177318] | fflush(&_IO_2_1_stderr_) = 0;
   1.230 us [177318] | fclose(&_IO_2_1_stderr_) = 0;

I thought that giving a full pathname or using which command doesn't looks nice for general users.
It can be used just like any other tracing tools such as strace, ltrace, etc. Please let me know how you think about it.

@honggyukim honggyukim requested a review from namhyung May 3, 2019 23:44
@honggyukim honggyukim force-pushed the find-exe-path branch 2 times, most recently from 4f81f84 to 5a24e7f Compare May 6, 2019 11:27
cmds/record.c Outdated

/* search opts->exename in PATH one by one */
p = strchr(path, ':');
while (p) {
Copy link
Owner

Choose a reason for hiding this comment

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

You'd better using strv for this kind of string handling.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Applied.

cmds/record.c Outdated
@@ -1510,14 +1553,11 @@ static void check_binary(struct opts *opts)
};

again:
pr_dbg3("checking binary %s\n", opts->exename);
pr_dbg("checking binary %s\n", opts->exename);
Copy link
Owner

Choose a reason for hiding this comment

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

What about moving this under the find_in_path()?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If I move this message inside find_in_path, then it will not print anything when the user gives the full pathname. The first is_regular_executable check will be successful so doesn't call find_in_path in this case.

Copy link
Owner

Choose a reason for hiding this comment

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

Oh, I meant the callsite - as find_in_path() can change the exename, it'd be nice to print the final name IMO.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I see. I think it's enough to place after the searching the binary as I updated. Thanks.

@honggyukim
Copy link
Collaborator Author

@namhyung I just updated it again based on your feedback. Thanks!

Currently uftrace explicitly requires to have a correct pathname, so it
has to be used with full pathname as follows:

  $ uftrace --force /usr/bin/python -c ''

This patch allows the user can provide the command name, which is placed
one of directories in PATH environment.
So uftrace can be simply used as follows:

  $ uftrace --force python -c ''

It'd be helpful especially when tracing with -P option and it will make
easier to use full dynamic tracing feature as follows:

  $ uftrace -P. -a python -c 'print("Hi, Python")'

Closes: namhyung#474

Signed-off-by: Honggyu Kim <[email protected]>
Copy link
Owner

@namhyung namhyung left a comment

Choose a reason for hiding this comment

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

LGTM

@namhyung namhyung merged commit fc75894 into namhyung:master May 13, 2019
@honggyukim honggyukim deleted the find-exe-path branch May 13, 2019 04:46
@honggyukim
Copy link
Collaborator Author

Thanks for merging this. Let's modify documents that includes the absolute path and which command. I mean let's keep it mind before making a release, not right now. :)

@namhyung
Copy link
Owner

You can file an issue for that. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Finding target binary in $PATH
2 participants