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

From: Brandon Williams <[email-masked]>
Date: Tue, 13 Mar 2018 12:52:20 -0700

On 03/13, Jonathan Tan wrote:
> On Wed, 28 Feb 2018 15:22:18 -0800
> Brandon Williams <[email-masked]> wrote:
> 
> > +	if (len < 0) {
> >  		die("protocol error: bad line length character: %.4s", linelen);
> > -	if (!len) {
> > +	} else if (!len) {
> >  		packet_trace("0000", 4, 0);
> > -		return 0;
> > +		return PACKET_READ_FLUSH;
> > +	} else if (len < 4) {
> > +		die("protocol error: bad line length %d", len);
> >  	}
> > +
> >  	len -= 4;
> > -	if (len >= size)
> > +	if ((unsigned)len >= size)
> >  		die("protocol error: bad line length %d", len);
> 
> The cast to unsigned is safe, since len was at least 4 before "len -=
> 4". I can't think of a better way to write this to make that more
> obvious, though.
> 
> > +/*
> > + * Read a packetized line into a buffer like the 'packet_read()' function but
> > + * returns an 'enum packet_read_status' which indicates the status of the read.
> > + * The number of bytes read will be assigined to *pktlen if the status of the
> > + * read was 'PACKET_READ_NORMAL'.
> > + */
> > +enum packet_read_status {
> > +	PACKET_READ_EOF = -1,
> > +	PACKET_READ_NORMAL,
> > +	PACKET_READ_FLUSH,
> > +};
> > +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);
> 
> jrnieder said in [1], referring to the definition of enum
> packet_read_status:
> 
> > nit: do any callers treat the return value as a number?  It would be
> > less magical if the numbering were left to the compiler (0, 1, 2).

yeah i'll do this.

> 
> I checked the result of the entire patch set and the only callers seem
> to be packet_read() (modified in this patch) and the
> soon-to-be-introduced packet_reader_read(). So not only can the
> numbering be left to the compiler, this function can (and should) be
> marked static as well (and the enum definition moved to .c), since I
> think that future development should be encouraged to use packet_reader.

The enum definition can't be moved as its needed outside this file.

> 
> The commit message would also thus need to be rewritten, since this
> becomes more of a refactoring into a function with a more precisely
> specified return type, to be used both by the existing packet_read() and
> a soon-to-be-introduced packet_reader_read().
> 
> [1] https://public-inbox.org/git/[email-masked]/

-- 
Brandon Williams