Re: [PATCH v8 07/11] pkt-line: add functions to read/write flush terminated packet streams

On Tue, Sep 27, 2016 at 10:14:16AM +0200, Lars Schneider wrote:

quoted

quoted

quoted

+		strbuf_grow(sb_out, PKTLINE_DATA_MAXLEN+1);
+		paket_len = packet_read(fd_in, NULL, NULL,
+			sb_out->buf + sb_out->len, PKTLINE_DATA_MAXLEN+1, options);

[...]
After looking at it with fresh eyes I think the existing code is probably correct,
but maybe a bit confusing.

packet_read() adds a '\0' at the end of the destination buffer:
https://github.com/git/git/blob/21f862b498925194f8f1ebe8203b7a7df756555b/pkt-line.c#L206

That is why the destination buffer needs to be one byte larger than the expected content.

However, in this particular case that wouldn't be necessary because the destination
buffer is a 'strbuf' that allocates an extra byte for '\0' at the end. But we are not
supposed to write to this extra byte:
https://github.com/git/git/blob/21f862b498925194f8f1ebe8203b7a7df756555b/strbuf.h#L25-L31

Right. The allocation happens as part of strbuf_grow(), but whatever
fills the buffer is expected to write the actual NUL (either manually,
or by calling strbuf_setlen().

I see the bit you quoted warns not to touch the extra byte yourself,
though I wonder if that is a bit heavy-handed (I guess it would matter
if we moved the extra 1-byte growth into strbuf_setlen(), but I find
that a rather unlikely change).

That being said, why don't you just use LARGE_PACKET_MAX here? It is
already the accepted size for feeding to packet_read(), and we know it
has enough space to hold a NUL terminator. Yes, we may over-allocate by
4 bytes, but that isn't really relevant. Strbufs over-allocate anyway.
So just:

  for (;;) {
        strbuf_grow(sb_out, LARGE_PACKET_MAX);
        packet_len = packet_read(fd_in, NULL, NULL,
	                         sb_out->buf + sb_out->len, LARGE_PACKET_MAX,
				 options);
        if (packet_len <= 0)
                break;
        /*
         * no need for strbuf_setlen() here; packet_read always adds a
         * NUL terminator.
         */
        sb_out->len += packet_len;
  }

You _could_ make the final line of the loop use strbuf_setlen(); it
would just overwrite something we already know is a NUL (and we know
that no extra allocation is necessary).

Also, using LARGE_PACKET_MAX fixes the fact that this patch is using
PKTLINE_DATA_MAXLEN before it is actually defined. :)

You might want to occasionally run:

  git rebase -x make

to make sure all of your incremental steps are valid (or even "make
test" if you are extremely patient; I often do that once after a big
round of complex interactive-rebase reordering).

I see two options:


(1) I leave the +1 as is and add a comment why the extra byte is necessary.

    Pro: No change in existing code necessary
    Con: The destination buffer has two '\0' at the end.


(2) I add an option PACKET_READ_DISABLE_NUL_TERMINATION. If the option is
    set then no '\0' byte is added to the end.

    Pro: Correct solution, no byte wasted.
    Con: Change in existing code required.


Any preference?

Of the two, I prefer (1), though I like what I suggested above even more
(big surprise, I know).

-Peff