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

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 25, 2016, 22:27 UTC
Message-ID: <xmqqzio0jxh7.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20160825110752.31581-7-larsxschneider@gmail.com>

larsxschneider@gmail.com writes:

Show 8 quoted lines

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

Show 11 quoted lines

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

Show 20 quoted lines

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

Show 7 quoted lines

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

Show 35 quoted lines

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

Show 38 quoted lines

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