Re: [PATCH v6 06/13] pkt-line: add functions to read/write flush terminated packet streams

[Junio C Hamano <gitster@pobox.com>]

larsxschneider@gmail.com writes:

> From: Lars Schneider <larsxschneider@gmail.com>
>
> packet_write_stream_with_flush_from_fd() and
> packet_write_stream_with_flush_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.
> packet_read_till_flush() reads arbitrary sized packets until it detects
> a `flush` packet.

These are awkwardly named and I couldn't guess what the input is (I
can tell one is to read from fd and the other is <mem,len> buffer,
but it is unclear if that is in packetized form or just raw data
stream to be copied to the end from their names) without reading the
implementation.  I _think_ you read a raw stream of data through the
end (either EOF or length limit) and write it out packetized, and
use the flush packet to mark the end of the stream.  In my mind,
that is "writing a packetized stream".  The words "packetizing" and
"stream" imply that the stream could consist of more data than what
would fit in a single packet, which in turn implies that there needs
a way to mark the end of one data item, so with_flush does not
necessarily have to be their names.

The counter-part would be "reading a packetized stream".

> +int packet_write_stream_with_flush_from_fd(int fd_in, int fd_out)
> +{

Especially this one I am tempted to suggest "copy-to-packetized-stream",
as it reads a stream from one fd and then copies out while packetizing.

> +	int err = 0;
> +	ssize_t bytes_to_write;
> +
> +	while (!err) {
> +		bytes_to_write = xread(fd_in, packet_write_buffer, sizeof(packet_write_buffer) - 4);
> +		if (bytes_to_write < 0)
> +			return COPY_READ_ERROR;
> +		if (bytes_to_write == 0)
> +			break;
> +		if (bytes_to_write > sizeof(packet_write_buffer) - 4)
> +			return COPY_WRITE_ERROR;

... and you seem to agree with me by using COPY here.

> +		err = packet_write_gently(fd_out, packet_write_buffer, bytes_to_write);
> +	}
> +	if (!err)
> +		err = packet_flush_gently(fd_out);
> +	return err;
> +}
> +
> +int packet_write_stream_with_flush_from_buf(const char *src_in, size_t len, int fd_out)
> +{
> +	int err = 0;
> +	size_t bytes_written = 0;
> +	size_t bytes_to_write;
> +
> +	while (!err) {
> +		if ((len - bytes_written) > sizeof(packet_write_buffer) - 4)
> +			bytes_to_write = sizeof(packet_write_buffer) - 4;
> +		else
> +			bytes_to_write = len - bytes_written;
> +		if (bytes_to_write == 0)
> +			break;

The lack of COPY_WRITE_ERROR puzzled me briefly here.  If you are
assuming that your math at the beginning of this loop is correct and
bytes_to_write will never exceed the write-buffer size, I think you
should be able to (and it would be better to) assume that the math
you do to tell xread() up to how many bytes it is allowed to read at
once is also correct, losing the COPY_WRITE_ERROR check in the other
function.  You can choose to play safer and do a check in this
function, too.  Either way, we would want to be consistent.

> +		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;
> +}

> +ssize_t packet_read_till_flush(int fd_in, struct strbuf *sb_out)
> +{
> +	int len, ret;
> +	int options = PACKET_READ_GENTLE_ON_EOF;
> +	char linelen[4];
> +
> +	size_t oldlen = sb_out->len;
> +	size_t oldalloc = sb_out->alloc;
> +
> +	for (;;) {
> +		/* Read packet header */
> +		ret = get_packet_data(fd_in, NULL, NULL, linelen, 4, options);
> +		if (ret < 0)
> +			goto done;
> +		len = packet_length(linelen);
> +		if (len < 0)
> +			die("protocol error: bad line length character: %.4s", linelen);
> +		if (!len) {
> +			/* Found a flush packet - Done! */
> +			packet_trace("0000", 4, 0);
> +			break;
> +		}
> +		len -= 4;
> +
> +		/* Read packet content */
> +		strbuf_grow(sb_out, len);
> +		ret = get_packet_data(fd_in, NULL, NULL, sb_out->buf + sb_out->len, len, options);
> +		if (ret < 0)
> +			goto done;
> +		if (ret != len) {
> +			error("protocol error: incomplete read (expected %d, got %d)", len, ret);
> +			goto done;
> +		}
> +
> +		packet_trace(sb_out->buf + sb_out->len, len, 0);

All of the above seems to pretty much duplicate the logic in
packet_read(), except that this user does not need options handling
it has.  Is optimizing that out the reason why you open-coded it
here?

Or is it because you cannot tell if you got a truly empty packet or
you got a flush from outside packet_read(), and you wanted to make
sure that you won't be fooled by a normal packet with 0-length
payload?

If the latter is the reason, it may be a viable alternative to
update packet_read() to take PACKET_READ_IGNORE_EMPTY_PACKET, i.e. a
new bit in its options parameter, so that a normal packet with
0-length payload is simply ignored there (i.e. even without
returning, packet_read() would repeat from the beginning when it got
such a packet).  That way, the above would become 

	strbuf_grow(); /* enough to hold max-packet-len more bytes */
	len = packet_read();
        if (!len)
        	/* we cannot get 0 unless we see flush */
                break;

which may be a lot cleaner?

> +		sb_out->len += len;
> +	}
> +
> +done:
> +	if (ret < 0) {
> +		if (oldalloc == 0)
> +			strbuf_release(sb_out);
> +		else
> +			strbuf_setlen(sb_out, oldlen);
> +		return ret;  /* unexpected EOF */
> +	}
> +	return sb_out->len - oldlen;
> +}
> diff --git a/pkt-line.h b/pkt-line.h
> index 3fa0899..9616117 100644
> --- a/pkt-line.h
> +++ b/pkt-line.h
> @@ -25,6 +25,8 @@ void packet_buf_flush(struct strbuf *buf);
>  void packet_buf_write(struct strbuf *buf, const char *fmt, ...) __attribute__((format (printf, 2, 3)));
>  int packet_flush_gently(int fd);
>  int packet_write_fmt_gently(int fd, const char *fmt, ...) __attribute__((format (printf, 2, 3)));
> +int packet_write_stream_with_flush_from_fd(int fd_in, int fd_out);
> +int packet_write_stream_with_flush_from_buf(const char *src_in, size_t len, int fd_out);
>  
>  /*
>   * Read a packetized line into the buffer, which must be at least size bytes
> @@ -77,6 +79,11 @@ char *packet_read_line(int fd, int *size);
>   */
>  char *packet_read_line_buf(char **src_buf, size_t *src_len, int *size);
>  
> +/*
> + * Reads a stream of variable sized packets until a flush packet is detected.
> + */
> +ssize_t packet_read_till_flush(int fd_in, struct strbuf *sb_out);
> +
>  #define DEFAULT_PACKET_MAX 1000
>  #define LARGE_PACKET_MAX 65520
>  extern char packet_buffer[LARGE_PACKET_MAX];