diff mbox series

[1/8] pkt-line: remove buffer arg from write_packetized_from_fd_no_flush()

Message ID d6ea7688dfb2536312b627f7ed47ff7f42091f60.1614889047.git.gitgitgadget@gmail.com (mailing list archive)
State New, archived
Headers show
Series Simple IPC Cleanups | expand

Commit Message

Jeff Hostetler March 4, 2021, 8:17 p.m. UTC 
From: Jeff Hostetler <jeffhost@microsoft.com>

Remove the scratch buffer argument from `write_packetized_from_fd_no_flush()`
and allocate a temporary buffer within the function.

In 3e4447f1ea (pkt-line: eliminate the need for static buffer in
packet_write_gently(), 2021-02-13) we added the argument in order to
get rid of a static buffer to make the routine safe in multi-threaded
contexts and to avoid putting a very large buffer on the stack.  This
delegated the problem to the caller who has more knowledge of the use
case and can best decide the most efficient way to provide such a
buffer.

However, in practice, the (currently, only) caller just created the
buffer on the stack, so we might as well just allocate it within the
function and restore the original API.

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

Comments

Junio C Hamano March 4, 2021, 10:55 p.m. UTC | #1 
"Jeff Hostetler via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Jeff Hostetler <jeffhost@microsoft.com>
>
> Remove the scratch buffer argument from `write_packetized_from_fd_no_flush()`
> and allocate a temporary buffer within the function.
>
> In 3e4447f1ea (pkt-line: eliminate the need for static buffer in
> packet_write_gently(), 2021-02-13) we added the argument in order to
> get rid of a static buffer to make the routine safe in multi-threaded
> contexts and to avoid putting a very large buffer on the stack.  This
> delegated the problem to the caller who has more knowledge of the use
> case and can best decide the most efficient way to provide such a
> buffer.
>
> However, in practice, the (currently, only) caller just created the
> buffer on the stack, so we might as well just allocate it within the
> function and restore the original API.

Hmph, I would have expected, since we already have changed the
callchain to pass the buffer down, that we'd keep the structure and
update the caller to heap-allocate, but I think I like the result of
this patch better.  It's not like the caller can allocate and reuse
a single buffer with repeated calls to the function; rather, the
caller makes a single call that results in relaying many bytes by
the helper, all done in a loop, and it is sufficient for the helper
to allocate/deallocate outside of its loop to reuse the buffer.
diff mbox series

Patch

diff --git a/convert.c b/convert.c
index 9f44f00d841f..516f1095b06e 100644
--- a/convert.c
+++ b/convert.c
@@ -883,10 +883,9 @@  static int apply_multi_file_filter(const char *path, const char *src, size_t len
 	if (err)
 		goto done;
 
-	if (fd >= 0) {
-		struct packet_scratch_space scratch;
-		err = write_packetized_from_fd_no_flush(fd, process->in, &scratch);
-	} else
+	if (fd >= 0)
+		err = write_packetized_from_fd_no_flush(fd, process->in);
+	else
 		err = write_packetized_from_buf_no_flush(src, len, process->in);
 	if (err)
 		goto done;
diff --git a/pkt-line.c b/pkt-line.c
index 18ecad65e08c..c933bb95586d 100644
--- a/pkt-line.c
+++ b/pkt-line.c
@@ -250,22 +250,25 @@  void packet_buf_write_len(struct strbuf *buf, const char *data, size_t len)
 	packet_trace(data, len, 1);
 }
 
-int write_packetized_from_fd_no_flush(int fd_in, int fd_out,
-				      struct packet_scratch_space *scratch)
+int write_packetized_from_fd_no_flush(int fd_in, int fd_out)
 {
 	int err = 0;
 	ssize_t bytes_to_write;
+	char *buf = xmalloc(LARGE_PACKET_DATA_MAX);
 
 	while (!err) {
-		bytes_to_write = xread(fd_in, scratch->buffer,
-				       sizeof(scratch->buffer));
-		if (bytes_to_write < 0)
-			return COPY_READ_ERROR;
+		bytes_to_write = xread(fd_in, buf, LARGE_PACKET_DATA_MAX);
+		if (bytes_to_write < 0) {
+			err = COPY_READ_ERROR;
+			break;
+		}
 		if (bytes_to_write == 0)
 			break;
-		err = packet_write_gently(fd_out, scratch->buffer,
-					  bytes_to_write);
+		err = packet_write_gently(fd_out, buf, bytes_to_write);
 	}
+
+	free(buf);
+
 	return err;
 }
 
diff --git a/pkt-line.h b/pkt-line.h
index e347fe46832a..54eec72dc0af 100644
--- a/pkt-line.h
+++ b/pkt-line.h
@@ -8,10 +8,6 @@ 
 #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.
@@ -39,7 +35,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_no_flush(int fd_in, int fd_out, struct packet_scratch_space *scratch);
+int write_packetized_from_fd_no_flush(int fd_in, int fd_out);
 int write_packetized_from_buf_no_flush(const char *src_in, size_t len, int fd_out);
 
 /*