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

[Lars Schneider <larsxschneider@gmail.com>]

> On 26 Aug 2016, at 00:27, Junio C Hamano <gitster@pobox.com> wrote:
> 
> 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.

OK, what function names would be more clear from your point of view?

copy_to_packetized_stream_from_fd()
copy_to_packetized_stream_from_buf()
copy_to_packetized_stream_to_buf()

or

write_packetized_stream_from_fd()
write_packetized_stream_from_buf()
read_packetized_stream_to_buf()

?


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

OK. I'll remove the (I just realized meaningless) check in the other function:

+		if (bytes_to_write > sizeof(packet_write_buffer) - 4)
+			return COPY_WRITE_ERROR;

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

No.

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

Correct!

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

Good idea! I will refactor it that way!


Thanks a lot for the review,
Lars