Re: [PATCH v6 2/2] tr2: log parent process name
- From
Ævar Arnfjörð Bjarmason <avarab@gmail.com>
- Date
- Aug 2, 2021, 09:38 UTC
- Message-ID
- <87r1fcp4gy.fsf@evledraar.gmail.com>
- In-Reply-To
- <20210722012707.205776-3-emilyshaffer@google.com>
On Wed, Jul 21 2021, Emily Shaffer wrote:
Show 14 quoted lines
> +`"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"]
> +}
> +------------
> +Okey, so because of later NEEDSWORK no system that runs systemd will currrently have output like this, just Windows.
Show 7 quoted lines
> + /* > + * 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. > + */
This comment:
Show 25 quoted lines
> + 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;
> + /* NEEDSWORK: add non-procfs-linux implementations here */
> +}
> +
> +void trace2_collect_process_info(enum trace2_process_info_reason reason)
> +{
> + if (!trace2_is_enabled())
> + return;
> +
> + /* someday we may want to write something extra here, but not today */
> + if (reason == TRACE2_PROCESS_INFO_EXIT)
> + return;
> +
> + if (reason == TRACE2_PROCESS_INFO_STARTUP) {This should be a switch/case, so we get the compiler asserting/warning if we don't check enum arms, in this case there's just these two, let's have that be clear for readability.
> + /* > + * NEEDSWORK: we could do the entire ptree in an array instead, > + * see compat/win32/trace2_win32_process_info.c. > + */
Seems to remove the need for this comment, i.e. we comment on this limitation of linux-specific get_ancestry_names twice, both in the function and its caller.
Show 5 quoted lines
> -
> + elsif ($event eq 'cmd_ancestry') {
> + # 'cmd_ancestry' is platform-specific and not implemented everywhere, so
> + # just skip it for testing purposes.
> + }The rest of this code uses two "\n" between elsif arms, let's be consistent here.