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
strbuf_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.
Signed-off-by: Jeff King
---
This adds two options to the generic packet_read interface for which
many callers will just pass (NULL, 0). We can hide that behind a
wrapper, but I was annoyed with the proliferation of wrappers from the
last round. Pick your poison.
connect.c | 3 ++-
daemon.c | 2 +-
pkt-line.c| 77 ++-
pkt-line.h| 23 +-
remote-curl.c | 22 -
sideband.c| 2 +-
6 files changed, 70 insertions(+), 59 deletions(-)
diff --git a/connect.c b/connect.c
index 611ffb4..061aa5b 100644
--- a/connect.c
+++ b/connect.c
@@ -76,7 +76,8 @@ struct ref **get_remote_heads(int in, struct ref **list,
int len, name_len;
char *buffer = packet_buffer;
- len = packet_read(in, packet_buffer, sizeof(packet_buffer),
+ len = packet_read(in, NULL, 0,
+ packet_buffer, sizeof(packet_buffer),
PACKET_READ_GENTLE_ON_EOF |
PACKET_READ_CHOMP_NEWLINE);
if (len < 0)
diff --git a/daemon.c b/daemon.c
index 3f70e79..9a241d9 100644
--- a/daemon.c
+++ b/daemon.c
@@ -612,7 +612,7 @@ static int execute(void)
loginfo("Connection from %s:%s", addr, port);
alarm(init_timeout ? init_timeout : timeout);
- pktlen = packet_read(0, packet_buffer, sizeof(packet_buffer), 0);
+ pktlen = packet_read(0, NULL, 0, packet_buffer, sizeof(packet_buffer),
0);
alarm(0);
len = strlen(line);
diff --git a/pkt-line.c b/pkt-line.c
index 55fb688..2c47052 100644
--- a/pkt-line.c
+++ b/pkt-line.c
@@ -104,12 +104,29 @@ static int safe_read(int fd, void *buffer, unsigned size,
int options)
strbuf_add(buf, buffer, n);
}
-static int safe_read(int fd, void *buffer, unsigned size, int options)
+static int get_packet_data(int fd, char **src_buf, size_t *src_size,
+ void *dst, unsigned size, int options)
{
- ssize_t ret = read_in_full(fd, buffer, size);
- if (ret < 0)
- die_errno("read error");
- else if (ret < size) {
+ ssize_t ret;
+
+ if (fd >= 0 && src_buf && *src_buf)
+ die("BUG: multiple sources given to packet_read");
+
+ /* Read up to "size" bytes from our source, whatever it is. */
+ if (src_buf && *src_buf) {
+ ret = size < *src_size ? size : *src_size;
+ memcpy(dst, *src_buf, ret);
+ *src_buf += size;
+ *src_size -= size;
+ }
+ else {
+ ret = read_in_full(fd, dst, size);
+ if (ret < 0)
+ die_errno("read error");
+ }
+
+
