Subject: Re: [PATCH 01/26] pkt-line: introduce packet_read_with_status

From: Jonathan Tan <[email-masked]>
Date: Tue, 9 Jan 2018 10:04:11 -0800

On Tue,  2 Jan 2018 16:18:03 -0800
Brandon Williams <[email-masked]> 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".

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')