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
