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

From: Emily Shaffer <emilyshaffer@google.com>
To: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
Cc: Jeff Hostetler <git@jeffhostetler.com>,
	git@vger.kernel.org, Junio C Hamano <gitster@pobox.com>,
	Bagas Sanjaya <bagasdotme@gmail.com>,
	"Randall S. Becker" <rsbecker@nexbridge.com>
Subject: Re: [PATCH v5] tr2: log parent process name
Date: Wed, 21 Jul 2021 17:21:07 -0700	[thread overview]
Message-ID: <YPi58/qRPQWhZkiI@google.com> (raw)
In-Reply-To: <87a6n7g9np.fsf@evledraar.gmail.com>

On Wed, Jun 30, 2021 at 08:10:59AM +0200, Ævar Arnfjörð Bjarmason wrote:
> 
> 
> On Tue, Jun 29 2021, Emily Shaffer wrote:
> 
> > On Mon, Jun 28, 2021 at 12:45:24PM -0400, Jeff Hostetler wrote:
> >> On 6/8/21 6:10 PM, Emily Shaffer wrote:
> >> > Range-diff against v4:
> >> > 1:  efb0a3ccb4 ! 1:  7a7e1ebbfa tr2: log parent process name
> >> >      @@ compat/procinfo.c (new)
> >> >       +	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));
> >> >       +	}
> >> >       +
> >> 
> >> You're only getting the name of the command (argv[0]) and not the
> >> full command line, right?  That is a good thing.
> >
> > Roughly. The name can be reset by the process itself (that's what
> > happened, I guess, in the tmux case I pasted below) but by default it's
> > argv[0]. It's also truncated to 15ch or something.
> 
> 16 including the \0. See prctl(2). Linux has two different ways to
> set/get the name, one is the argv method, the other is
> prctl(PR_SET_NAME). They don't need to match at all. The ps(1) utility
> and some top-like utilities allow you to switch between viewing the two
> versions.
> 
> As noted in the linked manual pages you'll also potentially need to deal
> with multithreaded programs having different names for each thread.
> 
> I don't think we use this now, but FWIW one thing I've wanted to do for
> a while was to have the progress.c code update this, so you see if git's
> at N% counting objects or whatever in top.
> 
> >> > +#ifdef HAVE_PROCFS_LINUX
> >> > +	/*
> >> > +	 * 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));
> >> > +	}
> >> > +
> >> > +	return;
> >> > +#endif
> >> > +	/* NEEDSWORK: add non-procfs-linux implementations here */
> >> > +}
> >> 
> >> Perhaps this has already been discussed, but would it be better
> >> to have a "compat/linux/trace2_linux_process_info.c"
> >> or "compat/procfs/trace2_procfs_process_info.c" source file and
> >> only compile it in Linux-compatible builds -- rather than #ifdef'ing
> >> the source.  This is a highly platform-specific feature.
> >> 
> >> For example, if I convert the Win32 version to use your new event,
> >> I wouldn't want to move the code.
> >> 
> >> I just noticed that you have both "BASIC_CFLAGS+=" and a "COMPAT_OBSJ+="
> >> lines.  If you made this source file procfs-specific, you wouldn't need
> >> the ifdef and you could avoid the new CFLAG.
> 
> 
> In general we've preferred not using ifdefs at all except for the small
> bits that absolutely need it.
> 
> So e.g. in this case the whole code should compile on non-Linux, we just
> need a small boolean guard somewhere to check what the platform is.
> 
> It means we don't have significant pieces of code that don't compile
> except on platform X. It's easy to get into your code not compiling if
> you overuse ifdefs.

Hmm. I see what you mean.

However, since win32 is already using some conditionally compiling code,
that's the approach I went for. It's still running CI to make sure I
didn't break Windows in the process, but I'll post it (and the Actions
runs) later today, assuming it passed.

Yes, the implementation I wrote would compile on any platform, but as I
understand it, other platforms may need drastically different
implementations which may not compile as easily as this. So, I'm not
sure if it's really appropriate to do run-time platform checks here
(which is what I think you were describing).

Anyway, I'll look forward to seeing what you folks think of the next
iteration (again, hopefully later today).

 - Emily