Re: [PATCH 1/3] pkt-line: simplify starts_with checks in packet tracing

From: Stefan Beller <sbeller@google.com>
Date: Jun 12, 2015, 21:35 UTC
Message-ID: <CAGZ79kaQro5bEcMAbZG1JBfb7mT7n7OKFaq4ECzcGCgimQe9sg@mail.gmail.com>
In-Reply-To: <20150612212808.GA25757@peff.net>

On Fri, Jun 12, 2015 at 2:28 PM, Jeff King <peff@peff.net> wrote:

Show 33 quoted lines

> We carefully check that our pkt buffer has enough characters
> before seeing if it starts with "PACK". The intent is to
> avoid reading random memory if we get a short buffer like
> "PAC".
>
> However, we know that the traced packets are always
> NUL-terminated. They come from one of these sources:
>
>   1. A string literal.
>
>   2. `format_packet`, which uses a strbuf.
>
>   3. `packet_read`, which defensively NUL-terminates what we
>      read.
>
> We can therefore drop the length checks, as we know we will
> hit the trailing NUL if we have a short input.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  pkt-line.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/pkt-line.c b/pkt-line.c
> index 187a229..0477d2e 100644
> --- a/pkt-line.c
> +++ b/pkt-line.c
> @@ -24,8 +24,7 @@ static void packet_trace(const char *buf, unsigned int len, int write)
>         strbuf_addf(&out, "packet: %12s%c ",
>                     packet_trace_prefix, write ? '>' : '<');
>
> -       if ((len >= 4 && starts_with(buf, "PACK")) ||
> -           (len >= 5 && starts_with(buf+1, "PACK"))) {

So I wondered why there is a possible extra character in front of PACK. So I run the blame machinery and ended up with bbc30f9963 (add packet tracing debug code, 2011-02-24), which was also authored by you. Where does the extra char come from?

Would it be better for readability to write it as

    int offset = 0;
    if (*buf == CHARACTER_STEFAN_IS_WONDERING_ABOUT)
        /* ignore char foo because bar */
        offset++;
    if (starts_with(buf+offset, "PACK") {
        ...

Show 7 quoted lines

> +       if (starts_with(buf, "PACK") || starts_with(buf + 1, "PACK")) {
>                 strbuf_addstr(&out, "PACK ...");
>                 trace_disable(&trace_packet);
>         }
> --
> 2.4.2.752.geeb594a
>