On Mon, May 18, 2020 at 11:47:17AM -0400, Denton Liu wrote: > Changes since v1: > > * Remove fallthrough in switch in favour of just extracting the common > call out of the switch in patch 3 > > * Add more detail in function comment and use `const char linelen[4]` in > patch 4 > > * Implement most of Peff's suggestions[0] in patch 5 > > * Only operate on stateless_connect() in patch 5 > > * Add tests in patch 5 > > * Drop "remote-curl: ensure last packet is a flush" in favour of > "stateless-connect: send response end packet" Overall this looks pretty cleanly done. I left a few minor comments throughout, but the real question is whether we prefer the "0002" packet in the last one, or if we instead insist that the response end in a flush. At first glance, the "0002" seems like it's more flexible, because we're making fewer assumptions about what's being transferred over the stateless-connect channel. But in reality it still has to be pktlines (because we're checking them for incomplete or invalid packets already). So all it really buys us is that the server response doesn't have to end with a flush packet. So I dunno. The "0002" solution is slightly more flexible, but I'm not sure it helps in practice. And it does eat up one of our two remaining special packet markers. There is another solution, which would allow arbitrary data over stateless-connect: adding an extra level of pktline framing between the helper and the parent process. But that's rather ugly (inner pktlines may be split across outer ones, so you have to do a bunch of buffer reassembly). I think that's actually how v0 http works, IIRC. IMHO it probably isn't worth pursuing. That extra layer wrecks the elegance to the v2 stateless-connect approach, and we really do expect only pktlines to go over it. So I think either of your solutions (enforcing a final flush, or the 0002 packet) is preferable. I'm on the fence between them. -Peff
