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

From: Junio C Hamano <gitster@pobox.com>
Date: May 21, 2021, 02:09 UTC
Message-ID: <xmqqpmxksuqa.fsf@gitster.g>
In-Reply-To: <20210520210546.4129620-1-emilyshaffer@google.com>

Emily Shaffer <emilyshaffer@google.com> 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 regardless of platform.

Not a strong objection, but I wonder if seeing random integer(s) is better than not having cmd_ancestry info at all. The latter better signals that the platform does not yet have the "parent process name" feature, I would think.

Show 8 quoted lines

> Git for Windows also gathers information about more than one 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.

Could we rephrase "more than one parent" at the beginning to clarify? I initially had to wonder what "an array of parentage" contains (father and mother, or a sole parent and its sole parent, which is a sole grandparent). Since there is no "multiple processes meet and spawn a single process", I take it is the latter. Perhaps "more than one generation of" or something?

Show 28 quoted lines

> +ifdef HAVE_PROCFS_LINUX
> +	BASIC_CFLAGS += -DHAVE_PROCFS_LINUX
> +	COMPAT_OBJS += compat/procinfo.o
> +endif
> +
>  ifdef HAVE_NS_GET_EXECUTABLE_PATH
>  	BASIC_CFLAGS += -DHAVE_NS_GET_EXECUTABLE_PATH
>  endif
> diff --git a/compat/procinfo.c b/compat/procinfo.c
> new file mode 100644
> index 0000000000..523600673f
> --- /dev/null
> +++ b/compat/procinfo.c
> @@ -0,0 +1,53 @@
> +#include "cache.h"
> +
> +#include "strbuf.h"
> +#include "trace2.h"
> +
> +char *get_process_name(int pid)
> +{
> +#ifdef HAVE_PROCFS_LINUX
> +	struct strbuf procfs_path = STRBUF_INIT;
> +	struct strbuf out = STRBUF_INIT;
> +	/* try to use procfs if it's present. */
> +	strbuf_addf(&procfs_path, "/proc/%d/comm", pid);
> +	if (!strbuf_read_file(&out, procfs_path.buf, 0))
> +	{

Place this opening brace at the end of the previous line.

Show 9 quoted lines

> +		/* All done with file reads, clean up early */
> +		strbuf_release(&procfs_path);
> +		return strbuf_detach(&out, NULL);
> +	}
> +#endif
> +
> +	/* NEEDSWORK: add non-procfs implementations here. */
> +	return NULL;
> +}

Show 11 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)
> +	{

Ditto.

Show 10 quoted lines

> +		/*
> +		 * NEEDSWORK: we could do the entire ptree in an array instead,
> +		 * see compat/win32/trace2_win32_process_info.c.
> +		 */
> +		char *names[2];
> +		names[0] = get_process_name(getppid());
> +		names[1] = NULL;
> +
> +		if (!names[0])
> +			return;

OK, so if there is no name given, we do not show pid as a placeholder.

Show 16 quoted lines

> +		trace2_cmd_ancestry((const char**)names);
> +
> +		free(names[0]);
> +	}
> +
> +	return;
> +}
> diff --git a/config.mak.uname b/config.mak.uname
> index cb443b4e02..7ad110a1d2 100644
> --- a/config.mak.uname
> +++ b/config.mak.uname
> @@ -58,6 +58,7 @@ ifeq ($(uname_S),Linux)
>  	FREAD_READS_DIRECTORIES = UnfortunatelyYes
>  	BASIC_CFLAGS += -DHAVE_SYSINFO
>  	PROCFS_EXECUTABLE_PATH = /proc/self/exe
> +	HAVE_PROCFS_LINUX = YesPlease

Have all Linux instances procfs enabled and mounted? It might be that we need to detect this at runtime anyway?

    ... goes and thinks ...

Ah, OK, that "try reading from proc/%d/comm" is the runtime detection, so it is only this Makefile variable is slightly misnamed (it is not "HAVE" but "is worth checking for it").

Makes sense.