Re: [PATCH v6 2/2] tr2: log parent process name

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 22, 2021, 21:02 UTC
Message-ID: <xmqqwnpi83kh.fsf@gitster.g>
In-Reply-To: <20210722012707.205776-3-emilyshaffer@google.com>

Emily Shaffer <emilyshaffer@google.com> writes:

Show 5 quoted lines

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

Show 8 quoted lines

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

Show 13 quoted lines

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

Show 30 quoted lines

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

Show 25 quoted lines

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