Re: [PATCH v9 10/14] pkt-line: add functions to read/write flush terminated packet streams

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 4, 2016, 19:53 UTC
Message-ID: <xmqq8tu3ubzl.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20161004125947.67104-11-larsxschneider@gmail.com>

larsxschneider@gmail.com writes:

Show 43 quoted lines

> From: Lars Schneider <larsxschneider@gmail.com>
>
> write_packetized_from_fd() and write_packetized_from_buf() write a
> stream of packets. All content packets use the maximal packet size
> except for the last one. After the last content packet a `flush` control
> packet is written.
>
> read_packetized_to_strbuf() reads arbitrary sized packets until it
> detects a `flush` packet.
>
> Signed-off-by: Lars Schneider <larsxschneider@gmail.com>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>  pkt-line.c | 69 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  pkt-line.h |  8 ++++++++
>  2 files changed, 77 insertions(+)
>
> diff --git a/pkt-line.c b/pkt-line.c
> index 3fd4dc0..8ffde22 100644
> --- a/pkt-line.c
> +++ b/pkt-line.c
> @@ -196,6 +196,47 @@ void packet_buf_write(struct strbuf *buf, const char *fmt, ...)
>  	va_end(args);
>  }
>  
> +int write_packetized_from_fd(int fd_in, int fd_out)
> +{
> +	static char buf[LARGE_PACKET_DATA_MAX];
> +	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;
> +}

OK.

Show 21 quoted lines

> +int write_packetized_from_buf(const char *src_in, size_t len, int fd_out)
> +{
> +	static char buf[LARGE_PACKET_DATA_MAX];
> +	int err = 0;
> +	size_t bytes_written = 0;
> +	size_t bytes_to_write;
> +
> +	while (!err) {
> +		if ((len - bytes_written) > sizeof(buf))
> +			bytes_to_write = sizeof(buf);
> +		else
> +			bytes_to_write = len - bytes_written;
> +		if (bytes_to_write == 0)
> +			break;
> +		err = packet_write_gently(fd_out, src_in + bytes_written, bytes_to_write);
> +		bytes_written += bytes_to_write;
> +	}
> +	if (!err)
> +		err = packet_flush_gently(fd_out);
> +	return err;
> +}

Hmph, what is buf[] used for, other than its sizeof() taken to yield a constant LARGE_PACKET_DATA_MAX?

Show 16 quoted lines

> @@ -305,3 +346,31 @@ 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_strbuf(int fd_in, struct strbuf *sb_out)
> +{
> +	int packet_len;
> +
> +	size_t orig_len = sb_out->len;
> +	size_t orig_alloc = sb_out->alloc;
> +
> +	for (;;) {
> +		strbuf_grow(sb_out, LARGE_PACKET_DATA_MAX);
> +		packet_len = packet_read(fd_in, NULL, NULL,
> +			// TODO: explain + 1

No // C99 comment please.

And I agree that the +1 needs to be explained.

> +			sb_out->buf + sb_out->len, LARGE_PACKET_DATA_MAX+1,
> +			PACKET_READ_GENTLE_ON_EOF);
> +		if (packet_len <= 0)
> +			break;

Hmph. So at the end of a data stream, we ask packet_read() to read 64kB or so, packet_read() gets the packet length by calling get_packet_data() and then another get_packet_data() reads that much and return. What happens during the next round? The first call to get_packet_data() in packet_read() will find that the stream has ended and returns -1, which is stored in packet_len here? But then the data is discarded after the loop when packet_len is negative.

I must be missing something. Is the other side always supposed to give a flush packet or something? Perhaps that is what is happening here. If so, I am OK with that, even though it somehow sounds a bit wasteful.