Re: [PATCH v2 1/2] Change how HTTP response body is returned

From: Jeff King <peff@peff.net>
Date: Jan 4, 2019, 10:30 UTC
Message-ID: <20190104103042.GB26185@sigill.intra.peff.net>
In-Reply-To: <20181229194447.157763-2-masayasuzuki@google.com>

On Sat, Dec 29, 2018 at 11:44:46AM -0800, Masaya Suzuki wrote:

Show 15 quoted lines

> +/*
> + * A callback for CURLOPT_WRITEFUNCTION. The return value is the bytes consumed
> + * from ptr.
> + */
>  static size_t rpc_in(char *ptr, size_t eltsize,
>  		size_t nmemb, void *buffer_)
>  {
>  	size_t size = eltsize * nmemb;
> -	struct rpc_state *rpc = buffer_;
> +	struct rpc_in_data *data = buffer_;
> +	long response_code;
> +
> +	if (curl_easy_getinfo(data->slot->curl, CURLINFO_RESPONSE_CODE,
> +			      &response_code) != CURLE_OK)
> +		return size;

This hunk was unexpected to me. The function here is just writing out the data, and I expected we'd handle the error after the whole transfer is done. But we do that anyway eventually via run_slot() (which uses handle_curl_result). I guess the goal here is to start throwing away data when we see an error, rather than storing it?

That makes some sense, though I do wonder if there's any case where curl would call our WRITEFUNCTION before it knows the HTTP status. That implies a body before our header, which seems impossible, though.

> +	if (response_code != 200)
> +		return size;

The current behavior with CURLOPT_FAILONERROR treats codes >= 400 as an error. And in handle_curl_result(), we treat >= 300 as an error (since we only see 3xx for a disabled redirect). I suppose it's unlikely for us to see any success code besides 200, but we probably ought to be following the same rules here.

-Peff