On Thu, Sep 8, 2016 at 11:21 AM,  <larsxschnei...@gmail.com> wrote:
> From: Lars Schneider <larsxschnei...@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.

I presume we need both write_* things in a later patch; can you clarify why
we need both of them?

> +       if (paket_len < 0) {
> +               if (oldalloc == 0)
> +                       strbuf_release(sb_out);

So if old alloc is 0, we release it, which is documented as
 * Release a string buffer and the memory it used. You should not use the
 * string buffer after using this function, unless you initialize it again.

> +               else
> +                       strbuf_setlen(sb_out, oldlen);

Otherwise we just set the length back, such that it looks like before.

So as a caller the strbuf is in a different state in case of error
depending whether
the strbuf already had some data in it. I think it would be better if
we only did
`strbuf_setlen(sb_out, oldlen);` here, such that the caller can
strbuf_release it

Or to make things more confusing, you could use strbuf_reset in case of 0,
as that is a strbuf_setlen internally. ;)

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

Strictly speaking we read until a packet of size 0 appears, but as per
the implementation
of packet_read we cannot distinguish between "0000" and "0004", i.e.
an empty non-flush
packet. So I think we're fine both in the implementation as well as
the documentation here.

Reply via email to