git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Taylor Blau <me@ttaylorr.com>
To: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
Cc: git@vger.kernel.org, Junio C Hamano <gitster@pobox.com>,
	Emily Shaffer <emilyshaffer@google.com>
Subject: Re: [PATCH 6/6] tr2: log N parent process names on Linux
Date: Thu, 26 Aug 2021 00:07:04 -0400	[thread overview]
Message-ID: <YScTaDcPTs1nrP2Y@nand.local> (raw)
In-Reply-To: <patch-6.6-da003330800-20210825T231400Z-avarab@gmail.com>

On Thu, Aug 26, 2021 at 01:19:24AM +0200, Ævar Arnfjörð Bjarmason wrote:
> In 2f732bf15e6 (tr2: log parent process name, 2021-07-21) we started
> logging parent process names, but only logged all parents on Windows.
> on Linux only the name of the immediate parent process was logged.
>
> Extend the functionality added there to also log full parent chain on
> Linux. In 2f732bf15e6 it was claimed that "further ancestry info can
> be gathered with procfs, but it's unwieldy to do so.".
>
> I don't know what the author meant by that, but I think it probably
> referred to needing to slurp this up from the FS, as opposed to having
> an API.

I don't think that this (specifically, "I don't know what the author
meant by that") is necessary information to include in a patch message.

If you're looking for a replacement (and you may not be, but just my
$.02) I would suggest:

    "2f732bf15e6 does not log the full parent chain on Linux; implement
    that functionality here."

> What is more complex on Linux is getting at the process name, a
> simpler approach is to use fscanf(), see [1] for an implementation of
> that, but as noted in the comment being added here it would fail in
> the face of some weird process names, so we need our own
> parse_proc_stat() to parse it out.

This is helpful information to have for readers that aren't familiar
with procfs (especially the detail about why the naive approach doesn't
work).

> diff --git a/compat/linux/procinfo.c b/compat/linux/procinfo.c
> index 46a751c9a1d..937084126a6 100644
> --- a/compat/linux/procinfo.c
> +++ b/compat/linux/procinfo.c
> @@ -4,27 +4,129 @@
>  #include "strvec.h"
>  #include "trace2.h"
>
> -static void get_ancestry_names(struct strvec *names)
> +/*
> + * We need more complex parsing instat_parent_pid() and
> + * parse_proc_stat() below than a dumb fscanf(). That's because while
> + * the statcomm field is surrounded by parentheses, the process itself
> + * is free to insert any arbitrary byte sequence its its name. That
> + * can include newlines, spaces, closing parentheses etc. See
> + * do_task_stat() in fs/proc/array.c in linux.git, this is in contrast
> + * with the escaped version of the name found in /proc/%d/status.
> + *
> + * So instead of using fscanf() we'll read N bytes from it, look for
> + * the first "(", and then the last ")", anything in-between is our
> + * process name.
> + *
> + * How much N do we need? On Linux /proc/sys/kernel/pid_max is 2^15 by
> + * default, but it can be raised set to values of up to 2^22. So
> + * that's 7 digits for a PID. We have 2 PIDs in the first four fields
> + * we're interested in, so 2 * 7 = 14.
> + *
> + * We then have 4 spaces between those four values, which brings us up
> + * to 18. Add the two parentheses and it's 20. The "state" is then one
> + * character (now at 21).

Hmm, aren't there three spaces, not four?

> + * Finally the maximum length of the "comm" name itself is 15
> + * characters, e.g. a setting of "123456789abcdefg" will be truncated
> + * to "123456789abcdef". See PR_SET_NAME in prctl(2). So all in all
> + * we'd need to read 21 + 15 = 36 bytes.

Ah, 36 is the right number even though you and I counted a different
number of spaces, since the name is truncated when it goes over *16*
characters, but that includes the NUL byte. So we both arrive at the
same number in the end ;).

But I agree it's safer to just read more (but not too much more) than
what we need.

> + * Let's just read 2^6 (64) instead for good measure. If PID_MAX ever
> + * grows past 2^22 we'll be future-proof. We'll then anchor at the
> + * last ")" we find to locate the parent PID.
> + */
> +#define STAT_PARENT_PID_READ_N 64
> +
> +static int parse_proc_stat(struct strbuf *sb, struct strbuf *name,
> +			    int *statppid)

Going to think aloud to make sure that this parsing looks right.

>  {
> +	const char *lhs = strchr(sb->buf, '(');
> +	const char *rhs = strrchr(sb->buf, ')');

lhs and rhs are going to be on either side of the comm field (which may
be helpful to indicate by calling these comm_lhs and comm_rhs). And
strrchr makes sure to handle process names that have a ')' in them.
Looks right.

> +	const char *ppid_lhs, *ppid_rhs;
> +	char *p;
> +	pid_t ppid;
> +
> +	if (!lhs || !rhs)
> +		goto bad_kernel;
> +

OK.

>  	/*
> -	 * 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.
> +	 * We're at the ")", that's followed by " X ", where X is a
> +	 * single "state" character. So advance by 4 bytes.
>  	 */
> +	ppid_lhs = rhs + 4;
> +
> +	ppid_rhs = strchr(ppid_lhs, ' ');

Skipping over the state field gives us the first character of ppid as
you say, good. And reading until the first space character will point us
right after the end, good.

> +	if (!ppid_rhs)
> +		goto bad_kernel;
> +
> +	ppid = strtol(ppid_lhs, &p, 10);
> +	if (ppid_rhs == p) {

Then parse the ppid and make sure we stopped at the right-hand side
where we should have. Good.

> +		const char *comm = lhs + 1;

Skipping past the '(', but now I feel like we really should
s/lhs/comm_&/.

> +		int commlen = rhs - lhs - 1;

This is right, but you could simplify the expression to be "rhs - comm",
since you just took into account the left-hand parenthesis in the
previous line. Also recommend a size_t here: it's obvious we're not
going to overflow int here, but it saves future readers of having to
wonder the same thing.

> +
> +		strbuf_addf(name, "%.*s", commlen, comm);
> +		*statppid = ppid;
> +
> +		return 0;
> +	}
> +
> +bad_kernel:
> +	/*
> +	 * We were able to read our STAT_PARENT_PID_READ_N bytes from
> +	 * /proc/%d/stat, but the content is bad. Broken kernel?
> +	 * Should not happen, but handle it gracefully.
> +	 */
> +	return -1;
> +}

Phew, all seems good. Thanks for bearing with me while I read through
all of that ;).

> +static int stat_parent_pid(pid_t pid, struct strbuf *name, int *statppid)
> +{
>  	struct strbuf procfs_path = STRBUF_INIT;
> -	struct strbuf name = STRBUF_INIT;
> +	struct strbuf sb = STRBUF_INIT;
> +	size_t n;
> +	FILE *fp = NULL;

fopen() will return NULL, and you call it unconditionally, so no need to
initialize here.

> +	int ret = -1;
>
>  	/* 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) > 0) {
> -		strbuf_trim_trailing_newline(&name);
> -		strvec_push(names, name.buf);
> -		strbuf_release(&name);
> -	}
> +	strbuf_addf(&procfs_path, "/proc/%d/stat", pid);
> +	fp = fopen(procfs_path.buf, "r");
> +	if (!fp)
> +		goto cleanup;
> +
> +	n = strbuf_fread(&sb, STAT_PARENT_PID_READ_N, fp);
> +	if (n != STAT_PARENT_PID_READ_N)
> +		goto cleanup;

Hmm. Wouldn't we always goto cleanup here, since STAT_PARENT_PID_READ_N
is deliberately oversized (and not constant anyways, since process ids
could be anywhere from 1-7 digits long)?

I think we could probably drop 'n' entirely here, and instead:

    if (strbuf_fread(...) < 0)
      goto cleanup;

> +	if (parse_proc_stat(&sb, name, statppid) < 0)
> +		goto cleanup;
>
> +	ret = 0;
> +cleanup:
> +	if (fp)
> +		fclose(fp);
>  	strbuf_release(&procfs_path);
> +	strbuf_release(&sb);
> +
> +	return ret;
> +}
> +
> +static void push_ancestry_name(struct strvec *names, pid_t pid)
> +{
> +	struct strbuf name = STRBUF_INIT;
> +	int ppid;
> +
> +	if (stat_parent_pid(pid, &name, &ppid) < 0)
> +		goto cleanup;
> +
> +	strvec_push(names, name.buf);
> +
> +	/*
> +	 * Both errors and reaching the end of the process chain are
> +	 * reported as fields of 0 by proc(5)
> +	 */
> +	if (ppid)
> +		push_ancestry_name(names, ppid);
> +cleanup:
> +	strbuf_release(&name);
>  	return;
>  }

The rest looks good to me, but it looks like you overwrote all of the
work that you did in patch 4/6. I guess separating them out makes sense
if this patch wasn't taken, but I probably would have gone right to this
patch instead of fixing leaks that you were going to get rid of anyway.

Thanks,
Taylor

  parent reply	other threads:[~2021-08-26  4:07 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
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 [this message]
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=YScTaDcPTs1nrP2Y@nand.local \
    --to=me@ttaylorr.com \
    --cc=avarab@gmail.com \
    --cc=emilyshaffer@google.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.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).