[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [PATCH v2 3/7] curl: Check completion in curl_multi_do(
From: |
John Snow |
Subject: |
Re: [Qemu-block] [PATCH v2 3/7] curl: Check completion in curl_multi_do() |
Date: |
Tue, 10 Sep 2019 21:16:59 -0400 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.8.0 |
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 <address@hidden>
>
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 <address@hidden>
- [Qemu-block] [PATCH v2 0/7] block/curl: Fix hang and potential crash, Max Reitz, 2019/09/10
- [Qemu-block] [PATCH v2 1/7] curl: Keep pointer to the CURLState in CURLSocket, Max Reitz, 2019/09/10
- [Qemu-block] [PATCH v2 2/7] curl: Keep *socket until the end of curl_sock_cb(), Max Reitz, 2019/09/10
- [Qemu-block] [PATCH v2 3/7] curl: Check completion in curl_multi_do(), Max Reitz, 2019/09/10
- [Qemu-block] [PATCH v2 4/7] curl: Pass CURLSocket to curl_multi_do(), Max Reitz, 2019/09/10
- [Qemu-block] [PATCH v2 5/7] curl: Report only ready sockets, Max Reitz, 2019/09/10
- [Qemu-block] [PATCH v2 6/7] curl: Handle success in multi_check_completion, Max Reitz, 2019/09/10
- [Qemu-block] [PATCH v2 7/7] curl: Check curl_multi_add_handle()'s return code, Max Reitz, 2019/09/10