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

On Mon, Jun 28, 2021 at 12:45:24PM -0400, Jeff Hostetler wrote:

On 6/8/21 6:10 PM, Emily Shaffer wrote:

quoted

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.

quoted

  ------------
+`"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.

quoted

+}

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.

quoted

+#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.

quoted

+
+		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