Re: [PATCH v5] tr2: log parent process name
- From
Ævar Arnfjörð Bjarmason <avarab@gmail.com>
- Date
- Jun 30, 2021, 06:10 UTC
- Message-ID
- <87a6n7g9np.fsf@evledraar.gmail.com>
- In-Reply-To
- <YNux62he9Mk43Y1B@google.com>
On Tue, Jun 29 2021, Emily Shaffer wrote:
Show 19 quoted lines
> 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.
Show 36 quoted lines
>> > +#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.