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

From: Emily Shaffer <emilyshaffer@google.com>
Date: May 21, 2021, 19:02 UTC
Message-ID: <YKgDxahhwK/zYznH@google.com>
In-Reply-To: <xmqqpmxksuqa.fsf@gitster.g>

On Fri, May 21, 2021 at 11:09:49AM +0900, Junio C Hamano wrote:

Show 12 quoted lines

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

Hm, we could; I guess then analyzers could still correlate "all these things were called by some kind of wrapper, even if we don't know if that was an IDE or a script or just the user or what, we can guess based on other heuristics". Ok.

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

Good point, will refer to "more than one generation". Thanks.

> > +	if (!strbuf_read_file(&out, procfs_path.buf, 0))
> > +	{
> 
> Place this opening brace at the end of the previous line.

Will polish up this and others for v3, hopefully today.

Show 5 quoted lines

> > +		if (!names[0])
> > +			return;
> 
> OK, so if there is no name given, we do not show pid as a
> placeholder.

Based on your suggestion above I think it will make sense to show pid as placeholder after all, though. So I will change that for v3.

Show 11 quoted lines

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

I wonder what is better. "MAYBE_PROCFS_LINUX"? I don't see any other vars in config.mak.uname that indicate "pretty sure but not totally sure" in a quick scan. However, right above this line we seem to feel certain in our guess about "PROCFS_EXECUTABLE_PATH"...

...but when it is used in exec-cmd.c:git_get_exec_path_procfs(), invoked by exec-cmd.c:git_get_exec_path(), we're very tolerant to faults if it's not there:

  static int git_get_exec_path(struct strbuf *buf, const char *argv0)
  {
  	/*
  	 * [snip]
  	 * Each of these functions returns 0 on success, so evaluation will stop
  	 * after the first successful method.
  	 */
  	if (
  #ifdef HAVE_BSD_KERN_PROC_SYSCTL
  		git_get_exec_path_bsd_sysctl(buf) &&
  #endif /* HAVE_BSD_KERN_PROC_SYSCTL */
  
  #ifdef HAVE_NS_GET_EXECUTABLE_PATH
  		git_get_exec_path_darwin(buf) &&
  #endif /* HAVE_NS_GET_EXECUTABLE_PATH */
  
  #ifdef PROCFS_EXECUTABLE_PATH
  		git_get_exec_path_procfs(buf) &&  /*** <- OK if fails ***/
  #endif /* PROCFS_EXECUTABLE_PATH */
  
  #ifdef HAVE_WPGMPTR
  		git_get_exec_path_wpgmptr(buf) &&
  #endif /* HAVE_WPGMPTR */
  
  		git_get_exec_path_from_argv0(buf, argv0)) {
  		return -1;
  	}
  
  [snip]
  
  #ifdef PROCFS_EXECUTABLE_PATH
  /*
   * Resolves the executable path by examining a procfs symlink.
   *
   * Returns 0 on success, -1 on failure.
   */
  static int git_get_exec_path_procfs(struct strbuf *buf)
  {
  	if (strbuf_realpath(buf, PROCFS_EXECUTABLE_PATH, 0)) {
  		trace_printf(
  			"trace: resolved executable path from procfs: %s\n",
  			buf->buf);
  		return 0;
  	}
  	return -1;
  }
  #endif /* PROCFS_EXECUTABLE_PATH */

So it seems this other procfs bit takes the "probably but not definitely" step and is tolerant at runtime as well. Which doesn't help me much to decide how to rename HAVE_PROCFS_LINUX.

I'll switch it to MAYBE_PROCFS_LINUX for v3 unless someone yells, I guess.

 - Emily