Re: [Qemu-block] [Qemu-devel] [PATCH 2/4] block/curl: Fix return value from curl_read_cb

[Max Reitz]

On 25.10.2016 20:37, Eric Blake wrote:
> On 10/24/2016 09:54 PM, Max Reitz wrote:
>> While commit 38bbc0a580f9f10570b1d1b5d3e92f0e6feb2970 is correct in that
>> the callback is supposed to return the number of bytes handled; what it
>> does not mention is that libcurl will throw an error if the callback did
>> not "handle" all of the data passed to it.
>>
>> Therefore, if the callback receives some data that it cannot handle
>> (either because the receive buffer has not been set up yet or because it
>> would not fit into the receive buffer) and we have to ignore it, we
>> still have to report that the data has been handled.
>>
>> Obviously, this should not happen normally. But it does happen at least
>> for FTP connections where some data (that we do not expect) may be
>> generated when the connection is established.
> 
> Just to make sure, we aren't losing data by reporting this value, but
> merely letting curl know that our callback has "dealt" with the data, so
> that we don't error out, in order to get a second chance at the same
> data later on?

I don't know about previous or future versions of libcurl, but I looked
at the current git code and it really just does two things with the
return code:

(1) Check whether it's a special code for pausing the connection, if so,
do what's necessary.

(2) If it is not, check whether it's different from the number of bytes
passed: If it is, report an error and abort the original transfer (which
in the case of establishing the connection means basically dropping the
connection).

Thus, curl will never save the data we didn't handle for later or
something like that. It's just an error if we don't report to have
handled everything.


This is also supported by the man page CURLOPT_WRITEFUNCTION(3):

> Your callback should return the number of bytes actually taken care
> of. If that amount differs from the amount passed to your callback
> function, it'll signal an error condition to the library. This will
> cause the transfer to get aborted and the libcurl function used will
> return CURLE_WRITE_ERROR.

Therefore, I don't think this is going to be changed in the future.

Max

> Reviewed-by: Eric Blake <address@hidden>
> 
> But given that it undoes 38bbc0a, I'd rather that it gets reviewed by
> Matthew and/or tested by Richard.
>

signature.asc
Description: OpenPGP digital signature