proto_pipe: use netbuf_read with a big buffer

[gperciva]

I'm confident about the end result, but not 100% certain about the best git history to get there.

[gperciva]

Nope, of course that doesn't work. netbuf_read happily gives me a large buffer (4096 bytes), and we can't shove all that to proto_crypt_enc at once. So that's why we have to have the "process multiple chunks of a large buffer" code in proto_pipe.c before we make the switch to netbuf_read.c.

(I had discovered this a few months ago, but I forgot when I was looking at the code again this weekend.)

[gperciva]

This PR is organized around minimizing the diff of the actual "switch to netbuf_read" commit, 6351106. Arguably, some of the changes (such as 7c1a793) would make more sense if that was combined with the "switch to netbuf_read" commit rather than appearing on its own.

I was tempted to split into 2 separate PRs: one which does the 4 refactoring commits, then the other one which does the actual import and switch. But I couldn't really justify that to myself, because the "handle a larger input buffer" code is completely irrelevant until we actually switch to netbuf_read. (Whereas with #331, I could justify it by saying that it simplified the code.)

[cperciva]

Can you put import netbuf_{read, write} from libcperciva into a separate PR? I'm assuming we're going to want both of them eventually; I think it's clearer to separate "import existing code" from "change stuff".

[gperciva]

Oh, ok. I thought that you normally didn't want PRs that imported stuff without using it.

[gperciva]

Why the repeated peek call?

I thought it was cleaner to let netbuf_read handle the bookkeeping. If we do it ourselves, then we need to keep track of inpos, use &inbuf[inpos] instead of just inbuf, and handle the addition & subtraction at the end of the loop. Granted, that's very common C code, so it doesn't really decrease the readability for anybody who should be reading this.

I've changed it as you requested.

[cperciva]

On 12/5/21 17:34, Graham Percival wrote:

Oh, ok. I thought that you normally didn't want PRs that imported stuff without using it.

Generally speaking I don't want to import a bunch of unnecessary code, but as long as we know that we're going to be using it later I think it's fine.

-- Colin Percival Security Officer Emeritus, FreeBSD | The power to serve Founder, Tarsnap | www.tarsnap.com | Online backups for the truly paranoid

[cperciva]

Why the repeated peek call?

I thought it was cleaner to let netbuf_read handle the bookkeeping. If we do it ourselves, then we need to keep track of inpos, use &inbuf[inpos] instead of just inbuf, and handle the addition & subtraction at the end of the loop. Granted, that's very common C code, so it doesn't really decrease the readability for anybody who should be reading this.

I've changed it as you requested.

FWIW, I think the biggest reason this feels cleaner to me is that this way the handling of input mirrors the handling of output -- in both cases we have a buffer we're moving through chunk by chunk.

[cperciva]

Actually, maybe hold off on importing netbuf_write for now -- I'm not sure it really serves any purpose. It lets us queue writes but we're better off relying on the OS socket buffers since they provide backpressure; if we switch over to netbuf_write we could end up buffering an unlimited amount of data internally if the outgoing connection is slow.

[cperciva]

BTW we probably want to have a netbuf_read_setbuflen since right now netbuf_read only reads a maximum of 4 kB at once.

[gperciva]

I've added fixes, but shall I rebase this onto master so that it benefits from the netbuf_read import?

[cperciva]

Fixes look good. Please rebase.

[gperciva]

Rebased, and renamed some variables.