Re: [PATCH v3 02/35] pkt-line: introduce struct packet_reader
From: Jonathan Nieder <hidden>
Date: 2018-02-13 00:49:47
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. [...]
quoted hunk ↗ jump to hunk
--- 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. [...]
quoted hunk ↗ jump to hunk
--- 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