Thread (329 messages) 329 messages, 12 authors, 2018-03-14

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

From: Jonathan Tan <hidden>
Date: 2018-03-13 19:35:51

On Wed, 28 Feb 2018 15:22:18 -0800
Brandon Williams [off-list ref] 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).
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 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/20180213002554.GA42272@aiede.svl.corp.google.com/
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help