RE: [PATCH] remote-curl: don't hang when a server dies before any output
Show 29 quoted lines
> -----Original Message-----
> From: Jeff King [mailto:peff@peff.net]
> Sent: Monday, November 14, 2016 2:41 PM
> To: David Turner
> Cc: git@vger.kernel.org; spearce@spearce.org
> Subject: Re: [PATCH] remote-curl: don't hang when a server dies before any
> output
>
> On Mon, Nov 14, 2016 at 01:24:31PM -0500, Jeff King wrote:
>
> > 2. Have remote-curl understand enough of the protocol that it can
> > abort rather than hang.
> >
> > I think that's effectively the approach of your patch, but for one
> > specific case. But could we, for example, make sure that everything
> > we proxy is a complete set of pktlines and ends with a flush? And
> > if not, then we hang up on fetch-pack.
> >
> > I _think_ that would work, because even the pack is always encased
> > in pktlines for smart-http.
>
> So something like this. It turned out to be a lot uglier than I had hoped
> because we get fed the data from curl in odd-sized chunks, so we need a
> state machine.
>
> 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.
Show 8 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.