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.
> 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.
[...]
> --- 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 */
[...]
> +/*
> + * 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/
> + * 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/
> + *
> + * 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.
[...]
> --- 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.
> +{
> + 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?
> + 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.
> +
> + 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
