On 01/09, Jonathan Tan wrote:
> On Tue, 2 Jan 2018 16:18:03 -0800
> Brandon Williams <bmw...@google.com> wrote:
>
> > -int packet_read(int fd, char **src_buf, size_t *src_len,
> > - char *buffer, unsigned size, int options)
> > +enum packet_read_status packet_read_with_status(int fd, char **src_buffer,
> > size_t *src_len,
> > + char *buffer, unsigned size,
> > int *pktlen,
> > + int options)
> > {
> > - int len, ret;
> > + int len;
> > char linelen[4];
> >
> > - ret = get_packet_data(fd, src_buf, src_len, linelen, 4, options);
> > - if (ret < 0)
> > - return ret;
> > + if (get_packet_data(fd, src_buffer, src_len, linelen, 4, options) < 0)
> > + return PACKET_READ_EOF;
> > +
> > len = packet_length(linelen);
> > if (len < 0)
> > die("protocol error: bad line length character: %.4s", linelen);
> > - if (!len) {
> > +
> > + if (len == 0) {
>
> This change (replacing "!len" with "len == 0") is unnecessary, I think.
>
> > packet_trace("0000", 4, 0);
> > - return 0;
> > + return PACKET_READ_FLUSH;
> > + } else if (len >= 1 && len <= 3) {
> > + die("protocol error: bad line length character: %.4s", linelen);
> > }
>
> This seems to be more of a "bad line length" than a "bad line length
> character".
I'll make these changes, though I do think this needs to stay as a "bad
line length character" as the len could be neg which is an indication of
parsing the linelen character failed.
>
> Also, some of the checks are redundant. Above, it is probably better to
> delete "len >= 1", and optionally write "len < 4" instead of "len <= 3"
> (to emphasize that the subtraction of 4 below does not result in a
> negative value).
>
> > +
> > len -= 4;
> > - if (len >= size)
> > + if ((len < 0) || ((unsigned)len >= size))
> > die("protocol error: bad line length %d", len);
>
> The "len < 0" check is redundant.
>
> > - ret = get_packet_data(fd, src_buf, src_len, buffer, len, options);
> > - if (ret < 0)
> > - return ret;
> > +
> > + if (get_packet_data(fd, src_buffer, src_len, buffer, len, options) < 0)
> > + return PACKET_READ_EOF;
> >
> > if ((options & PACKET_READ_CHOMP_NEWLINE) &&
> > len && buffer[len-1] == '\n')
--
Brandon Williams
