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.
