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);