Messages in this thread | | | From | Namhyung Kim <> | Date | Tue, 1 Feb 2022 11:54:14 -0800 | Subject | Re: [PATCH 3/3] perf annotate: Try chroot filename for objdump |
| |
Hi Jiri,
On Mon, Jan 31, 2022 at 11:23 AM Jiri Olsa <jolsa@redhat.com> wrote: > > On Fri, Jan 28, 2022 at 12:39:50PM -0800, Namhyung Kim wrote: > > Likewise, it should use a proper name in case the task runs under > > chroot. The child_process.err was needed to set to -1 to show error > > messages properly in TUI. > > > > Signed-off-by: Namhyung Kim <namhyung@kernel.org> > > --- > > tools/perf/util/annotate.c | 11 +++++++++++ > > 1 file changed, 11 insertions(+) > > > > diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c > > index 01900689dc00..e4c641b240df 100644 > > --- a/tools/perf/util/annotate.c > > +++ b/tools/perf/util/annotate.c > > @@ -33,6 +33,7 @@ > > #include "string2.h" > > #include "util/event.h" > > #include "arch/common.h" > > +#include "namespaces.h" > > #include <regex.h> > > #include <pthread.h> > > #include <linux/bitops.h> > > @@ -1696,6 +1697,15 @@ static int dso__disassemble_filename(struct dso *dso, char *filename, size_t fil > > * DSO is the same as when 'perf record' ran. > > */ > > __symbol__join_symfs(filename, filename_size, dso->long_name); > > + > > + if (access(filename, R_OK) && errno == ENOENT && dso->nsinfo) { > > + char *new_name = filename_with_chroot(dso->nsinfo->pid, > > + filename); > > + if (new_name) { > > + strlcpy(filename, new_name, filename_size); > > + free(new_name); > > + } > > + } > > } > > > > free(build_id_path); > > @@ -2036,6 +2046,7 @@ static int symbol__disassemble(struct symbol *sym, struct annotate_args *args) > > memset(&objdump_process, 0, sizeof(objdump_process)); > > objdump_process.argv = objdump_argv; > > objdump_process.out = -1; > > + objdump_process.err = -1; > > is this unrelated fix?
Right, it's unrelated. Maybe I can split it if Arnaldo wants.
> > otherwise the whole patchset looks good to me > > I guess we'd need to add something like PERF_RECORD_CHROOT to > handle and instrument sys_chroot to handle this completely?
Are you talking about an env variable? Maybe we can track chroot syscall. But it needs to be task-specific rather than a global setup.
As it hasn't been an issue so far, I think it's ok to have it as a fallback right now.
> > Acked-by: Jiri Olsa <jolsa@kernel.org>
Thanks for your review! Namhyung
> > > if (start_command(&objdump_process)) { > > pr_err("Failure starting to run %s\n", command); > > err = -1; > > -- > > 2.35.0.rc2.247.g8bbb082509-goog > > >
| |