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

On 27 Sep 2016, at 11:00, Jeff King [off-list ref] wrote:

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

quoted

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.

TBH in that case I would prefer the "PKTLINE_DATA_MAXLEN+1" solution with
an additional comment explaining "+1".

Would that be OK for you?

I am not worried about the extra 4 bytes. I am worried that we make it harder
to see what is going on if we use LARGE_PACKET_MAX.

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. :)

Yeah, I noticed that too and fixed it in v9 :-) Thanks for the reminder!

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).

That is a good suggestion. I'll add that to my "tool-belt" :-)


Thanks,
Lars