On 2/26/21 2:21 AM, Jeff King wrote:
On Wed, Feb 17, 2021 at 09:48:37PM +0000, Jeff Hostetler via GitGitGadget wrote:
Change the API of `write_packetized_from_fd()` to accept a scratch space
argument from its caller to avoid similar issues here.
OK, but...
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);
Isn't this just putting the buffer onto the stack anyway? Your
scratch_space struct is really just a big array. You'd want to make
it static here, but then we haven't really solved anything. :)
Yeah, I was letting the caller decide how to provide the buffer.
They could put it on the stack or allocate it once across a whole
set of files or use a static buffer -- the caller has context for
what works best that we don't have here. For example, the caller
may know that is not in threaded code at all, but we cannot assume
that here.
I think instead that:
-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);
}
...just heap-allocating the buffer in this function would be fine. It's
one malloc for the whole sequence of pktlines, which is unlikely to be a
problem.
Right, I think it would be fine to malloc it here, but I didn't
want to assume that everyone would think that.
I'll change it.
Thanks
Jeff
