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

On Wed, Jun 30, 2021 at 08:10:59AM +0200, Ævar Arnfjörð Bjarmason wrote:

On Tue, Jun 29 2021, Emily Shaffer wrote:

quoted

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

quoted

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.

16 including the \0. See prctl(2). Linux has two different ways to
set/get the name, one is the argv method, the other is
prctl(PR_SET_NAME). They don't need to match at all. The ps(1) utility
and some top-like utilities allow you to switch between viewing the two
versions.

As noted in the linked manual pages you'll also potentially need to deal
with multithreaded programs having different names for each thread.

I don't think we use this now, but FWIW one thing I've wanted to do for
a while was to have the progress.c code update this, so you see if git's
at N% counting objects or whatever in top.

quoted

quoted

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.

In general we've preferred not using ifdefs at all except for the small
bits that absolutely need it.

So e.g. in this case the whole code should compile on non-Linux, we just
need a small boolean guard somewhere to check what the platform is.

It means we don't have significant pieces of code that don't compile
except on platform X. It's easy to get into your code not compiling if
you overuse ifdefs.

Hmm. I see what you mean.

However, since win32 is already using some conditionally compiling code,
that's the approach I went for. It's still running CI to make sure I
didn't break Windows in the process, but I'll post it (and the Actions
runs) later today, assuming it passed.

Yes, the implementation I wrote would compile on any platform, but as I
understand it, other platforms may need drastically different
implementations which may not compile as easily as this. So, I'm not
sure if it's really appropriate to do run-time platform checks here
(which is what I think you were describing).

Anyway, I'll look forward to seeing what you folks think of the next
iteration (again, hopefully later today).

 - Emily