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

From: Brandon Williams <[email-masked]>
Date: Tue, 9 Jan 2018 11:28:01 -0800

On 01/09, Jonathan Tan wrote:
> 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".

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