On Sat, 14 Jan 2023 11:58:41 +1300
Paulo Miguel Almeida <paulo.miguel.almeida.rodenas@xxxxxxxxx> wrote:
Hi Paulo,
A couple of nits about submitting a follow up patch.
1) A second patch should always start a new thread. It's easier to find
in inboxes.
If you want, you could add a link to the first thread in the "changes"
section (see below).
2) Please start the subject with a capital letter:
[PATCH v2] trace-cmd: Open code execvp routine to avoid multiple execve syscalls
> In tracecmd/trace-record.c:<run_cmd>, trace-cmd record -F <executable>
> is launched via the libc's execvp() routine. The way that execvp() routine
> works is by invoking execve syscall for every entry on the $PATH if
> command specified is neither absolute nor relative which can come across
> as a bit cryptic to untrained eyes.
>
> - absolute path example:
>
> # trace-cmd record -p function_graph \
> -g __x64_sys_execve -O nofuncgraph-irqs \
> -n __cond_resched --max-graph-depth 1 \
> -F /usr/bin/echo "ftrace" > /dev/null
>
> # trace-cmd report
> echo-172994 [000] 185539.798539: funcgraph_entry: ! 803.376 us | __x64_sys_execve();
>
> - PATH-dependent path example:
>
> # trace-cmd record -p function_graph \
> -g __x64_sys_execve -O nofuncgraph-irqs \
> -n __cond_resched --max-graph-depth 1 \
> -F echo "ftrace" > /dev/null
>
> # trace-cmd report
> echo-172656 [002] 185009.671586: funcgraph_entry: ! 288.732 us | __x64_sys_execve();
> echo-172656 [002] 185009.671879: funcgraph_entry: ! 158.337 us | __x64_sys_execve();
> echo-172656 [002] 185009.672042: funcgraph_entry: ! 161.843 us | __x64_sys_execve();
> echo-172656 [002] 185009.672207: funcgraph_entry: ! 157.656 us | __x64_sys_execve();
> echo-172656 [002] 185009.672369: funcgraph_entry: ! 156.343 us | __x64_sys_execve();
> echo-172656 [002] 185009.672529: funcgraph_entry: ! 863.629 us | __x64_sys_execve();
>
> Open code the libc's execvp routine into trace-cmd so ftrace will only
> start recording once the command is found when it needs to be found in
> PATH.
>
> Signed-off-by: Paulo Miguel Almeida <paulo.miguel.almeida.rodenas@xxxxxxxxx>
> ---
> Changelog:
>
> - v2: open code execvp routine into trace-cmd. (Req. Steve Rostedt)
> - v1: https://lore.kernel.org/linux-trace-devel/Y7dUo6woh9Y31cdl@xxxxxxxxxxxxxxx/
> ---
> tracecmd/trace-record.c | 59 ++++++++++++++++++++++++++++++++++++-----
> 1 file changed, 53 insertions(+), 6 deletions(-)
>
> diff --git a/tracecmd/trace-record.c b/tracecmd/trace-record.c
> index 7f0cebe..4a54637 100644
> --- a/tracecmd/trace-record.c
> +++ b/tracecmd/trace-record.c
> @@ -1683,6 +1683,58 @@ static int change_user(const char *user)
> return 0;
> }
>
> +static void execute_program(int argc, char **argv)
> +{
> + char buf[PATH_MAX + NAME_MAX + 1];
> + char *path_env;
> + size_t path_len;
> + size_t entry_len;
> + char *ptr_start;
> + char *ptr_end;
> +
> + /*
> + * if command specified by user is neither absolute nor
> + * relative than we search for it in $PATH.
> + */
> + if (!strchr(argv[0], '/') && !strchr(argv[0], '.')) {
Why the search of '.'? If you have an executable called:
my.exec
Wouldn't that be found?
Can you have a relative path without '/'? Usually, you would do:
./exec
> + path_env = getenv("PATH");
Need to check for NULL, in the rare case that no "PATH" is defined.
> + path_len = strlen(path_env);
> + ptr_start = path_env;
> +
> + while ((ptr_start - path_env) < path_len) {
> + ptr_end = strchr(ptr_start, ':');
Why not just use strtok_r() here?
Something like (untested):
char *saveptr;
for (path = strtok_r(path_env, ":", &saveptr);
path; path = strtok_r(NULL, ":", &saveptr) {
snprintf(buf, PATH_MAX, "%s/%s", path, argv[0]);
if (access(buf, X_OK) == 0)
break;
}
> +
> + /* single entry in PATH? */
> + if (!ptr_end)
> + entry_len = path_len;
> + else
> + entry_len = ptr_end - ptr_start;
> +
> + strncpy(buf, ptr_start, entry_len);
> +
> + if (buf[entry_len - 1] != '/')
> + buf[entry_len++] = '/';
> +
> + strncpy(buf + entry_len, argv[0], sizeof(buf) - entry_len);
> +
> + /* does it exist and can we execute it? */
> + if (access(buf, X_OK) == 0)
> + break;
> +
> + ptr_start = ptr_end + 1;
> + }
> + } else {
> + strncpy(buf, argv[0], sizeof(buf));
> + }
Don't we want to enable tracing here?
-- Steve
> +
> + if (execve(buf, argv, environ)) {
> + fprintf(stderr, "\n********************\n");
> + fprintf(stderr, " Unable to exec %s\n", argv[0]);
> + fprintf(stderr, "********************\n");
> + die("Failed to exec %s", argv[0]);
> + }
> +}
> +
> static void run_cmd(enum trace_type type, const char *user, int argc, char **argv)
> {
> int status;
> @@ -1709,12 +1761,7 @@ static void run_cmd(enum trace_type type, const char *user, int argc, char **arg
> if (change_user(user) < 0)
> die("Failed to change user to %s", user);
>
> - if (execvp(argv[0], argv)) {
> - fprintf(stderr, "\n********************\n");
> - fprintf(stderr, " Unable to exec %s\n", argv[0]);
> - fprintf(stderr, "********************\n");
> - die("Failed to exec %s", argv[0]);
> - }
> + execute_program(argc, argv);
> }
> if (fork_process)
> exit(0);
|