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

[Junio C Hamano <gitster@pobox.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.

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

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

> +		/* 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;
> +}

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

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

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