@matheusbsbs / matheus-bagliano / commits / 4981fe750b

pkt-line: share buffer/descriptor reading implementation

The packet_read function reads from a descriptor. The packet_get_line function is similar, but reads from an in-memory buffer, and uses a completely separate implementation. This patch teaches the generic packet_read function to accept either source, and we can do away with packet_get_line's implementation. There are two other differences to account for between the old and new functions. The first is that we used to read into a strbuf, but now read into a fixed size buffer. The only two callers are fine with that, and in fact it simplifies their code, since they can use the same static-buffer interface as the rest of the packet_read_line callers (and we provide a similar convenience wrapper for reading from a buffer rather than a descriptor). This is technically an externally-visible behavior change in that we used to accept arbitrary sized packets up to 65532 bytes, and now cap out at LARGE_PACKET_MAX, 65520. In practice this doesn't matter, as we use it only for parsing smart-http headers (of which there is exactly one defined, and it is small and fixed-size). And any extension headers would be breaking the protocol to go over LARGE_PACKET_MAX anyway. The other difference is that packet_get_line would return on error rather than dying. However, both callers of packet_get_line are actually improved by dying. The first caller does its own error checking, but we can drop that; as a result, we'll actually get more specific reporting about protocol breakage when packet_read dies internally. The only downside is that packet_read will not print the smart-http URL that failed, but that's not a big deal; anybody not debugging can already see the remote's URL already, and anybody debugging would want to run with GIT_CURL_VERBOSE anyway to see way more information. The second caller, which is just trying to skip past any extra smart-http headers (of which there are none defined, but which we allow to keep room for future expansion), did not error check at all. As a result, it would treat an error just like a flush packet. The resulting mess would generally cause an error later in get_remote_heads, but now we get error reporting much closer to the source of the problem. Brown-paper-bag-fixes-by: Ramsay Jones <ramsay@ramsay1.demon.co.uk> Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Jeff King committed Feb 23, 2013 at 17:31 UTC 4981fe750b1fc58bfdf5b9ca9843f4f505b9bb4d

6 files changed +69 -59

connect.c

+2 -1

@@ -76,7 +76,8 @@ struct ref **get_remote_heads(int in, struct ref **list,

76 int len, name_len;

77 char *buffer = packet_buffer;

79 - len = packet_read(in, packet_buffer, sizeof(packet_buffer),

79 + len = packet_read(in, NULL, NULL,

80 + packet_buffer, sizeof(packet_buffer),

81 PACKET_READ_GENTLE_ON_EOF |

82 PACKET_READ_CHOMP_NEWLINE);

83 if (len < 0)

daemon.c

+1 -1

@@ -612,7 +612,7 @@ static int execute(void)

612 loginfo("Connection from %s:%s", addr, port);

613

614 alarm(init_timeout ? init_timeout : timeout);

615 - pktlen = packet_read(0, packet_buffer, sizeof(packet_buffer), 0);

615 + pktlen = packet_read(0, NULL, NULL, packet_buffer, sizeof(packet_buffer), 0);

616 alarm(0);

617

618 len = strlen(line);

pkt-line.c

+38 -38

@@ -104,12 +104,28 @@ void packet_buf_write(struct strbuf *buf, const char *fmt, ...)

104 strbuf_add(buf, buffer, n);

105 }

106

107 -static int safe_read(int fd, void *buffer, unsigned size, int options)

107 +static int get_packet_data(int fd, char **src_buf, size_t *src_size,

108 + void *dst, unsigned size, int options)

109 {

109 - ssize_t ret = read_in_full(fd, buffer, size);

110 - if (ret < 0)

111 - die_errno("read error");

112 - else if (ret < size) {

110 + ssize_t ret;

111 +

112 + if (fd >= 0 && src_buf && *src_buf)

113 + die("BUG: multiple sources given to packet_read");

114 +

115 + /* Read up to "size" bytes from our source, whatever it is. */

116 + if (src_buf && *src_buf) {

117 + ret = size < *src_size ? size : *src_size;

118 + memcpy(dst, *src_buf, ret);

119 + *src_buf += ret;

120 + *src_size -= ret;

121 + } else {

122 + ret = read_in_full(fd, dst, size);

123 + if (ret < 0)

124 + die_errno("read error");

125 + }

126 +

127 + /* And complain if we didn't get enough bytes to satisfy the read. */

128 + if (ret < size) {

129 if (options & PACKET_READ_GENTLE_ON_EOF)

130 return -1;

131

@@ -144,12 +160,13 @@ static int packet_length(const char *linelen)

160 return len;

161 }

162

147 -int packet_read(int fd, char *buffer, unsigned size, int options)

163 +int packet_read(int fd, char **src_buf, size_t *src_len,

164 + char *buffer, unsigned size, int options)

165 {

166 int len, ret;

167 char linelen[4];

168

152 - ret = safe_read(fd, linelen, 4, options);

169 + ret = get_packet_data(fd, src_buf, src_len, linelen, 4, options);

170 if (ret < 0)

171 return ret;

172 len = packet_length(linelen);

@@ -162,7 +179,7 @@ int packet_read(int fd, char *buffer, unsigned size, int options)

179 len -= 4;

180 if (len >= size)

181 die("protocol error: bad line length %d", len);

165 - ret = safe_read(fd, buffer, len, options);

182 + ret = get_packet_data(fd, src_buf, src_len, buffer, len, options);

183 if (ret < 0)

184 return ret;

185

@@ -175,41 +192,24 @@ int packet_read(int fd, char *buffer, unsigned size, int options)

192 return len;

193 }

194

178 -char *packet_read_line(int fd, int *len_p)

195 +static char *packet_read_line_generic(int fd,

196 + char **src, size_t *src_len,

197 + int *dst_len)

198 {

180 - int len = packet_read(fd, packet_buffer, sizeof(packet_buffer),

199 + int len = packet_read(fd, src, src_len,

200 + packet_buffer, sizeof(packet_buffer),

201 PACKET_READ_CHOMP_NEWLINE);

182 - if (len_p)

183 - *len_p = len;

202 + if (dst_len)

203 + *dst_len = len;

204 return len ? packet_buffer : NULL;

205 }

206

187 -int packet_get_line(struct strbuf *out,

188 - char **src_buf, size_t *src_len)

207 +char *packet_read_line(int fd, int *len_p)

208 {

190 - int len;

191 -

192 - if (*src_len < 4)

193 - return -1;

194 - len = packet_length(*src_buf);

195 - if (len < 0)

196 - return -1;

197 - if (!len) {

198 - *src_buf += 4;

199 - *src_len -= 4;

200 - packet_trace("0000", 4, 0);

201 - return 0;

202 - }

203 - if (*src_len < len)

204 - return -2;

205 -

206 - *src_buf += 4;

207 - *src_len -= 4;

208 - len -= 4;

209 + return packet_read_line_generic(fd, NULL, NULL, len_p);

210 +}

211

210 - strbuf_add(out, *src_buf, len);

211 - *src_buf += len;

212 - *src_len -= len;

213 - packet_trace(out->buf, out->len, 0);

214 - return len;

212 +char *packet_read_line_buf(char **src, size_t *src_len, int *dst_len)

213 +{

214 + return packet_read_line_generic(-1, src, src_len, dst_len);

215 }

pkt-line.h

+17 -6

@@ -25,9 +25,16 @@ void packet_buf_flush(struct strbuf *buf);

25 void packet_buf_write(struct strbuf *buf, const char *fmt, ...) __attribute__((format (printf, 2, 3)));

27 /*

28 - * Read a packetized line from the descriptor into the buffer, which must be at

29 - * least size bytes long. The return value specifies the number of bytes read

30 - * into the buffer.

28 + * Read a packetized line into the buffer, which must be at least size bytes

29 + * long. The return value specifies the number of bytes read into the buffer.

30 + *

31 + * If src_buffer is not NULL (and nor is *src_buffer), it should point to a

32 + * buffer containing the packet data to parse, of at least *src_len bytes.

33 + * After the function returns, src_buf will be incremented and src_len

34 + * decremented by the number of bytes consumed.

35 + *

36 + * If src_buffer (or *src_buffer) is NULL, then data is read from the

37 + * descriptor "fd".

38 *

39 * If options does not contain PACKET_READ_GENTLE_ON_EOF, we will die under any

40 * of the following conditions:

@@ -50,7 +57,8 @@ void packet_buf_write(struct strbuf *buf, const char *fmt, ...) __attribute__((f

57 */

58 #define PACKET_READ_GENTLE_ON_EOF (1u<<0)

59 #define PACKET_READ_CHOMP_NEWLINE (1u<<1)

53 -int packet_read(int fd, char *buffer, unsigned size, int options);

60 +int packet_read(int fd, char **src_buffer, size_t *src_len, char

61 + *buffer, unsigned size, int options);

63 /*

64 * Convenience wrapper for packet_read that is not gentle, and sets the

@@ -61,11 +69,14 @@ int packet_read(int fd, char *buffer, unsigned size, int options);

69 */

70 char *packet_read_line(int fd, int *size);

72 +/*

73 + * Same as packet_read_line, but read from a buf rather than a descriptor;

74 + * see packet_read for details on how src_* is used.

75 + */

76 +char *packet_read_line_buf(char **src_buf, size_t *src_len, int *size);

78 #define DEFAULT_PACKET_MAX 1000

79 #define LARGE_PACKET_MAX 65520

80 extern char packet_buffer[LARGE_PACKET_MAX];

69 -int packet_get_line(struct strbuf *out, char **src_buf, size_t *src_len);

70 -

82 #endif

remote-curl.c

+10 -12

@@ -138,28 +138,26 @@ static struct discovery* discover_refs(const char *service)

138 if (maybe_smart &&

139 (5 <= last->len && last->buf[4] == '#') &&

140 !strbuf_cmp(&exp, &type)) {

141 + char *line;

142 +

143 /*

144 * smart HTTP response; validate that the service

145 * pkt-line matches our request.

146 */

145 - if (packet_get_line(&buffer, &last->buf, &last->len) <= 0)

146 - die("%s has invalid packet header", refs_url);

147 - if (buffer.len && buffer.buf[buffer.len - 1] == '\n')

148 - strbuf_setlen(&buffer, buffer.len - 1);

147 + line = packet_read_line_buf(&last->buf, &last->len, NULL);

148

149 strbuf_reset(&exp);

150 strbuf_addf(&exp, "# service=%s", service);

152 - if (strbuf_cmp(&exp, &buffer))

153 - die("invalid server response; got '%s'", buffer.buf);

151 + if (strcmp(line, exp.buf))

152 + die("invalid server response; got '%s'", line);

153 strbuf_release(&exp);

154

155 /* The header can include additional metadata lines, up

156 * until a packet flush marker. Ignore these now, but

157 * in the future we might start to scan them.

158 */

160 - strbuf_reset(&buffer);

161 - while (packet_get_line(&buffer, &last->buf, &last->len) > 0)

162 - strbuf_reset(&buffer);

159 + while (packet_read_line_buf(&last->buf, &last->len, NULL))

160 + ;

161

162 last->proto_git = 1;

163 }

@@ -308,7 +306,7 @@ static size_t rpc_out(void *ptr, size_t eltsize,

306

307 if (!avail) {

308 rpc->initial_buffer = 0;

311 - avail = packet_read(rpc->out, rpc->buf, rpc->alloc, 0);

309 + avail = packet_read(rpc->out, NULL, NULL, rpc->buf, rpc->alloc, 0);

310 if (!avail)

311 return 0;

312 rpc->pos = 0;

@@ -425,7 +423,7 @@ static int post_rpc(struct rpc_state *rpc)

423 break;

424 }

425

428 - n = packet_read(rpc->out, buf, left, 0);

426 + n = packet_read(rpc->out, NULL, NULL, buf, left, 0);

427 if (!n)

428 break;

429 rpc->len += n;

@@ -579,7 +577,7 @@ static int rpc_service(struct rpc_state *rpc, struct discovery *heads)

577 rpc->hdr_accept = strbuf_detach(&buf, NULL);

578

579 while (!err) {

582 - int n = packet_read(rpc->out, rpc->buf, rpc->alloc, 0);

580 + int n = packet_read(rpc->out, NULL, NULL, rpc->buf, rpc->alloc, 0);

581 if (!n)

582 break;

583 rpc->pos = 0;

sideband.c

+1 -1

@@ -38,7 +38,7 @@ int recv_sideband(const char *me, int in_stream, int out)

39 while (1) {

40 int band, len;

41 - len = packet_read(in_stream, buf + pf, LARGE_PACKET_MAX, 0);

41 + len = packet_read(in_stream, NULL, NULL, buf + pf, LARGE_PACKET_MAX, 0);

42 if (len == 0)

43 break;

44 if (len < 1) {