Re: [PATCH] remote-curl: don't hang when a server dies before any output

From: Jeff King <peff@peff.net>
Date: Nov 14, 2016, 23:48 UTC
Message-ID: <20161114234847.2nexsgedpg7zvrr5@sigill.intra.peff.net>
In-Reply-To: <a57cc9c4a0a840baab5b8123fac9388b@exmbdft7.ad.twosigma.com>

On Mon, Nov 14, 2016 at 11:25:30PM +0000, David Turner wrote:

Show 9 quoted lines

> > But it does seem to work. At least it doesn't seem to break anything
> > in the test suite, and it fixes the new tests you added. I'd worry
> > that there's some obscure case where the response isn't packetized
> > in the same way.
> 
> Overall, this looks good to me.  The state machine is pretty clean. I
> think I would have used a tiny buffer for the length field, and then I
> would have regretted it.  Your way looks nicer than my unwritten patch
> would have looked.

Heh, I started it that way but you end up dealing with the same states (they're just implicit in your "how big is my temp buffer" field).

Show 10 quoted lines

> > +#define READ_ONE_HEX(shift) do { \
> > +	int val = hexval(buf[0]); \
> > +	if (val < 0) { \
> > +		warning("error on %d", *buf); \
> > +		rpc->pktline_state = RPC_PKTLINE_ERROR; \
> > +		return; \
> > +	} \
> > +	rpc->pktline_len |= val << shift; \
> 
> Nit: parenthesize shift here, since it is a parameter to a macro.

Yeah, I'm often a bit slack on these one-off inside-a-function macros. But it does not hurt to to be careful.

I'll make that change and then try to wrap this up with a commit message. I plan to steal your tests, if that's OK.

-Peff