On Mon, Jun 28, 2021 at 12:45:24PM -0400, Jeff Hostetler wrote:
> On 6/8/21 6:10 PM, Emily Shaffer wrote:
> > Range-diff against v4:
> > 1: efb0a3ccb4 ! 1: 7a7e1ebbfa tr2: log parent process name
> > @@ compat/procinfo.c (new)
> > + strbuf_addf(&procfs_path, "/proc/%d/comm", getppid());
> > + if (strbuf_read_file(&name, procfs_path.buf, 0)) {
> > + strbuf_release(&procfs_path);
> > ++ strbuf_trim_trailing_newline(&name);
> > + strvec_push(names, strbuf_detach(&name, NULL));
> > + }
> > +
>
> You're only getting the name of the command (argv[0]) and not the
> full command line, right? That is a good thing.
Roughly. The name can be reset by the process itself (that's what
happened, I guess, in the tmux case I pasted below) but by default it's
argv[0]. It's also truncated to 15ch or something.
> > ------------
> > +`"cmd_ancestry"`::
> > + This event contains the text command name for the parent (and earlier
> > + generations of parents) of the current process, in an array ordered from
> > + nearest parent to furthest great-grandparent. It may not be implemented
> > + on all platforms.
> > ++
> > +------------
> > +{
> > + "event":"cmd_ancestry",
> > + ...
> > + "ancestry":["bash","tmux: server","systemd"]
>
> Is the second element really "tmux: server". Seems odd that that's what
> the command name (argv[0]) is. Perhaps I misread something??
See above. This is what shows up in pstree, though, and by poking around in
/proc I confirmed that this is indeed the content of /proc/<tmux-pid>/comm:
├─tmux: server─┬─bash───mutt───open-vim-in-new───vim
│ ├─bash───pstree
│ └─mutt
This is a somewhat contrived example, though, because in Linux as of this patch,
only one ancestor is gathered. So maybe I had better make the doc
reflect what's actually possible. I'm planning on sending a follow-on
sometime soon exposing more generations of ancestry, so I guess I could
update the docs back to this state around then.
>
> > +}
>
> This array is bounded and that implies that you captured all of
> the grand parents back to "init" (or whatever it is called these
> days).
In this case it does - pid 1 is systemd, which hasn't got a parent
process.
> Is there value in having a final "..." or "(truncated)" element
> to indicate that the list incomplete? I did the latter in the
> Windows version.
Hrm. I'm not the one who wants to parse these - it's someone else who's
working with our team internally - so I'll ask around and see what they
think is best.
> > +#ifdef HAVE_PROCFS_LINUX
> > + /*
> > + * NEEDSWORK: We could gather the entire pstree into an array to match
> > + * functionality with compat/win32/trace2_win32_process_info.c.
> > + * To do so, we may want to examine /proc/<pid>/stat. For now, just
> > + * gather the immediate parent name which is readily accessible from
> > + * /proc/$(getppid())/comm.
> > + */
> > + struct strbuf procfs_path = STRBUF_INIT;
> > + struct strbuf name = STRBUF_INIT;
> > +
> > + /* try to use procfs if it's present. */
> > + strbuf_addf(&procfs_path, "/proc/%d/comm", getppid());
> > + if (strbuf_read_file(&name, procfs_path.buf, 0)) {
> > + strbuf_release(&procfs_path);
> > + strbuf_trim_trailing_newline(&name);
> > + strvec_push(names, strbuf_detach(&name, NULL));
> > + }
> > +
> > + return;
> > +#endif
> > + /* NEEDSWORK: add non-procfs-linux implementations here */
> > +}
>
> Perhaps this has already been discussed, but would it be better
> to have a "compat/linux/trace2_linux_process_info.c"
> or "compat/procfs/trace2_procfs_process_info.c" source file and
> only compile it in Linux-compatible builds -- rather than #ifdef'ing
> the source. This is a highly platform-specific feature.
>
> For example, if I convert the Win32 version to use your new event,
> I wouldn't want to move the code.
>
> I just noticed that you have both "BASIC_CFLAGS+=" and a "COMPAT_OBSJ+="
> lines. If you made this source file procfs-specific, you wouldn't need
> the ifdef and you could avoid the new CFLAG.
Sure, I'll investigate it, thanks.
> > +
> > + if (names.nr == 0) {
> > + strvec_clear(&names);
> > + return;
> > + }
> > +
> > + trace2_cmd_ancestry(names.v);
> > +
> > + strvec_clear(&names);
>
> I agree with Junio here, it would be simpler to say it like this:
>
> get_ancestry_names(&names);
> if (names.nr)
> trace2_cmd_ancestry(names.v);
> strvec_clear(&names);
>
Thanks both, done locally.
> Otherwise, this looks good to me.
Thanks. Look for a v6 from me this week, hopefully with the build stuff
sorted out.
- Emily
