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

From: Jeff King <peff@peff.net>
Date: Feb 26, 2021, 20:43 UTC
Message-ID: <YDldiMC6qx0z/NBf@coredump.intra.peff.net>
In-Reply-To: <9304ac66-f493-2150-95e3-15303c914ee3@jeffhostetler.com>

On Fri, Feb 26, 2021 at 02:52:22PM -0500, Jeff Hostetler wrote:

Show 19 quoted lines

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

Yeah, I think it's successfully pushed the problem up to the caller. But it introduced a _new_ problem in putting the large buffer on the stack. So if this were "static struct packet_scratch_space scratch", I think we'd be OK.

And perhaps that would meet your needs (if you just need to call write_packed_from_fd() in a thread, and not this other caller).

But I do think the heap approach is nice in that it keeps the interface clean, and I think the performance should be comparable.

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

-Peff