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

From: Stefan Beller <sbeller@google.com>
Date: Sep 8, 2016, 21:49 UTC
Message-ID: <CAGZ79kYHX6-hhKW_6JEd1hBNi8R+sBY-RjZARfHDMnyLKFZnsQ@mail.gmail.com>
In-Reply-To: <20160908182132.50788-7-larsxschneider@gmail.com>

On Thu, Sep 8, 2016 at 11:21 AM,  <larsxschneider@gmail.com> wrote:

Show 6 quoted lines

> From: Lars Schneider <larsxschneider@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 unconditionally.

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

Show 6 quoted lines

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