[PATCH v3 01/12] pkt-line: eliminate the need for static buffer in packet_write_gently()

From: "Jeff Hostetler via GitGitGadget" <gitgitgadget@gmail.com>
To: git@vger.kernel.org
Cc: "Jeff Hostetler" <git@jeffhostetler.com>,
	"Jeff King" <peff@peff.net>,
	"SZEDER Gábor" <szeder.dev@gmail.com>,
	"Johannes Schindelin" <Johannes.Schindelin@gmx.de>,
	"Jeff Hostetler" <jeffhost@microsoft.com>,
	"Jeff Hostetler" <jeffhost@microsoft.com>
Subject: [PATCH v3 01/12] pkt-line: eliminate the need for static buffer in packet_write_gently()
Date: Sat, 13 Feb 2021 00:09:02 +0000	[thread overview]
Message-ID: <2d6858b1625aa3c96688c6c6a9157c2d2b16f43e.1613174954.git.gitgitgadget@gmail.com> (raw)
In-Reply-To: <pull.766.v3.git.1613174954.gitgitgadget@gmail.com>

From: Jeff Hostetler <jeffhost@microsoft.com>

Teach `packet_write_gently()` to write the pkt-line header and the actual
buffer in 2 separate calls to `write_in_full()` and avoid the need for a
static buffer, thread-safe scratch space, or an excessively large stack
buffer.

Change the API of `write_packetized_from_fd()` to accept a scratch space
argument from its caller to avoid similar issues here.

These changes are intended to make it easier to use pkt-line routines in
a multi-threaded context with multiple concurrent writers writing to
different streams.

Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
---
 convert.c  |  7 ++++---
 pkt-line.c | 28 +++++++++++++++++++---------
 pkt-line.h | 12 +++++++++---
 3 files changed, 32 insertions(+), 15 deletions(-)

diff --git a/convert.c b/convert.c
index ee360c2f07ce..41012c2d301c 100644
--- a/convert.c
+++ b/convert.c
@@ -883,9 +883,10 @@ static int apply_multi_file_filter(const char *path, const char *src, size_t len
 	if (err)
 		goto done;
 
-	if (fd >= 0)
-		err = write_packetized_from_fd(fd, process->in);
-	else
+	if (fd >= 0) {
+		struct packet_scratch_space scratch;
+		err = write_packetized_from_fd(fd, process->in, &scratch);
+	} else
 		err = write_packetized_from_buf(src, len, process->in);
 	if (err)
 		goto done;
diff --git a/pkt-line.c b/pkt-line.c
index d633005ef746..4cff2f7a68a5 100644
--- a/pkt-line.c
+++ b/pkt-line.c
@@ -196,17 +196,25 @@ int packet_write_fmt_gently(int fd, const char *fmt, ...)
 
 static int packet_write_gently(const int fd_out, const char *buf, size_t size)
 {
-	static char packet_write_buffer[LARGE_PACKET_MAX];
+	char header[4];
 	size_t packet_size;
 
-	if (size > sizeof(packet_write_buffer) - 4)
+	if (size > LARGE_PACKET_DATA_MAX)
 		return error(_("packet write failed - data exceeds max packet size"));
 
 	packet_trace(buf, size, 1);
 	packet_size = size + 4;
-	set_packet_header(packet_write_buffer, packet_size);
-	memcpy(packet_write_buffer + 4, buf, size);
-	if (write_in_full(fd_out, packet_write_buffer, packet_size) < 0)
+
+	set_packet_header(header, packet_size);
+
+	/*
+	 * Write the header and the buffer in 2 parts so that we do not need
+	 * to allocate a buffer or rely on a static buffer.  This avoids perf
+	 * and multi-threading issues.
+	 */
+
+	if (write_in_full(fd_out, header, 4) < 0 ||
+	    write_in_full(fd_out, buf, size) < 0)
 		return error(_("packet write failed"));
 	return 0;
 }
@@ -242,19 +250,21 @@ void packet_buf_write_len(struct strbuf *buf, const char *data, size_t len)
 	packet_trace(data, len, 1);
 }
 
-int write_packetized_from_fd(int fd_in, int fd_out)
+int write_packetized_from_fd(int fd_in, int fd_out,
+			     struct packet_scratch_space *scratch)
 {
-	static char buf[LARGE_PACKET_DATA_MAX];
 	int err = 0;
 	ssize_t bytes_to_write;
 
 	while (!err) {
-		bytes_to_write = xread(fd_in, buf, sizeof(buf));
+		bytes_to_write = xread(fd_in, scratch->buffer,
+				       sizeof(scratch->buffer));
 		if (bytes_to_write < 0)
 			return COPY_READ_ERROR;
 		if (bytes_to_write == 0)
 			break;
-		err = packet_write_gently(fd_out, buf, bytes_to_write);
+		err = packet_write_gently(fd_out, scratch->buffer,
+					  bytes_to_write);
 	}
 	if (!err)
 		err = packet_flush_gently(fd_out);
diff --git a/pkt-line.h b/pkt-line.h
index 8c90daa59ef0..c0722aefe638 100644
--- a/pkt-line.h
+++ b/pkt-line.h
@@ -5,6 +5,13 @@
 #include "strbuf.h"
 #include "sideband.h"
 
+#define LARGE_PACKET_MAX 65520
+#define LARGE_PACKET_DATA_MAX (LARGE_PACKET_MAX - 4)
+
+struct packet_scratch_space {
+	char buffer[LARGE_PACKET_DATA_MAX]; /* does not include header bytes */
+};
+
 /*
  * Write a packetized stream, where each line is preceded by
  * its length (including the header) as a 4-byte hex number.
@@ -32,7 +39,7 @@ void packet_buf_write(struct strbuf *buf, const char *fmt, ...) __attribute__((f
 void packet_buf_write_len(struct strbuf *buf, const char *data, size_t len);
 int packet_flush_gently(int fd);
 int packet_write_fmt_gently(int fd, const char *fmt, ...) __attribute__((format (printf, 2, 3)));
-int write_packetized_from_fd(int fd_in, int fd_out);
+int write_packetized_from_fd(int fd_in, int fd_out, struct packet_scratch_space *scratch);
 int write_packetized_from_buf(const char *src_in, size_t len, int fd_out);
 
 /*
@@ -213,8 +220,7 @@ enum packet_read_status packet_reader_read(struct packet_reader *reader);
 enum packet_read_status packet_reader_peek(struct packet_reader *reader);
 
 #define DEFAULT_PACKET_MAX 1000
-#define LARGE_PACKET_MAX 65520
-#define LARGE_PACKET_DATA_MAX (LARGE_PACKET_MAX - 4)
+
 extern char packet_buffer[LARGE_PACKET_MAX];
 
 struct packet_writer {
-- 
gitgitgadget

