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

> 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. :)

> 
> 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.