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

From: Lars Schneider <larsxschneider@gmail.com>
Date: Sep 26, 2016, 20:23 UTC
Message-ID: <77315FC2-47F3-433A-8D70-5497FB04CBBE@gmail.com>
In-Reply-To: <507a29a1-99ff-c13e-cf1f-76631345a822@gmail.com>

On 25 Sep 2016, at 15:46, Jakub Narębski <jnareb@gmail.com> wrote:

> W dniu 20.09.2016 o 21:02, larsxschneider@gmail.com pisze:
>> From: Lars Schneider <larsxschneider@gmail.com>

Show 5 quoted lines

>> +{
>> +	static char buf[PKTLINE_DATA_MAXLEN];
> 
> Sidenote: we have LARGE_PACKET_MAX (used in previous patch), but
> PKTLINE_DATA_MAXLEN not LARGE_PACKET_DATA_MAX.

Agreed, I will rename it.

Show 22 quoted lines

> 
>> +	int err = 0;
>> +	ssize_t bytes_to_write;
>> +
>> +	while (!err) {
>> +		bytes_to_write = xread(fd_in, buf, sizeof(buf));
>> +		if (bytes_to_write < 0)
>> +			return COPY_READ_ERROR;
>> +		if (bytes_to_write == 0)
>> +			break;
>> +		err = packet_write_gently(fd_out, buf, bytes_to_write);
>> +	}
>> +	if (!err)
>> +		err = packet_flush_gently(fd_out);
>> +	return err;
>> +}
> 
> Looks good: clean and readable.
> 
> Sidenote (probably outside of scope of this patch): what are the
> errors that we can get from this function, beside COPY_READ_ERROR
> of course?

Everything that is returned by "read()"

Show 15 quoted lines

>> +
>> static int get_packet_data(int fd, char **src_buf, size_t *src_size,
>> 			   void *dst, unsigned size, int options)
>> {
>> @@ -305,3 +346,30 @@ char *packet_read_line_buf(char **src, size_t *src_len, int *dst_len)
>> {
>> 	return packet_read_line_generic(-1, src, src_len, dst_len);
>> }
>> +
>> +ssize_t read_packetized_to_buf(int fd_in, struct strbuf *sb_out)
> 
> It's a bit strange that the signature of write_packetized_from_buf() is
> that different from read_packetized_to_buf().  This includes the return
> value: int vs ssize_t.  As I have checked, write() and read() both
> use ssize_t, while fread() and fwrite() both use size_t.

read_packetized_to_buf() returns the number of bytes read or a negative error code.

write_packetized_from_buf() returns 0 if the call was successful and an error code if not.

That's the reason these two functions have a different signature

> Perhaps this function should be named read_packetized_to_strbuf()
> (err, I asked this already)?

I agree with the rename as makes it distinct from write_packetized_from_buf().

>> +{
>> +	int paket_len;
> 
> Possible typo: shouldn't it be called packet_len?

True!

> Shouldn't it be initialized to 0?

Well, it is set for sure later. That's why I think it is not necessary.

Plus, Eric Wong thought me not to: "Compilers complain about uninitialized variables." http://public-inbox.org/git/20160725072745.GB11634@starla/ (Note: he was talking about pointers there :-)

>> +	int options = PACKET_READ_GENTLE_ON_EOF;
> 
> Why is this even a local variable?  It is never changed, and it is
> used only in one place; we can inline it.

Removed.

Show 10 quoted lines

>> +
>> +	size_t oldlen = sb_out->len;
>> +	size_t oldalloc = sb_out->alloc;
> 
> Just a nitpick (feel free to ignore): doesn't this looks better:
> 
>  +	size_t old_len   = sb_out->len;
>  +	size_t old_alloc = sb_out->alloc;
> 
> Also perhaps s/old_/orig_/g.

Agreed. That matches the other variables better.

Show 6 quoted lines

>> +		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);
> 
> A question (which perhaps was answered during the development of this
> patch series): why is this +1 in PKTLINE_DATA_MAXLEN+1 here?

Nice catch. I think this is wrong: https://github.com/git/git/blob/6fe1b1407ed91823daa5d487abe457ff37463349/pkt-line.c#L196

It should be "if (len > size)" ... then we don't need the "+1" here. (but I need to think a bit more about this)

Show 14 quoted lines

> 
>> +		if (paket_len <= 0)
>> +			break;
>> +		sb_out->len += paket_len;
>> +	}
>> +
>> +	if (paket_len < 0) {
>> +		if (oldalloc == 0)
>> +			strbuf_release(sb_out);
>> +		else
>> +			strbuf_setlen(sb_out, oldlen);
> 
> A question (maybe I don't understand strbufs): why there is a special
> case for oldalloc == 0?

I tried to mimic the behavior of strbuf_read() [1]. The error handling was introduced in 2fc647 [2] to ease error handling:

"This allows for easier error handling, as callers only need to call strbuf_release() if A) the command succeeded or B) if they would have had to do so anyway because they added something to the strbuf themselves."

[1] https://github.com/git/git/blob/cda1bbd474805e653dda8a71d4ea3790e2a66cbb/strbuf.c#L377-L383 [2] https://github.com/git/git/commit/2fc647004ac7016128372a85db8245581e493812

Thanks, Lars