Re: [PATCH 2/2] http: update curl http/2 info matching for curl 8.3.0

[Taylor Blau <me@ttaylorr.com>]

On Fri, Sep 15, 2023 at 07:34:43AM -0400, Jeff King wrote:
> @@ -751,6 +753,18 @@ static int match_curl_h2_trace(const char *line, const char **out)
>  	    skip_iprefix(line, "h2 [", out))
>  		return 1;
>
> +	/*
> +	 * curl 8.3.0 uses:
> +	 *   [HTTP/2] [<stream-id>] [<header-name>: <header-val>]
> +	 * where <stream-id> is numeric.
> +	 */
> +	if (skip_iprefix(line, "[HTTP/2] [", &p)) {
> +		while (isdigit(*p))
> +			p++;
> +		if (skip_prefix(p, "] [", out))
> +			return 1;
> +	}
> +

This looks good, too, though I do have one question. The HTTP/2
specification in 5.1 says (among other things):

    Streams are identified with an unsigned 31-bit integer. Streams
    initiated by a client MUST use odd-numbered stream identifiers; those
    initiated by the server MUST use even-numbered stream identifiers. A
    stream identifier of zero (0x0) is used for connection control messages;
    the stream identifier of zero cannot be used to establish a new stream.

So the parsing you wrote here makes sense in that we consume digits
between the pair of square brackets enclosing the stream identifier.

But I think we would happily eat a line like:

    [HTTP/2] [] [Secret: xyz]

even lacking a stream identifier. I think that's reasonably OK in
practice, because we're being over-eager in redacting instead of the
other way around. And we're unlikely to see such a line from curl
anyway, so I don't think that it matters.

If you feel otherwise, though, I think something as simple as:

    if (skip_iprefix(line, "[HTTP/2] [", &p)) {
      if (!*p)
        return 0;
      while (isdigit(*p))
        p++;
      if (skip_prefix(p, "] [", out))
        return 1;
    }

would do the trick. I *think* that this would also work:

    if (skip_iprefix(line, "[HTTP/2] [", &p)) {
      do {
        p++;
      } while (isdigit(*p))
      if (skip_prefix(p, "] [", out))
        return 1;
    }

since we know that p is non-NULL, and if it's the end of the line, *p
will be NUL and isdigit(*p) will return 0. But it's arguably less
direct, and requires some extra reasoning, so I have a vague preference
for the former.

But this may all be moot anyway, I don't feel strongly one way or the
other.

Thanks,
Taylor