From: Emily Shaffer <emilyshaffer@google.com>
To: Jeff Hostetler <git@jeffhostetler.com>
Cc: git@vger.kernel.org, "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>,
"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: Tue, 29 Jun 2021 16:51:07 -0700 [thread overview]
Message-ID: <YNux62he9Mk43Y1B@google.com> (raw)
In-Reply-To: <3327f108-6cd1-1d3d-eae9-2cdff96e1375@jeffhostetler.com>
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.
> > ------------
> > +`"cmd_ancestry"`::
> > + This event contains the text command name for the parent (and earlier
> > + generations of parents) of the current process, in an array ordered from
> > + nearest parent to furthest great-grandparent. It may not be implemented
> > + on all platforms.
> > ++
> > +------------
> > +{
> > + "event":"cmd_ancestry",
> > + ...
> > + "ancestry":["bash","tmux: server","systemd"]
>
> Is the second element really "tmux: server". Seems odd that that's what
> the command name (argv[0]) is. Perhaps I misread something??
See above. This is what shows up in pstree, though, and by poking around in
/proc I confirmed that this is indeed the content of /proc/<tmux-pid>/comm:
├─tmux: server─┬─bash───mutt───open-vim-in-new───vim
│ ├─bash───pstree
│ └─mutt
This is a somewhat contrived example, though, because in Linux as of this patch,
only one ancestor is gathered. So maybe I had better make the doc
reflect what's actually possible. I'm planning on sending a follow-on
sometime soon exposing more generations of ancestry, so I guess I could
update the docs back to this state around then.
>
> > +}
>
> This array is bounded and that implies that you captured all of
> the grand parents back to "init" (or whatever it is called these
> days).
In this case it does - pid 1 is systemd, which hasn't got a parent
process.
> Is there value in having a final "..." or "(truncated)" element
> to indicate that the list incomplete? I did the latter in the
> Windows version.
Hrm. I'm not the one who wants to parse these - it's someone else who's
working with our team internally - so I'll ask around and see what they
think is best.
> > +#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.
Sure, I'll investigate it, thanks.
> > +
> > + if (names.nr == 0) {
> > + strvec_clear(&names);
> > + return;
> > + }
> > +
> > + trace2_cmd_ancestry(names.v);
> > +
> > + strvec_clear(&names);
>
> I agree with Junio here, it would be simpler to say it like this:
>
> get_ancestry_names(&names);
> if (names.nr)
> trace2_cmd_ancestry(names.v);
> strvec_clear(&names);
>
Thanks both, done locally.
> Otherwise, this looks good to me.
Thanks. Look for a v6 from me this week, hopefully with the build stuff
sorted out.
- Emily
next prev parent reply other threads:[~2021-06-29 23:51 UTC|newest]
Thread overview: 87+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-05-07 0:29 [PATCH] tr2: log parent process name Emily Shaffer
2021-05-07 3:25 ` Bagas Sanjaya
2021-05-07 17:09 ` Emily Shaffer
2021-05-10 12:29 ` Ævar Arnfjörð Bjarmason
2021-05-11 21:31 ` Junio C Hamano
2021-05-14 22:06 ` Emily Shaffer
2021-05-16 3:48 ` Junio C Hamano
2021-05-17 20:17 ` Emily Shaffer
2021-05-11 17:28 ` Jeff Hostetler
2021-05-14 22:07 ` Emily Shaffer
2021-05-20 21:05 ` [PATCH v2] " Emily Shaffer
2021-05-20 21:36 ` Randall S. Becker
2021-05-20 23:23 ` Emily Shaffer
2021-05-21 13:20 ` Randall S. Becker
2021-05-21 16:24 ` Randall S. Becker
2021-05-21 2:09 ` Junio C Hamano
2021-05-21 19:02 ` Emily Shaffer
2021-05-21 23:22 ` Junio C Hamano
2021-05-24 18:37 ` Emily Shaffer
2021-05-21 19:15 ` Jeff Hostetler
2021-05-21 20:05 ` Emily Shaffer
2021-05-21 20:23 ` Randall S. Becker
2021-05-22 11:18 ` Jeff Hostetler
2021-05-24 23:33 ` Ævar Arnfjörð Bjarmason
2021-05-24 20:10 ` [PATCH v3] " Emily Shaffer
2021-05-24 20:49 ` Emily Shaffer
2021-05-25 3:54 ` Junio C Hamano
2021-05-25 13:33 ` Randall S. Becker
2021-06-08 18:58 ` [PATCH v4] " Emily Shaffer
2021-06-08 20:56 ` Emily Shaffer
2021-06-08 22:10 ` [PATCH v5] " Emily Shaffer
2021-06-08 22:16 ` Randall S. Becker
2021-06-08 22:24 ` Emily Shaffer
2021-06-08 22:39 ` Randall S. Becker
2021-06-09 20:17 ` Emily Shaffer
2021-06-16 8:42 ` Junio C Hamano
2021-06-28 16:45 ` Jeff Hostetler
2021-06-29 23:51 ` Emily Shaffer [this message]
2021-06-30 6:10 ` Ævar Arnfjörð Bjarmason
2021-07-22 0:21 ` Emily Shaffer
2021-07-22 1:27 ` [PATCH v6 0/2] " Emily Shaffer
2021-07-22 1:27 ` [PATCH v6 1/2] tr2: make process info collection platform-generic Emily Shaffer
2021-08-02 9:34 ` Ævar Arnfjörð Bjarmason
2021-07-22 1:27 ` [PATCH v6 2/2] tr2: log parent process name Emily Shaffer
2021-07-22 21:02 ` Junio C Hamano
2021-08-02 9:38 ` Ævar Arnfjörð Bjarmason
2021-08-02 12:45 ` Ævar Arnfjörð Bjarmason
2021-08-02 10:22 ` Ævar Arnfjörð Bjarmason
2021-08-02 12:47 ` Ævar Arnfjörð Bjarmason
2021-08-02 15:23 ` Jeff Hostetler
2021-08-02 16:10 ` Randall S. Becker
2021-08-02 18:41 ` Ævar Arnfjörð Bjarmason
2021-08-25 23:19 ` [PATCH 0/6] tr2: plug memory leaks + logic errors + Win32 & Linux feature parity Ævar Arnfjörð Bjarmason
2021-08-25 23:19 ` [PATCH 1/6] tr2: remove NEEDSWORK comment for "non-procfs" implementations Ævar Arnfjörð Bjarmason
2021-08-25 23:19 ` [PATCH 2/6] tr2: clarify TRACE2_PROCESS_INFO_EXIT comment under Linux Ævar Arnfjörð Bjarmason
2021-08-25 23:19 ` [PATCH 3/6] tr2: stop leaking "thread_name" memory Ævar Arnfjörð Bjarmason
2021-08-26 3:09 ` Taylor Blau
2021-08-25 23:19 ` [PATCH 4/6] tr2: fix memory leak & logic error in 2f732bf15e6 Ævar Arnfjörð Bjarmason
2021-08-26 3:21 ` Taylor Blau
2021-08-25 23:19 ` [PATCH 5/6] tr2: do compiler enum check in trace2_collect_process_info() Ævar Arnfjörð Bjarmason
2021-08-26 3:23 ` Taylor Blau
2021-08-25 23:19 ` [PATCH 6/6] tr2: log N parent process names on Linux Ævar Arnfjörð Bjarmason
2021-08-25 23:49 ` Eric Sunshine
2021-08-26 4:07 ` Taylor Blau
2021-08-26 12:24 ` "I don't know what the author meant by that..." (was "Re: [PATCH 6/6] tr2: log N parent process names on Linux") Ævar Arnfjörð Bjarmason
2021-08-26 12:22 ` [PATCH v2 0/6] tr2: plug memory leaks + logic errors + Win32 & Linux feature parity Ævar Arnfjörð Bjarmason
2021-08-26 12:22 ` [PATCH v2 1/6] tr2: remove NEEDSWORK comment for "non-procfs" implementations Ævar Arnfjörð Bjarmason
2021-08-26 12:22 ` [PATCH v2 2/6] tr2: clarify TRACE2_PROCESS_INFO_EXIT comment under Linux Ævar Arnfjörð Bjarmason
2021-08-26 12:22 ` [PATCH v2 3/6] tr2: stop leaking "thread_name" memory Ævar Arnfjörð Bjarmason
2021-08-26 12:22 ` [PATCH v2 4/6] tr2: fix memory leak & logic error in 2f732bf15e6 Ævar Arnfjörð Bjarmason
2021-08-26 15:58 ` Eric Sunshine
2021-08-26 16:42 ` Junio C Hamano
2021-08-26 12:22 ` [PATCH v2 5/6] tr2: do compiler enum check in trace2_collect_process_info() Ævar Arnfjörð Bjarmason
2021-08-26 12:22 ` [PATCH v2 6/6] tr2: log N parent process names on Linux Ævar Arnfjörð Bjarmason
2021-08-26 22:38 ` [PATCH v2 0/6] tr2: plug memory leaks + logic errors + Win32 & Linux feature parity Taylor Blau
2021-08-27 8:02 ` [PATCH v3 " Ævar Arnfjörð Bjarmason
2021-08-27 8:02 ` [PATCH v3 1/6] tr2: remove NEEDSWORK comment for "non-procfs" implementations Ævar Arnfjörð Bjarmason
2021-08-27 8:02 ` [PATCH v3 2/6] tr2: clarify TRACE2_PROCESS_INFO_EXIT comment under Linux Ævar Arnfjörð Bjarmason
2021-08-27 8:02 ` [PATCH v3 3/6] tr2: stop leaking "thread_name" memory Ævar Arnfjörð Bjarmason
2021-08-27 8:02 ` [PATCH v3 4/6] tr2: leave the parent list empty upon failure & don't leak memory Ævar Arnfjörð Bjarmason
2021-08-27 8:02 ` [PATCH v3 5/6] tr2: do compiler enum check in trace2_collect_process_info() Ævar Arnfjörð Bjarmason
2021-08-27 8:02 ` [PATCH v3 6/6] tr2: log N parent process names on Linux Ævar Arnfjörð Bjarmason
2021-08-31 0:17 ` [PATCH v3 0/6] tr2: plug memory leaks + logic errors + Win32 & Linux feature parity Taylor Blau
2021-08-02 10:30 ` [PATCH v6 2/2] tr2: log parent process name Ævar Arnfjörð Bjarmason
2021-08-02 16:24 ` Junio C Hamano
2021-08-02 18:42 ` Ævar Arnfjörð Bjarmason
2021-07-22 16:59 ` [PATCH v6 0/2] " Jeff Hostetler
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=YNux62he9Mk43Y1B@google.com \
--to=emilyshaffer@google.com \
--cc=avarab@gmail.com \
--cc=bagasdotme@gmail.com \
--cc=git@jeffhostetler.com \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=rsbecker@nexbridge.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).