Re: [PATCH 01/26] pkt-line: introduce packet_read_with_status
- From
- Jonathan Tan <jonathantanmy@google.com>
- Date
- Jan 9, 2018, 18:04 UTC
- Message-ID
- <20180109100411.b63dd903e2037372e4a28a91@google.com>
- In-Reply-To
- <20180103001828.205012-2-bmwill@google.com>
On Tue, 2 Jan 2018 16:18:03 -0800 Brandon Williams <bmwill@google.com> wrote:
Show 22 quoted lines
> -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.
Show 6 quoted lines
> 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".
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).
Show 5 quoted lines
> +
> 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.
Show 9 quoted lines
> - 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')