Re: [PATCH v3 02/35] pkt-line: introduce struct packet_reader

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Feb 13, 2018, 00:49 UTC
Message-ID: <20180213004937.GB42272@aiede.svl.corp.google.com>
In-Reply-To: <20180207011312.189834-3-bmwill@google.com>

Hi,

Brandon Williams wrote:

> Subject: pkt-line: introduce struct packet_reader

nit: this subject line doesn't describe what the purpose/intent behind the patch is. Maybe something like

	pkt-line: allow peeking at a packet line without consuming it

would make it clearer.

Show 7 quoted lines

> Sometimes it is advantageous to be able to peek the next packet line
> without consuming it (e.g. to be able to determine the protocol version
> a server is speaking).  In order to do that introduce 'struct
> packet_reader' which is an abstraction around the normal packet reading
> logic.  This enables a caller to be able to peek a single line at a time
> using 'packet_reader_peek()' and having a caller consume a line by
> calling 'packet_reader_read()'.

Makes sense. The packet_reader owns a buffer to support the peek operation and make buffer reuse a little easier.

[...]

Show 13 quoted lines

> --- a/pkt-line.h
> +++ b/pkt-line.h
> @@ -111,6 +111,64 @@ char *packet_read_line_buf(char **src_buf, size_t *src_len, int *size);
>   */
>  ssize_t read_packetized_to_strbuf(int fd_in, struct strbuf *sb_out);
>  
> +struct packet_reader {
> +	/* source file descriptor */
> +	int fd;
> +
> +	/* source buffer and its size */
> +	char *src_buffer;
> +	size_t src_len;

Can or should this be a strbuf?

> +
> +	/* buffer that pkt-lines are read into and its size */
> +	char *buffer;
> +	unsigned buffer_size;

Likewise.

> +
> +	/* options to be used during reads */
> +	int options;

What option values are possible?

> +
> +	/* status of the last read */
> +	enum packet_read_status status;

This reminds me of FILE*'s status value, ferror, etc. I'm mildly nervous about it --- it encourages a style of error handling where you ignore errors from an individual operation and hope the recorded status later has the most relevant error.

I think it is being used to support peek --- you need to record the error to reply it. Is that right? I wonder if it would make sense to structure as

		struct packet_read_result last_line_read;
	};

	struct packet_read_result {
		enum packet_read_status status;

		const char *line;
		int len;
	};

What you have here also seems fine. I think what would help most for readability is if the comment mentioned the purpose --- e.g.

	/* status of the last read, to support peeking */

Or if the contract were tied to the purpose:

	/* status of the last read, only valid if line_peeked is true */

[...]

Show 7 quoted lines

> +/*
> + * Initialize a 'struct packet_reader' object which is an
> + * abstraction around the 'packet_read_with_status()' function.
> + */
> +extern void packet_reader_init(struct packet_reader *reader, int fd,
> +			       char *src_buffer, size_t src_len,
> +			       int options);

This comment doesn't describe how I should use the function. Is the intent to point the reader to packet_read_with_status for more details about the arguments?

Can src_buffer be a const char *?

[...]

> +/*
> + * Perform a packet read and return the status of the read.

nit: s/Perform a packet read/Read one pkt-line/

Show 9 quoted lines

> + * The values of 'pktlen' and 'line' are updated based on the status of the
> + * read as follows:
> + *
> + * PACKET_READ_ERROR: 'pktlen' is set to '-1' and 'line' is set to NULL
> + * PACKET_READ_NORMAL: 'pktlen' is set to the number of bytes read
> + *		       'line' is set to point at the read line
> + * PACKET_READ_FLUSH: 'pktlen' is set to '0' and 'line' is set to NULL
> + */
> +extern enum packet_read_status packet_reader_read(struct packet_reader *reader);

This is reasonable. As described above an alternative would be possible to have a separate packet_read_result output parameter but the interface described here looks pretty easy/pleasant to use.

> +
> +/*
> + * Peek the next packet line without consuming it and return the status.

nit: s/Peek/Peek at/, or s/Peek/Read/

> + * The next call to 'packet_reader_read()' will perform a read of the same line
> + * that was peeked, consuming the line.

nit: s/peeked/peeked at/

Show 5 quoted lines

> + *
> + * Peeking multiple times without calling 'packet_reader_read()' will return
> + * the same result.
> + */
> +extern enum packet_read_status packet_reader_peek(struct packet_reader *reader);

Nice.

[...]

Show 11 quoted lines

> --- a/pkt-line.c
> +++ b/pkt-line.c
> @@ -406,3 +406,62 @@ ssize_t read_packetized_to_strbuf(int fd_in, struct strbuf *sb_out)
>  	}
>  	return sb_out->len - orig_len;
>  }
> +
> +/* Packet Reader Functions */
> +void packet_reader_init(struct packet_reader *reader, int fd,
> +			char *src_buffer, size_t src_len,
> +			int options)

This comment looks like it's attached to packet_reader_init, but it's meant to be attached to the whole collection. It's possible that this title-above-multiple-functions won't be maintained, but that's okay.

Show 8 quoted lines

> +{
> +	memset(reader, 0, sizeof(*reader));
> +
> +	reader->fd = fd;
> +	reader->src_buffer = src_buffer;
> +	reader->src_len = src_len;
> +	reader->buffer = packet_buffer;
> +	reader->buffer_size = sizeof(packet_buffer);

Looks like this is very non-reentrant. Can the doc comment warn about that? Or even better, can it be made reentrant by owning its own strbuf?

Show 9 quoted lines

> +	reader->options = options;
> +}
> +
> +enum packet_read_status packet_reader_read(struct packet_reader *reader)
> +{
> +	if (reader->line_peeked) {
> +		reader->line_peeked = 0;
> +		return reader->status;
> +	}

Nice.

Show 29 quoted lines

> +
> +	reader->status = packet_read_with_status(reader->fd,
> +						 &reader->src_buffer,
> +						 &reader->src_len,
> +						 reader->buffer,
> +						 reader->buffer_size,
> +						 &reader->pktlen,
> +						 reader->options);
> +
> +	switch (reader->status) {
> +	case PACKET_READ_EOF:
> +		reader->pktlen = -1;
> +		reader->line = NULL;
> +		break;
> +	case PACKET_READ_NORMAL:
> +		reader->line = reader->buffer;
> +		break;
> +	case PACKET_READ_FLUSH:
> +		reader->pktlen = 0;
> +		reader->line = NULL;
> +		break;
> +	}
> +
> +	return reader->status;
> +}
> +
> +enum packet_read_status packet_reader_peek(struct packet_reader *reader)
> +{
> +	/* Only allow peeking a single line */

nit: s/peeking at/

> +	if (reader->line_peeked)
> +		return reader->status;
> +
> +	/* Peek a line by reading it and setting peeked flag */

nit: s/Peek/Peek at/

> +	packet_reader_read(reader);
> +	reader->line_peeked = 1;
> +	return reader->status;
> +}

Thanks for a pleasant read.

Jonathan