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

On 25 Sep 2016, at 15:46, Jakub Narębski [off-list ref] wrote:

W dniu 20.09.2016 o 21:02, larsxschneider@gmail.com pisze:

quoted

From: Lars Schneider <redacted>

quoted

+{
+	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.

quoted

+	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()"

quoted

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

quoted

+{
+	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 :-)

quoted

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

quoted

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

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

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)

quoted

+		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