Re: [PATCH v6 2/2] tr2: log parent process name
From: Junio C Hamano <hidden>
Date: 2021-07-22 21:02:41
Emily Shaffer [off-list ref] writes:
Unfortunately, there's no cross-platform reliable way to gather the name of the parent process. If procfs is present, we can use that; otherwise we will need to discover the name another way. However, the process ID should be sufficient to look up the process name on most platforms, so that code may be shareable.
Is the sentence that begin with "However" still relevant? The latest get_ancestry_names() does not add anything when the result read from the procfs is unusable, but the sentence gives a false impression that it may somehow fall back to show the PID.
Git for Windows also gathers information about more than one generation of parent. In Linux further ancestry info can be gathered with procfs, but it's unwieldy to do so. In the interest of later moving Git for Windows ancestry logging to the 'cmd_ancestry' event, and in the interest of later adding more ancestry to the Linux implementation - or of adding this functionality to other platforms which have an easier time walking the process tree - let's make 'cmd_ancestry' accept an array of parentage.
Clearly written. Nice.
+`"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"]
+}
+------------OK.
quoted hunk
diff --git a/compat/linux/procinfo.c b/compat/linux/procinfo.c new file mode 100644 index 0000000000..578fed4cd3 --- /dev/null +++ b/compat/linux/procinfo.c@@ -0,0 +1,55 @@ +#include "cache.h" + +#include "strbuf.h" +#include "strvec.h" +#include "trace2.h" + +static void get_ancestry_names(struct strvec *names) +{ + /* + * 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));
At this point, we successfully read from /proc/$(ppid)/comm and pushed the result into names strvec. I think you would want to have an explicit "return;" here. Alternatively, you could put the rest of the function in the corresponding "else" clause, but I think an early return after each method successfully collects the result would make a lot more sense.
+ } + + return; + /* NEEDSWORK: add non-procfs-linux implementations here */
This looks the other way around. A future non-procfs-linux
implementation written here will be unreachable code ;-)
Just lose "return;" from here, have one in the if(){} body, and keep
the "here is where you add more/other implementations" comment and
we'd be OK, I think.
+void trace2_collect_process_info(enum trace2_process_info_reason reason)
+{
+ if (!trace2_is_enabled())
+ return;
+
+ /* someday we may want to write something extra here, but not today */
+ if (reason == TRACE2_PROCESS_INFO_EXIT)
+ return;
+
+ if (reason == TRACE2_PROCESS_INFO_STARTUP) {
+ /*
+ * NEEDSWORK: we could do the entire ptree in an array instead,
+ * see compat/win32/trace2_win32_process_info.c.
+ */
+ struct strvec names = STRVEC_INIT;
+
+ get_ancestry_names(&names);
+
+ if (names.nr)
+ trace2_cmd_ancestry(names.v);
+ strvec_clear(&names);
+ }
+
+ return;
+}Good. Thanks.