RE: [PATCH v3] tr2: log parent process name
On May 24, 2021 11:54 PM, Junio C Hamano wrote:
Show 66 quoted lines
>Emily Shaffer <emilyshaffer@google.com> writes:
>
>> diff --git a/compat/procinfo.c b/compat/procinfo.c new file mode
>> 100644 index 0000000000..0e92fb8b7c
>> --- /dev/null
>> +++ b/compat/procinfo.c
>> @@ -0,0 +1,51 @@
>> +#include "cache.h"
>> +
>> +#include "strbuf.h"
>> +#include "trace2.h"
>> +
>> +char *get_process_name(int pid)
>> +{
>> +#ifdef HAVE_PROCFS_LINUX
>> + struct strbuf procfs_path = STRBUF_INIT;
>> + struct strbuf out = STRBUF_INIT;
>> + /* try to use procfs if it's present. */
>> + strbuf_addf(&procfs_path, "/proc/%d/comm", pid);
>> + if (!strbuf_read_file(&out, procfs_path.buf, 0)) {
>> + /* All done with file reads, clean up early */
>> + strbuf_release(&procfs_path);
>> + return strbuf_detach(&out, NULL);
>> + }
>> +#endif
>> +
>> + /* NEEDSWORK: add non-procfs implementations here. */
>> + return NULL;
>> +}
>
>Is the reason why this takes "int" and not "pid_t" because we may port to non-POSIX platforms that do not have pid_t defined?
>
> ... goes and greps ...
>
>Nah, we use pid_t everywhere (including compat/mingw.c); unless there is a reason not to, let's use that type.
>
>> +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.
>> + */
>> + char *names[2];
>> + names[0] = get_process_name(getppid());
>> + names[1] = NULL;
>
>Makes me wonder if get_process_name() is an appropriate abstraction; specifically, something like
>
> const char **names = get_ancestry_names();
> int cnt;
> if (names)
> trace2_cmd_ancestry(names);
> for (cnt = 0; names[cnt]; cnt++)
> free((char *)names[cnt]);
> free(names);
>
>would allow platforms to decide how many levels is easy for them to grab for reporting, for example (and they do not even have to have
>to assume that getting process IDs to feed get_process_name() one by one is the easiest way to show ancestry).Passing pid_t == 1 to get_process_names is non-informative. We can't tell what is really intended inside get_process_names(). Knowing that we are getting the ancestor, however, gives us an the important glue that the parent is wanted so we can interpret 1 as a non-ancestor. NonStop has pid_t defined regardless of whether it is used or relevant, so a higher-level of abstraction makes sense. The git process always has a valid pid_t, but the ancestor may not, but we do not know this at compile time. The platform code previously shared appears to be the correct technique for us. Going with get_ancestry_names() is interesting, but I think a count (how many ancestors do you want) is important (1 - just immediate, 2 - parent, grandparent, -1 - everyone). The ancestry tree can be very large (although each lookup is only about 8us). During a trace, I really would not want to care about more than 2 ancestors, typically, not the likely 8 or 10 that I can see happening in my situation (too much
noise). The number of ancestors to trace probably needs to be in .git.config.
Randall S. Becker <rsbecker@nexbridge.com>