Emily Shaffer <emilyshaffer@xxxxxxxxxx> writes:
> Unfortunately, there's no cross-platform reliable way to gather the name
> of the parent process. If procfs is present, we can use that; otherwise
> we will need to discover the name another way. However, the process ID
> should be sufficient to look up the process name on most platforms, so
> that code may be shareable.
Is the sentence that begin with "However" still relevant? The
latest get_ancestry_names() does not add anything when the result
read from the procfs is unusable, but the sentence gives a false
impression that it may somehow fall back to show the PID.
> Git for Windows also gathers information about more than one generation
> of parent. In Linux further ancestry info can be gathered with procfs,
> but it's unwieldy to do so. In the interest of later moving Git for
> Windows ancestry logging to the 'cmd_ancestry' event, and in the
> interest of later adding more ancestry to the Linux implementation - or
> of adding this functionality to other platforms which have an easier
> time walking the process tree - let's make 'cmd_ancestry' accept an
> array of parentage.
Clearly written. Nice.
> +`"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"]
> +}
> +------------
OK.
> diff --git a/compat/linux/procinfo.c b/compat/linux/procinfo.c
> new file mode 100644
> index 0000000000..578fed4cd3
> --- /dev/null
> +++ b/compat/linux/procinfo.c
> @@ -0,0 +1,55 @@
> +#include "cache.h"
> +
> +#include "strbuf.h"
> +#include "strvec.h"
> +#include "trace2.h"
> +
> +static void get_ancestry_names(struct strvec *names)
> +{
> + /*
> + * 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));
At this point, we successfully read from /proc/$(ppid)/comm
and pushed the result into names strvec. I think you would want to
have an explicit "return;" here.
Alternatively, you could put the rest of the function in the
corresponding "else" clause, but I think an early return after each
method successfully collects the result would make a lot more sense.
> + }
> +
> + return;
> + /* NEEDSWORK: add non-procfs-linux implementations here */
This looks the other way around. A future non-procfs-linux
implementation written here will be unreachable code ;-)
Just lose "return;" from here, have one in the if(){} body, and keep
the "here is where you add more/other implementations" comment and
we'd be OK, I think.
> +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) {
> + /*
> + * NEEDSWORK: we could do the entire ptree in an array instead,
> + * see compat/win32/trace2_win32_process_info.c.
> + */
> + struct strvec names = STRVEC_INIT;
> +
> + get_ancestry_names(&names);
> +
> + if (names.nr)
> + trace2_cmd_ancestry(names.v);
> + strvec_clear(&names);
> + }
> +
> + return;
> +}
Good.
Thanks.
