> On 26 Aug 2016, at 00:27, Junio C Hamano <gits...@pobox.com> wrote:
> 
> larsxschnei...@gmail.com writes:
> 
>> From: Lars Schneider <larsxschnei...@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

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reply via email to