Re: [Qemu-devel] [Qemu-block] [PATCH v2 3/7] curl: Check completion in curl_multi_do()

[John Snow <jsnow@redhat.com>]

On 9/10/19 12:11 PM, Maxim Levitsky wrote:
> On Tue, 2019-09-10 at 14:41 +0200, Max Reitz wrote:
>> While it is more likely that transfers complete after some file
>> descriptor has data ready to read, we probably should not rely on it.
>> Better be safe than sorry and call curl_multi_check_completion() in
>> curl_multi_do(), too, just like it is done in curl_multi_read().
>>
>> With this change, curl_multi_do() and curl_multi_read() are actually the
>> same, so drop curl_multi_read() and use curl_multi_do() as the sole FD
>> handler.
> 
> I understand the reasoning, but I still a bit worry that this
> could paper over some bug/race in the future.
> If curl asks us only to deal with write, that would mean
> that it doesn't expect any data to be received.
> 
> Do you by a chance have an example, of this patch
> affecting the code? Maybe when a unexpected error reply
> is received from the server?
> 
> I don't really know the CURL library, so I probably missed
> something important.
> 
> Other than that,
> Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com>
> 

In this case, it's because I had some doubts in V1 about when we call
the post-completion cleanup code. It didn't look obvious at a glance.

This just makes it simpler.

I don't think that by checking *more* things we're going to paper over
some race condition -- it's the opposite. If anything, this will explode
sooner rather than later.

So I think it's OK, naturally.

Reviewed-by: John Snow <jsnow@redhat.com>