Re: [PATCH v5] tr2: log parent process name

[Emily Shaffer <emilyshaffer@google.com>]

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